| Message ID | 7332D45F-9BD3-4D0E-A5AF-9845353415A9@live.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | USB: core: add 'shutdown' callback to usb_driver | expand |
Replying to this since I forgot to add the author Kerem Karabay to cc, so adding now > On 5 Jul 2024, at 4:50 PM, Aditya Garg <gargaditya08@live.com> wrote: > > From: Kerem Karabay <kekrby@gmail.com> > > This simplifies running code on shutdown for USB drivers. > > Signed-off-by: Kerem Karabay <kekrby@gmail.com> > Signed-off-by: Aditya Garg <gargaditya08@live.com> > --- > drivers/usb/core/driver.c | 14 ++++++++++++++ > drivers/usb/storage/uas.c | 5 ++--- > include/linux/usb.h | 3 +++ > 3 files changed, 19 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c > index f58a0299f..dc0f86376 100644 > --- a/drivers/usb/core/driver.c > +++ b/drivers/usb/core/driver.c > @@ -514,6 +514,19 @@ static int usb_unbind_interface(struct device *dev) > return 0; > } > > +static void usb_shutdown_interface(struct device *dev) > +{ > + struct usb_interface *intf = to_usb_interface(dev); > + struct usb_driver *driver; > + > + if (!dev->driver) > + return; > + > + driver = to_usb_driver(dev->driver); > + if (driver->shutdown) > + driver->shutdown(intf); > +} > + > /** > * usb_driver_claim_interface - bind a driver to an interface > * @driver: the driver to be bound > @@ -1053,6 +1066,7 @@ int usb_register_driver(struct usb_driver *new_driver, struct module *owner, > new_driver->driver.bus = &usb_bus_type; > new_driver->driver.probe = usb_probe_interface; > new_driver->driver.remove = usb_unbind_interface; > + new_driver->driver.shutdown = usb_shutdown_interface; > new_driver->driver.owner = owner; > new_driver->driver.mod_name = mod_name; > new_driver->driver.dev_groups = new_driver->dev_groups; > diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c > index 2583ee981..591fa0379 100644 > --- a/drivers/usb/storage/uas.c > +++ b/drivers/usb/storage/uas.c > @@ -1221,9 +1221,8 @@ static void uas_disconnect(struct usb_interface *intf) > * hang on reboot when the device is still in uas mode. Note the reset is > * necessary as some devices won't revert to usb-storage mode without it. > */ > -static void uas_shutdown(struct device *dev) > +static void uas_shutdown(struct usb_interface *intf) > { > - struct usb_interface *intf = to_usb_interface(dev); > struct usb_device *udev = interface_to_usbdev(intf); > struct Scsi_Host *shost = usb_get_intfdata(intf); > struct uas_dev_info *devinfo = (struct uas_dev_info *)shost->hostdata; > @@ -1246,7 +1245,7 @@ static struct usb_driver uas_driver = { > .suspend = uas_suspend, > .resume = uas_resume, > .reset_resume = uas_reset_resume, > - .driver.shutdown = uas_shutdown, > + .shutdown = uas_shutdown, > .id_table = uas_usb_ids, > }; > > diff --git a/include/linux/usb.h b/include/linux/usb.h > index 25f8e62a3..5f3ae2186 100644 > --- a/include/linux/usb.h > +++ b/include/linux/usb.h > @@ -1194,6 +1194,7 @@ struct usbdrv_wrap { > * post_reset method is called. > * @post_reset: Called by usb_reset_device() after the device > * has been reset > + * @shutdown: Called at shut-down time to quiesce the device. > * @id_table: USB drivers use ID table to support hotplugging. > * Export this with MODULE_DEVICE_TABLE(usb,...). This must be set > * or your driver's probe function will never get called. > @@ -1245,6 +1246,8 @@ struct usb_driver { > int (*pre_reset)(struct usb_interface *intf); > int (*post_reset)(struct usb_interface *intf); > > + void (*shutdown)(struct usb_interface *intf); > + > const struct usb_device_id *id_table; > const struct attribute_group **dev_groups; > > -- > 2.42.0 >
On Fri, Jul 05, 2024 at 11:21:06AM +0000, Aditya Garg wrote: > From: Kerem Karabay <kekrby@gmail.com> > > This simplifies running code on shutdown for USB drivers. > Sorry, but this does not explain why this is needed at all :( Where did this change come from? What problem does it solve? Why should we take it? I think I know the answers to these questions, but you need to document it here as to why it is needed (please read the kernel documentation for how to write a good changelog text and subject line.) thanks, greg k-h
Hi Greg > On 6 Jul 2024, at 1:43 PM, gregkh@linuxfoundation.org wrote: > > On Fri, Jul 05, 2024 at 11:21:06AM +0000, Aditya Garg wrote: >> From: Kerem Karabay <kekrby@gmail.com> >> >> This simplifies running code on shutdown for USB drivers. >> > > Sorry, but this does not explain why this is needed at all :( > > Where did this change come from? What problem does it solve? Why > should we take it? > Currently there is no standardized method for USB drivers to handle shutdown events. This patch simplifies running code on shutdown for USB devices by adding a shutdown callback to usb_driver. It also implements the new method to existing "USB Attached SCSI” driver that required shutdown Is this good enough? > I think I know the answers to these questions, but you need to document > it here as to why it is needed (please read the kernel documentation for > how to write a good changelog text and subject line.) The subject looks fine to me. If you think it can be improved, suggestions shall be appreciated. Thanks Aditya
On Sat, Jul 06, 2024 at 08:56:05AM +0000, Aditya Garg wrote: > > Hi Greg > > > On 6 Jul 2024, at 1:43 PM, gregkh@linuxfoundation.org wrote: > > > > On Fri, Jul 05, 2024 at 11:21:06AM +0000, Aditya Garg wrote: > >> From: Kerem Karabay <kekrby@gmail.com> > >> > >> This simplifies running code on shutdown for USB drivers. > >> > > > > Sorry, but this does not explain why this is needed at all :( > > > > Where did this change come from? What problem does it solve? Why > > should we take it? > > > > Currently there is no standardized method for USB drivers to handle > shutdown events. This patch simplifies running code on shutdown for USB > devices by adding a shutdown callback to usb_driver. It also implements the > new method to existing "USB Attached SCSI” driver that required shutdown > > Is this good enough? It's a good start, yes. But as you say "also" that means you should split this up into at least 2 changes, right? > > I think I know the answers to these questions, but you need to document > > it here as to why it is needed (please read the kernel documentation for > > how to write a good changelog text and subject line.) > > The subject looks fine to me. If you think it can be improved, suggestions shall be appreciated. When you split the patch up, the subject lines will get better. thanks, greg k-h
> On 6 Jul 2024, at 2:32 PM, gregkh@linuxfoundation.org wrote: > > On Sat, Jul 06, 2024 at 08:56:05AM +0000, Aditya Garg wrote: >> >> Hi Greg >> >>>> On 6 Jul 2024, at 1:43 PM, gregkh@linuxfoundation.org wrote: >>> >>> On Fri, Jul 05, 2024 at 11:21:06AM +0000, Aditya Garg wrote: >>>> From: Kerem Karabay <kekrby@gmail.com> >>>> >>>> This simplifies running code on shutdown for USB drivers. >>>> >>> >>> Sorry, but this does not explain why this is needed at all :( >>> >>> Where did this change come from? What problem does it solve? Why >>> should we take it? >>> >> >> Currently there is no standardized method for USB drivers to handle >> shutdown events. This patch simplifies running code on shutdown for USB >> devices by adding a shutdown callback to usb_driver. It also implements the >> new method to existing "USB Attached SCSI” driver that required shutdown >> >> Is this good enough? > > It's a good start, yes. > > But as you say "also" that means you should split this up into at least > 2 changes, right? Patch 1 with changes to hid core Patch 2 implementing the change to uas Right? > >>> I think I know the answers to these questions, but you need to document >>> it here as to why it is needed (please read the kernel documentation for >>> how to write a good changelog text and subject line.) >> >> The subject looks fine to me. If you think it can be improved, suggestions shall be appreciated. > > When you split the patch up, the subject lines will get better. > > thanks, > > greg k-h
> But as you say "also" that means you should split this up into at least > 2 changes, right? Sent a v2: https://lore.kernel.org/all/A6C4519F-852E-4B5C-B791-7396B515B8A6@live.com/T/#t
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c index f58a0299f..dc0f86376 100644 --- a/drivers/usb/core/driver.c +++ b/drivers/usb/core/driver.c @@ -514,6 +514,19 @@ static int usb_unbind_interface(struct device *dev) return 0; } +static void usb_shutdown_interface(struct device *dev) +{ + struct usb_interface *intf = to_usb_interface(dev); + struct usb_driver *driver; + + if (!dev->driver) + return; + + driver = to_usb_driver(dev->driver); + if (driver->shutdown) + driver->shutdown(intf); +} + /** * usb_driver_claim_interface - bind a driver to an interface * @driver: the driver to be bound @@ -1053,6 +1066,7 @@ int usb_register_driver(struct usb_driver *new_driver, struct module *owner, new_driver->driver.bus = &usb_bus_type; new_driver->driver.probe = usb_probe_interface; new_driver->driver.remove = usb_unbind_interface; + new_driver->driver.shutdown = usb_shutdown_interface; new_driver->driver.owner = owner; new_driver->driver.mod_name = mod_name; new_driver->driver.dev_groups = new_driver->dev_groups; diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 2583ee981..591fa0379 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -1221,9 +1221,8 @@ static void uas_disconnect(struct usb_interface *intf) * hang on reboot when the device is still in uas mode. Note the reset is * necessary as some devices won't revert to usb-storage mode without it. */ -static void uas_shutdown(struct device *dev) +static void uas_shutdown(struct usb_interface *intf) { - struct usb_interface *intf = to_usb_interface(dev); struct usb_device *udev = interface_to_usbdev(intf); struct Scsi_Host *shost = usb_get_intfdata(intf); struct uas_dev_info *devinfo = (struct uas_dev_info *)shost->hostdata; @@ -1246,7 +1245,7 @@ static struct usb_driver uas_driver = { .suspend = uas_suspend, .resume = uas_resume, .reset_resume = uas_reset_resume, - .driver.shutdown = uas_shutdown, + .shutdown = uas_shutdown, .id_table = uas_usb_ids, }; diff --git a/include/linux/usb.h b/include/linux/usb.h index 25f8e62a3..5f3ae2186 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -1194,6 +1194,7 @@ struct usbdrv_wrap { * post_reset method is called. * @post_reset: Called by usb_reset_device() after the device * has been reset + * @shutdown: Called at shut-down time to quiesce the device. * @id_table: USB drivers use ID table to support hotplugging. * Export this with MODULE_DEVICE_TABLE(usb,...). This must be set * or your driver's probe function will never get called. @@ -1245,6 +1246,8 @@ struct usb_driver { int (*pre_reset)(struct usb_interface *intf); int (*post_reset)(struct usb_interface *intf); + void (*shutdown)(struct usb_interface *intf); + const struct usb_device_id *id_table; const struct attribute_group **dev_groups;