Re: [PATCH 4/5] USB: core: Add API to change the wireless_status

[Bastien Nocera <hadess@hadess.net>]

On Thu, 2023-02-23 at 10:41 -0500, Alan Stern wrote:
> On Thu, Feb 23, 2023 at 02:24:51PM +0100, Bastien Nocera wrote:
> > Allow device specific drivers to change the wireless status of a
> > device.
> > This will allow user-space to know whether the device is available,
> > whether or not specific USB interfaces can detect it.
> > 
> > This can be used by wireless headsets with USB receivers to
> > propagate to
> > user-space whether or not the headset is turned on, so as to
> > consider it
> > as unavailable, and not switch to it just because the receiver is
> > plugged in.
> > 
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > ---
> >  drivers/usb/core/message.c | 13 +++++++++++++
> >  drivers/usb/core/usb.c     | 24 ++++++++++++++++++++++++
> >  include/linux/usb.h        |  4 ++++
> >  3 files changed, 41 insertions(+)
> > 
> > diff --git a/drivers/usb/core/message.c
> > b/drivers/usb/core/message.c
> > index 127fac1af676..d5c7749d515e 100644
> > --- a/drivers/usb/core/message.c
> > +++ b/drivers/usb/core/message.c
> > @@ -1908,6 +1908,18 @@ 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);
> > +
> > +       usb_update_wireless_status_attr(iface);
> > +       usb_put_intf(iface);    /* Undo _get_ in
> > usb_set_wireless_status() */
> > +}
> 
> Have you thought about what will happen if this routine ends up
> running 
> after the interface has been deleted?

I believe that usb_release_interface() will only be called once the
last reference to the interface is dropped, so bar any refcounting
bugs, the interface should always exist when this function is called.

> >  /*
> >   * usb_set_configuration - Makes a particular device setting be
> > current
> > @@ -2100,6 +2112,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/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > index 11b15d7b357a..5f42c5b9d209 100644
> > --- a/drivers/usb/core/usb.c
> > +++ b/drivers/usb/core/usb.c
> > @@ -871,6 +871,30 @@ int usb_get_current_frame_number(struct
> > usb_device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(usb_get_current_frame_number);
> >  
> > +/**
> > + * 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);
> 
> This routine belongs in message.c, next to
> __usb_wireless_status_intf().

Sure, done locally.