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

[Guan-Yu Lin]

On Thu, Oct 10, 2024 at 2:33 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, Oct 10, 2024 at 01:30:00PM +0800, Guan-Yu Lin wrote:
> > 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:
> > > > +             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.
>
> Why would a hub care?
>

Without the information of sideband activities on the usb devices
connected to the hub, the hub couldn't determine if it could suspend
or not.

> > 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.
>
> But that's exactly what is needed here, if a hub wants to know what is
> happening on a child device, it should just walk the list of children
> and look :)
>
> > So we combine two counters in rpm as sb_usage_count,
>
> Combining counters is almost always never a good idea and will come back
> to bite you in the end.  Memory isn't an issue here, speed isn't an
> issue here, so why not just do it properly?
>

By combining the two comments above, my understanding is that we should either:
1. separating the counter to one recording the sideband activity of
itself, one for its children.
2. walk the list of children to check sideband activities on demand.
Please correct me if I mistake your messages.

> > 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.
>
> I can understand that for the device being "controlled" by a sideband,
> but that's it.
>
> > When sideband activity changes on a usb device, its usb device parents
> > should all get notified to maintain the correctness of sb_usage_count.
>
> Why "should" they?  They shouldn't really care.
>

Hubs need the sideband activity information on downstream usb devices,
so that the hub won't suspend the upstream usb port when there is a
sideband accessing the usb device connected to it.

> > This notifying process creates the procedure to walk up the device
> > chain.
>
> You aren't notifying anyone, you are just incrementing a count that can
> change at any moment in time.
>

Apologies for the misleading word selection, by notify I mean changing
the counter.

> > > > +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?
>
> I don't know, as I don't know exactly what you are going to do with this
> information.  But as-is, you can't just go "let's put an atomic variable
> in there to make it race free" as that's just not going to work.
>

Agree that changing the variable to atomic wouldn't resolve race
conditions. Let me identify and mark critical sections in the next
patchset.

> > > > @@ -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?
>
> But the driver that is "sideband" is bound to the interface, not the
> device, right?  Or is it the device?
>
> And again, nothing is being "broadcasted" here, it's just a variable to
> be read at random times and can change randomly :)
>
> thanks,
>
> greg k-h

By looking at the comment in xhci-sideband.c, I think the sideband is
bound to an usb device instead of an usb interface. Maybe Mathias or
Wesley could provide more details on this.
/**
 * xhci_sideband_register - register a sideband for a usb device
 * @udev: usb device to be accessed via sideband
...
 */

Regards,
Guan-Yu