lkml.org 
[lkml]   [2022]   [Nov]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v4 2/6] can: etas_es58x: add devlink support
On Mon. 28 Nov. 2022 at 00:41, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sun, Nov 27, 2022 at 02:10:32PM +0900, Vincent MAILHOL wrote:
> > > Should devlink_free() be after usb_set_inftdata()?
> >
> > A look at
> > $ git grep -W "usb_set_intfdata(.*NULL)"
> >
> > shows that the two patterns (freeing before or after
> > usb_set_intfdata()) coexist.
> >
> > You are raising an important question here. usb_set_intfdata() does
> > not have documentation that freeing before it is risky. And the
> > documentation of usb_driver::disconnect says that:
> > "@disconnect: Called when the interface is no longer accessible,
> > usually because its device has been (or is being) disconnected
> > or the driver module is being unloaded."
> > Ref: https://elixir.bootlin.com/linux/v6.1-rc6/source/include/linux/usb.h#L1130
> >
> > So the interface no longer being accessible makes me assume that the
> > order does not matter. If it indeed matters, then this is a foot gun
> > and there is some clean-up work waiting for us on many drivers.
> >
> > @Greg, any thoughts on whether or not the order of usb_set_intfdata()
> > and resource freeing matters or not?
>
> In fact, drivers don't have to call usb_set_intfdata(NULL) at all; the
> USB core does it for them after the ->disconnect() callback returns.

Interesting. This fact is widely unknown, cf:
$ git grep "usb_set_intfdata(.*NULL)" | wc -l
215

I will do some clean-up later on, at least for the CAN USB drivers.

> But if a driver does make the call, it should be careful to ensure that
> the call happens _after_ the driver is finished using the interface-data
> pointer. For example, after all outstanding URBs have completed, if the
> completion handlers will need to call usb_get_intfdata().

ACK. I understand that it should be called *after* the completion of
any ongoing task.

My question was more on:

devlink_free(priv_to_devlink(es58x_dev));
usb_set_intfdata(intf, NULL);

VS.

usb_set_intfdata(intf, NULL);
devlink_free(priv_to_devlink(es58x_dev));

From your comments, I understand that both are fine.

> Remember, the interface-data pointer is owned by the driver. Nothing
> else in the kernel uses it. So the driver merely has to be careful not
> to clear the pointer while it is still using it.

Thanks for your comments!


Yours sincerely,
Vincent Mailhol

\
 
 \ /
  Last update: 2022-11-28 02:35    [W:0.111 / U:0.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site