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

[Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>]

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:
> > > +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.

Why would a hub care?

> 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?

> 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.

> 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.

> > > +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.

> > > @@ -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