[PATCH] USB: drop misleading usb_set_intfdata() kernel doc

[PATCH] USB: drop misleading usb_set_intfdata() kernel doc

[Johan Hovold]

The struct device driver-data pointer is used for any data that a driver
may need in various callbacks while bound to the device. For
convenience, subsystems typically provide wrappers such as
usb_set_intfdata() of the generic accessor functions for use in bus
callbacks.

There is generally no longer any need for a driver to clear the pointer,
but since commit 0998d0631001 ("device-core: Ensure drvdata = NULL when
no driver is bound") the driver-data pointer is set to NULL by driver
core post unbind anyway.

For historical reasons, USB core also clears this pointer when an
explicitly claimed interface is released.

Due to a misunderstanding, a redundant and misleading kernel doc comment
for usb_set_intfdata() was recently added which claimed that the driver
data pointer must not be cleared during disconnect before "all actions
[are] completed", which is both imprecise and incorrect.

Specifically, drivers like cdc-acm which claim additional interfaces use
the driver-data pointer as a flag which is cleared when the first
interface is unbound. As long as a driver does not do something odd like
dereference the pointer in, for example, completion callbacks, this can
be done at any time during disconnect. And in any case this is no
different than for any other resource, like the driver data itself,
which may be freed by the disconnect callback.

Drop the incorrect and unnecessary comment.

Fixes: 27ef17849779 ("usb: add usb_set_intfdata() documentation")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 include/linux/usb.h | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/include/linux/usb.h b/include/linux/usb.h
index b47371d44d1b..010c681b8822 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -266,18 +266,6 @@ static inline void *usb_get_intfdata(struct usb_interface *intf)
 	return dev_get_drvdata(&intf->dev);
 }
 
