| Message ID | 20230301122310.3579-5-hadess@hadess.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/6] HID: logitech-hidpp: Simplify array length check | expand |
On Wed, Mar 01, 2023 at 01:23:09PM +0100, Bastien Nocera wrote: > This adds the API that allows device specific drivers to tell user-space > about whether the wireless device is connected to its receiver dongle. > > See "USB: core: Add wireless_status sysfs attribute" for a detailed > explanation of what this attribute should be used for. > > Signed-off-by: Bastien Nocera <hadess@hadess.net> > --- > Fixed locking/use-after-free in v2, thanks to Alan Stern > > drivers/usb/core/message.c | 40 ++++++++++++++++++++++++++++++++++++++ > include/linux/usb.h | 5 +++++ > 2 files changed, 45 insertions(+) > > diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c > index 127fac1af676..3867d9a85145 100644 > --- a/drivers/usb/core/message.c > +++ b/drivers/usb/core/message.c > @@ -1908,6 +1908,45 @@ static void __usb_queue_reset_device(struct work_struct *ws) > usb_put_intf(iface); /* Undo _get_ in usb_queue_reset_device() */ > } > > +/* > + * Internal function to set the wireless_status sysfs attribute > + * See usb_set_wireless_status() for more details > + */ > +static void __usb_wireless_status_intf(struct work_struct *ws) > +{ > + struct usb_interface *iface = > + container_of(ws, struct usb_interface, wireless_status_work); > + > + device_lock(iface->dev.parent); > + if (iface->sysfs_files_created) > + usb_update_wireless_status_attr(iface); > + usb_put_intf(iface); /* Undo _get_ in usb_set_wireless_status() */ > + device_unlock(iface->dev.parent); Whoops! Calling usb_put_intf() means the iface pointer is no longer valid. The device_unlock() call should come before it, not after. Alan PS: You might also want to edit the sysfs documentation in the preceding patch, to make sure the text doesn't extend beyond the 80-column limit. I know people don't pay too much attention to that restriction in code any more, but in documentation it helps to keep the lines fairly short. People have trouble reading text when the lines get too long.
On Wed, 2023-03-01 at 10:33 -0500, Alan Stern wrote: > On Wed, Mar 01, 2023 at 01:23:09PM +0100, Bastien Nocera wrote: > > This adds the API that allows device specific drivers to tell user- > > space > > about whether the wireless device is connected to its receiver > > dongle. > > > > See "USB: core: Add wireless_status sysfs attribute" for a detailed > > explanation of what this attribute should be used for. > > > > Signed-off-by: Bastien Nocera <hadess@hadess.net> > > --- > > Fixed locking/use-after-free in v2, thanks to Alan Stern > > > > drivers/usb/core/message.c | 40 > > ++++++++++++++++++++++++++++++++++++++ > > include/linux/usb.h | 5 +++++ > > 2 files changed, 45 insertions(+) > > > > diff --git a/drivers/usb/core/message.c > > b/drivers/usb/core/message.c > > index 127fac1af676..3867d9a85145 100644 > > --- a/drivers/usb/core/message.c > > +++ b/drivers/usb/core/message.c > > @@ -1908,6 +1908,45 @@ static void __usb_queue_reset_device(struct > > work_struct *ws) > > usb_put_intf(iface); /* Undo _get_ in > > usb_queue_reset_device() */ > > } > > > > +/* > > + * Internal function to set the wireless_status sysfs attribute > > + * See usb_set_wireless_status() for more details > > + */ > > +static void __usb_wireless_status_intf(struct work_struct *ws) > > +{ > > + struct usb_interface *iface = > > + container_of(ws, struct usb_interface, > > wireless_status_work); > > + > > + device_lock(iface->dev.parent); > > + if (iface->sysfs_files_created) > > + usb_update_wireless_status_attr(iface); > > + usb_put_intf(iface); /* Undo _get_ in > > usb_set_wireless_status() */ > > + device_unlock(iface->dev.parent); > > Whoops! Calling usb_put_intf() means the iface pointer is no longer > valid. The device_unlock() call should come before it, not after. Fixed this in v3. > PS: You might also want to edit the sysfs documentation in the > preceding > patch, to make sure the text doesn't extend beyond the 80-column > limit. > I know people don't pay too much attention to that restriction in > code > any more, but in documentation it helps to keep the lines fairly > short. > People have trouble reading text when the lines get too long. That's an easy fix, so done in v3 as well.
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c index 127fac1af676..3867d9a85145 100644 --- a/drivers/usb/core/message.c +++ b/drivers/usb/core/message.c @@ -1908,6 +1908,45 @@ static void __usb_queue_reset_device(struct work_struct *ws) usb_put_intf(iface); /* Undo _get_ in usb_queue_reset_device() */ } +/* + * Internal function to set the wireless_status sysfs attribute + * See usb_set_wireless_status() for more details + */ +static void __usb_wireless_status_intf(struct work_struct *ws) +{ + struct usb_interface *iface = + container_of(ws, struct usb_interface, wireless_status_work); + + device_lock(iface->dev.parent); + if (iface->sysfs_files_created) + usb_update_wireless_status_attr(iface); + usb_put_intf(iface); /* Undo _get_ in usb_set_wireless_status() */ + device_unlock(iface->dev.parent); +} + +/** + * usb_set_wireless_status - sets the wireless_status struct member + * @dev: the device to modify + * @status: the new wireless status + * + * Set the wireless_status struct member to the new value, and emit + * sysfs changes as necessary. + * + * Returns: 0 on success, -EALREADY if already set. + */ +int usb_set_wireless_status(struct usb_interface *iface, + enum usb_wireless_status status) +{ + if (iface->wireless_status == status) + return -EALREADY; + + usb_get_intf(iface); + iface->wireless_status = status; + schedule_work(&iface->wireless_status_work); + + return 0; +} +EXPORT_SYMBOL_GPL(usb_set_wireless_status); /* * usb_set_configuration - Makes a particular device setting be current @@ -2100,6 +2139,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration) intf->dev.type = &usb_if_device_type; intf->dev.groups = usb_interface_groups; INIT_WORK(&intf->reset_ws, __usb_queue_reset_device); + INIT_WORK(&intf->wireless_status_work, __usb_wireless_status_intf); intf->minor = -1; device_initialize(&intf->dev); pm_runtime_no_callbacks(&intf->dev); diff --git a/include/linux/usb.h b/include/linux/usb.h index 46fc85aba0df..a48eeec62a66 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -262,6 +262,7 @@ struct usb_interface { unsigned resetting_device:1; /* true: bandwidth alloc after reset */ unsigned authorized:1; /* used for interface authorization */ enum usb_wireless_status wireless_status; + struct work_struct wireless_status_work; struct device dev; /* interface specific device info */ struct device *usb_dev; @@ -896,6 +897,10 @@ static inline int usb_interface_claimed(struct usb_interface *iface) extern void usb_driver_release_interface(struct usb_driver *driver, struct usb_interface *iface); + +int usb_set_wireless_status(struct usb_interface *iface, + enum usb_wireless_status status); + const struct usb_device_id *usb_match_id(struct usb_interface *interface, const struct usb_device_id *id); extern int usb_match_one_id(struct usb_interface *interface,
This adds the API that allows device specific drivers to tell user-space about whether the wireless device is connected to its receiver dongle. See "USB: core: Add wireless_status sysfs attribute" for a detailed explanation of what this attribute should be used for. Signed-off-by: Bastien Nocera <hadess@hadess.net> --- Fixed locking/use-after-free in v2, thanks to Alan Stern drivers/usb/core/message.c | 40 ++++++++++++++++++++++++++++++++++++++ include/linux/usb.h | 5 +++++ 2 files changed, 45 insertions(+)