Re: [PATCH v4 3/5] usb: add apis for sideband uasge tracking

[Guan-Yu Lin]

On Wed, Oct 9, 2024 at 8:44 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, Oct 09, 2024 at 05:42:57AM +0000, Guan-Yu Lin wrote:
> > +void usb_sideband_get(struct usb_device *udev)
> > +{
> > +     struct usb_device *parent = udev;
> > +
> > +     do {
> > +             atomic_inc(&parent->sb_usage_count);
>
> As this is a reference count, use refcount_t please.

Acknowledged, will change it in the next patch. Thanks for the guidance.

>
> > +             parent = parent->parent;
> > +     } while (parent);
>
> Woah, walking up the device chain?  That should not be needed, or if so,
> then each device's "usage count" is pointless.
>

Say a hub X with usb devices A,B,C attached on it, where usb device A
is actively used by sideband now. We'd like to introduce a mechanism
so that hub X won't have to iterate through all its children to
determine sideband activities under this usb device tree. This problem
is similar to runtime suspending a device, where rpm uses
power.usage_count for tracking activity of the device itself and
power.child_count to check the children's activity. In our scenario,
we don't see the need to separate activities on the device itself or
on its children. So we combine two counters in rpm as sb_usage_count,
denoting the sideband activities under a specific usb device. We have
to keep a counter in each device so that we won't influence the usb
devices that aren't controlled by a sideband.
When sideband activity changes on a usb device, its usb device parents
should all get notified to maintain the correctness of sb_usage_count.
This notifying process creates the procedure to walk up the device
chain.

> > +bool usb_sideband_check(struct usb_device *udev)
> > +{
> > +     return !!atomic_read(&udev->sb_usage_count);
>
> And what happens if it changes right after you make this call?  This
> feels racy and broken.
>

Seems like we need a mechanism to block any new sideband access after
the usb device has been suspended. How about adding a lock during the
period when the usb device is suspended? Do you think this is the
correct direction to address the race condition?

> > @@ -731,6 +732,8 @@ struct usb_device {
> >
> >       u16 hub_delay;
> >       unsigned use_generic_driver:1;
> > +
> > +     atomic_t sb_usage_count;
>
> Why is this on the device and not the interface that is bound to the
> driver that is doing this work?
>
> thanks,
>
> greg k-h

If the count is bound on the usb interface, I'm afraid that the
sideband information couldn't be broadcasted across its usb device
parents. Do you have some insight on how we can achieve this?

Regards,
Guan-Yu