| Message ID | 20221211120626.12210-1-johan@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB: drop misleading usb_set_intfdata() kernel doc | expand |
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
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
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
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
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
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
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
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
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
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
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
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);
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(-)