-/**
- * usb_set_intfdata() - associate driver-specific data with the interface
- * @intf: the usb interface
- * @data: pointer to the device priv structure or %NULL
- *
- * Drivers should use this function in their probe() to associate their
- * driver-specific data with the usb interface.
- *
- * When disconnecting, the core will take care of setting @intf back to %NULL,
- * so no actions are needed on the driver side. The interface should not be set
- * to %NULL before all actions completed (e.g. no outsanding URB remaining).
- */
 static inline void usb_set_intfdata(struct usb_interface *intf, void *data)
 {
 	dev_set_drvdata(&intf->dev, data);
-- 
2.37.4

Re: [PATCH] USB: drop misleading usb_set_intfdata() kernel doc

[Oliver Neukum]

On 11.12.22 13:06, Johan Hovold wrote:

> Due to a misunderstanding, a redundant and misleading kernel doc comment
> for usb_set_intfdata() was recently added which claimed that the driver
> data pointer must not be cleared during disconnect before "all actions
> [are] completed", which is both imprecise and incorrect.

OK, but is that a reason to remove all kerneldoc? Kerneldoc is generally
a good thing. And if a pointer is NULLed by driver core, that will need
to be in it. IMHO you'd better just remove the questionable part of the
kerneldoc.

	Regards
		Oliver

Re: [PATCH] USB: drop misleading usb_set_intfdata() kernel doc

[Johan Hovold]

On Mon, Dec 12, 2022 at 11:19:00AM +0100, Oliver Neukum wrote:
> On 11.12.22 13:06, Johan Hovold wrote:
> 
> > Due to a misunderstanding, a redundant and misleading kernel doc comment
> > for usb_set_intfdata() was recently added which claimed that the driver
> > data pointer must not be cleared during disconnect before "all actions
> > [are] completed", which is both imprecise and incorrect.
> 
> OK, but is that a reason to remove all kerneldoc? Kerneldoc is generally
> a good thing. And if a pointer is NULLed by driver core, that will need
> to be in it. IMHO you'd better just remove the questionable part of the
> kerneldoc.

Yeah, I started off with just rewriting the kernel doc and removing the
obviously incorrect bits, but then there is essentially nothing left of
the documentation.

A driver does not need to care that the pointer is cleared by driver
core after the driver is unbound. The driver is gone.

So this information isn't really useful and trying to document it for
only one of the subsystem wrappers probably only adds to the confusion
(e.g. by signalling that this is in some way relevant information).

Johan

Re: [PATCH] USB: drop misleading usb_set_intfdata() kernel doc

[Oliver Neukum]

On 12.12.22 11:31, Johan Hovold wrote:
> On Mon, Dec 12, 2022 at 11:19:00AM +0100, Oliver Neukum wrote:
>> On 11.12.22 13:06, Johan Hovold wrote:
>>
>>> Due to a misunderstanding, a redundant and misleading kernel doc comment
>>> for usb_set_intfdata() was recently added which claimed that the driver
>>> data pointer must not be cleared during disconnect before "all actions
>>> [are] completed", which is both imprecise and incorrect.
>>
>> OK, but is that a reason to remove all kerneldoc? Kerneldoc is generally
>> a good thing. And if a pointer is NULLed by driver core, that will need
>> to be in it. IMHO you'd better just remove the questionable part of the
>> kerneldoc.
> 
> Yeah, I started off with just rewriting the kernel doc and removing the
> obviously incorrect bits, but then there is essentially nothing left of
> the documentation.

1. that the function exists and its purpose
2. its parameters

most kerneldoc isn't exactly a great revelation. Nevertheless it
serves a purpose.

> A driver does not need to care that the pointer is cleared by driver
> core after the driver is unbound. The driver is gone.

Is that true even with respect to sysfs?

	Regards
		Oliver

Re: [PATCH] USB: drop misleading usb_set_intfdata() kernel doc

[Johan Hovold]

On Mon, Dec 12, 2022 at 12:13:54PM +0100, Oliver Neukum wrote:
> 
> 
> On 12.12.22 11:31, Johan Hovold wrote:
> > On Mon, Dec 12, 2022 at 11:19:00AM +0100, Oliver Neukum wrote:
> >> On 11.12.22 13:06, Johan Hovold wrote:
> >>
> >>> Due to a misunderstanding, a redundant and misleading kernel doc comment
> >>> for usb_set_intfdata() was recently added which claimed that the driver
> >>> data pointer must not be cleared during disconnect before "all actions
> >>> [are] completed", which is both imprecise and incorrect.
> >>
> >> OK, but is that a reason to remove all kerneldoc? Kerneldoc is generally
> >> a good thing. And if a pointer is NULLed by driver core, that will need
> >> to be in it. IMHO you'd better just remove the questionable part of the
> >> kerneldoc.
> > 
> > Yeah, I started off with just rewriting the kernel doc and removing the
> > obviously incorrect bits, but then there is essentially nothing left of
> > the documentation.
> 
> 1. that the function exists and its purpose

That should be apparent from the function name (and implementation).

> 2. its parameters

Apparent from the prototype.

But sure, it would not show up in generated documentation (like many
other functions).
 
> most kerneldoc isn't exactly a great revelation. Nevertheless it
> serves a purpose.

Yeah, we have a lot of

	/**
	 * set_x_to_y() - set x to y
	 * @x: the x
	 * @y: the y
	 */

it seems. Not sure how much value there is in that, though.

And in this case there was also no kernel doc for usb_get_intfdata()
which is equally self documenting. Why add redundant docs for only one
of these functions?

I'd rather drop this particular documentation which was added due to a
misunderstanding then go down the rabbit hole of adding mindless kernel
doc to every helper we have.

> > A driver does not need to care that the pointer is cleared by driver
> > core after the driver is unbound. The driver is gone.
> 
> Is that true even with respect to sysfs?

Yes. The (device group) attributes are removed by driver core before
->remove() is called, otherwise you'd have an even bigger issue with the
driver data itself which is typically deallocated before the pointer is
cleared.

Johan

Re: [PATCH] USB: drop misleading usb_set_intfdata() kernel doc

[Oliver Neukum]

On 12.12.22 14:14, Johan Hovold wrote:
> On Mon, Dec 12, 2022 at 12:13:54PM +0100, Oliver Neukum wrote:

>> 1. that the function exists and its purpose
> 
> That should be apparent from the function name (and implementation).
> 
>> 2. its parameters
> 
> Apparent from the prototype.
> 
> But sure, it would not show up in generated documentation (like many
> other functions).

purpose, yes. existence, no.

> it seems. Not sure how much value there is in that, though.

I am afraid there is no consensus answer to that question.
  
> And in this case there was also no kernel doc for usb_get_intfdata()
> which is equally self documenting. Why add redundant docs for only one
> of these functions?

Because knowing that one of them exists makes the other much more
obvious.

> I'd rather drop this particular documentation which was added due to a
> misunderstanding then go down the rabbit hole of adding mindless kernel
> doc to every helper we have.

Yes, but those are not the alternatives.
Removing the perfectly good part of the kerneldoc is a needless regression,
albeit a minor one.

> Yes. The (device group) attributes are removed by driver core before
> ->remove() is called, otherwise you'd have an even bigger issue with the
> driver data itself which is typically deallocated before the pointer is

So what happens if user space calls read() or write() on an existing fd?
Sorry, but this is an issue we need to be sure about.

	Regards
		Oliver

Re: [PATCH] USB: drop misleading usb_set_intfdata() kernel doc

[Johan Hovold]

On Mon, Dec 12, 2022 at 02:27:46PM +0100, Oliver Neukum wrote:
> On 12.12.22 14:14, Johan Hovold wrote:
> > On Mon, Dec 12, 2022 at 12:13:54PM +0100, Oliver Neukum wrote:

> > And in this case there was also no kernel doc for usb_get_intfdata()
> > which is equally self documenting. Why add redundant docs for only one
> > of these functions?
> 
> Because knowing that one of them exists makes the other much more
> obvious.

I doubt anyone finds out about this function by reading generated kernel
documentation. You look at a driver, grep the function and look at the
single-line implementation.

Driver data is such as integral part of the driver model so it's kinda
hard to miss. Still dev_set_drvdata() also has no kernel doc either.
 
> > I'd rather drop this particular documentation which was added due to a
> > misunderstanding then go down the rabbit hole of adding mindless kernel
> > doc to every helper we have.
> 
> Yes, but those are not the alternatives.
> Removing the perfectly good part of the kerneldoc is a needless regression,
> albeit a minor one.

But it was never perfectly good. It claims that a driver "should" use it,
when it may not need to (think simple driver using devres) and talks
about driver core resetting the pointer which is irrelevant.

> > Yes. The (device group) attributes are removed by driver core before
> > ->remove() is called, otherwise you'd have an even bigger issue with the
> > driver data itself which is typically deallocated before the pointer is
> 
> So what happens if user space calls read() or write() on an existing fd?
> Sorry, but this is an issue we need to be sure about.

No need to be sorry, and I've looked at this before. kernfs handles the
serialisation.

But as I mentioned above, this is again irrelevant for the question at
hand as the driver data is freed before the pointer is set to NULL.

Johan

Re: [PATCH] USB: drop misleading usb_set_intfdata() kernel doc

[Oliver Neukum]

On 12.12.22 14:40, Johan Hovold wrote:
> On Mon, Dec 12, 2022 at 02:27:46PM +0100, Oliver Neukum wrote:
>> On 12.12.22 14:14, Johan Hovold wrote:
>>> On Mon, Dec 12, 2022 at 12:13:54PM +0100, Oliver Neukum wrote:
> 
>>> And in this case there was also no kernel doc for usb_get_intfdata()
>>> which is equally self documenting. Why add redundant docs for only one
>>> of these functions?
>>
>> Because knowing that one of them exists makes the other much more
>> obvious.
> 
> I doubt anyone finds out about this function by reading generated kernel
> documentation. You look at a driver, grep the function and look at the
> single-line implementation.

Look, we cannot solve the issue whether kerneldoc is a good idea
in general here. If you want to open that can of worms on lkml,
by all means. go for it.
But failing that, silently eliminating it is just not nice.

> But it was never perfectly good. It claims that a driver "should" use it,
> when it may not need to (think simple driver using devres) and talks

If you are presented with an interface something needs to be done
specific to that interface.

> about driver core resetting the pointer which is irrelevant.

But correct and topical. I am not arguing what people should or should
mot know.
  
If you just remove the last sentence, all will be well. And if you
insist replace "should" with "can".

	Regards
		Oliver

Re: [PATCH] USB: drop misleading usb_set_intfdata() kernel doc

[Johan Hovold]

On Mon, Dec 12, 2022 at 03:04:50PM +0100, Oliver Neukum wrote:
> 
> 
> On 12.12.22 14:40, Johan Hovold wrote:
> > On Mon, Dec 12, 2022 at 02:27:46PM +0100, Oliver Neukum wrote:
> >> On 12.12.22 14:14, Johan Hovold wrote:
> >>> On Mon, Dec 12, 2022 at 12:13:54PM +0100, Oliver Neukum wrote:
> > 
> >>> And in this case there was also no kernel doc for usb_get_intfdata()
> >>> which is equally self documenting. Why add redundant docs for only one
> >>> of these functions?
> >>
> >> Because knowing that one of them exists makes the other much more
> >> obvious.
> > 
> > I doubt anyone finds out about this function by reading generated kernel
> > documentation. You look at a driver, grep the function and look at the
> > single-line implementation.
> 
> Look, we cannot solve the issue whether kerneldoc is a good idea
> in general here. If you want to open that can of worms on lkml,
> by all means. go for it.
> But failing that, silently eliminating it is just not nice.

It was just added. It's mostly misleading and incorrect. Hence revert,
rather than put the burden of adding proper documentation on the person
calling out the misunderstanding (which has already led to a series of
bogus patches).

> > But it was never perfectly good. It claims that a driver "should" use it,
> > when it may not need to (think simple driver using devres) and talks
> 
> If you are presented with an interface something needs to be done
> specific to that interface.

I'm not sure what you're saying here.

> > about driver core resetting the pointer which is irrelevant.
> 
> But correct and topical. I am not arguing what people should or should
> mot know.

The comment is also misleading as it signals that this is something you
need to care about.

> If you just remove the last sentence, all will be well. And if you
> insist replace "should" with "can".

Since you insist, I'll just rewrite the whole thing.

Johan

Re: [PATCH] USB: drop misleading usb_set_intfdata() kernel doc

[Alan Stern]

On Mon, Dec 12, 2022 at 03:14:49PM +0100, Johan Hovold wrote:
> On Mon, Dec 12, 2022 at 03:04:50PM +0100, Oliver Neukum wrote:
> > 
> > 
> > On 12.12.22 14:40, Johan Hovold wrote:
> > > On Mon, Dec 12, 2022 at 02:27:46PM +0100, Oliver Neukum wrote:
> > >> On 12.12.22 14:14, Johan Hovold wrote:
> > >>> On Mon, Dec 12, 2022 at 12:13:54PM +0100, Oliver Neukum wrote:
> > > 
> > >>> And in this case there was also no kernel doc for usb_get_intfdata()
> > >>> which is equally self documenting. Why add redundant docs for only one
> > >>> of these functions?
> > >>
> > >> Because knowing that one of them exists makes the other much more
> > >> obvious.
> > > 
> > > I doubt anyone finds out about this function by reading generated kernel
> > > documentation. You look at a driver, grep the function and look at the
> > > single-line implementation.
> > 
> > Look, we cannot solve the issue whether kerneldoc is a good idea
> > in general here. If you want to open that can of worms on lkml,
> > by all means. go for it.
> > But failing that, silently eliminating it is just not nice.
> 
> It was just added. It's mostly misleading and incorrect. Hence revert,
> rather than put the burden of adding proper documentation on the person
> calling out the misunderstanding (which has already led to a series of
> bogus patches).
> 
> > > But it was never perfectly good. It claims that a driver "should" use it,
> > > when it may not need to (think simple driver using devres) and talks
> > 
> > If you are presented with an interface something needs to be done
> > specific to that interface.
> 
> I'm not sure what you're saying here.
> 
> > > about driver core resetting the pointer which is irrelevant.
> > 
> > But correct and topical. I am not arguing what people should or should
> > mot know.
> 
> The comment is also misleading as it signals that this is something you
> need to care about.
> 
> > If you just remove the last sentence, all will be well. And if you
> > insist replace "should" with "can".
> 
> Since you insist, I'll just rewrite the whole thing.

You're both missing the main point, which is that the USB core clears 
intfdata after a driver is unbound.  As a consequence, drivers don't 
need to worry about clearing intfdata themselves -- a fact which is 
_not_ easily apparent from the implementation.  This would be a useful 
thing to mention in the kerneldoc, as it may help prevent lots of 
drivers from making unnecessary function calls (like the ones that 
Vincent recently removed).

Of course, this doesn't mean that drivers can't clear intfdata if they 
want to, for example, if they use it as an internal flag.  But 
developers shouldn't feel that they _need_ to clear it as a sort of 
hygienic measure.

IMO it's worthwhile keeping the kerneldoc (but correcting it) so that it 
can get this point across.

Alan Stern

Re: [PATCH] USB: drop misleading usb_set_intfdata() kernel doc

[Johan Hovold]

On Mon, Dec 12, 2022 at 10:25:25AM -0500, Alan Stern wrote:

> > Since you insist, I'll just rewrite the whole thing.
> 
> You're both missing the main point, which is that the USB core clears 
> intfdata after a driver is unbound.

I assure you that that hasn't been missed. :)

> As a consequence, drivers don't 
> need to worry about clearing intfdata themselves -- a fact which is 
> _not_ easily apparent from the implementation.  This would be a useful 
> thing to mention in the kerneldoc, as it may help prevent lots of 
> drivers from making unnecessary function calls (like the ones that 
> Vincent recently removed).

My point is that the fact the USB core (and driver core) clears the
pointer after the driver is unbound is mostly irrelevant. The driver
would typically have freed the driver data at that point anyway, and the
interface must no longer be used by the driver (e.g. as it could be
bound to a new driver).

This is a fundamental property of the driver model and not something
that necessarily needs to be repeated for every function that operates
on a struct device (or a subsystem container like struct usb_interface).

> Of course, this doesn't mean that drivers can't clear intfdata if they 
> want to, for example, if they use it as an internal flag.  But 
> developers shouldn't feel that they _need_ to clear it as a sort of 
> hygienic measure.

Right.

> IMO it's worthwhile keeping the kerneldoc (but correcting it) so that it 
> can get this point across.

As you saw I fixed up the kernel doc in v2, and tried to get the point
about not having to clear the pointer across without getting into too
much detail.

The fact that USB core and driver core clears the pointer is an
implementation detail which in principle could change. The important
part is that drivers generally should not touch the struct device or
containing structure after unbind (and must do so with care otherwise).

Johan

Re: [PATCH] USB: drop misleading usb_set_intfdata() kernel doc

[Alan Stern]

On Mon, Dec 12, 2022 at 05:08:17PM +0100, Johan Hovold wrote:
> The fact that USB core and driver core clears the pointer is an
> implementation detail which in principle could change.

With the new kerneldoc, the fact that the USB core clears the pointer 
becomes part of the API.  It is no longer an implementation detail but 
rather a guarantee that drivers can depend on.  (I.e., if it does 
change, all the USB drivers will need to be audited.)

>  The important
> part is that drivers generally should not touch the struct device or
> containing structure after unbind (and must do so with care otherwise).

While that certainly is an important point, it is not the point of 
usb_set_intfdata() or its kerneldoc.  The main reason for adding the 
kerneldoc -- Vincent's original motivation -- was to inform coders that 
drivers generally don't need to worry about clearing the data pointer.

A subtext was that clearing the pointer while the driver is still using 
it can lead to problems, but I don't think we need to mention this.  
It's pretty obvious.

Alan Stern