Message-ID: <CAOuDEK1B6cz58vcL=xx60C9Mdy57QqahRgFvRRLW-SHpuYDDHA@mail.gmail.com>
Date: Fri, 11 Oct 2024 15:33:00 +0800
From: Guan-Yu Lin <guanyulin@...gle.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: Thinh.Nguyen@...opsys.com, mathias.nyman@...el.com, 
	stern@...land.harvard.edu, elder@...nel.org, oneukum@...e.com, 
	yajun.deng@...ux.dev, dianders@...omium.org, kekrby@...il.com, perex@...ex.cz, 
	tiwai@...e.com, tj@...nel.org, stanley_chang@...ltek.com, 
	andreyknvl@...il.com, christophe.jaillet@...adoo.fr, 
	quic_jjohnson@...cinc.com, ricardo@...liere.net, grundler@...omium.org, 
	niko.mauno@...sala.com, linux-usb@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-sound@...r.kernel.org, badhri@...gle.com, 
	albertccwang@...gle.com, quic_wcheng@...cinc.com, pumahsu@...gle.com
Subject: Re: [PATCH v4 3/5] usb: add apis for sideband uasge tracking

On Fri, Oct 11, 2024 at 12:40 PM Greg KH <gregkh@...uxfoundation.org> wrote:
>
> On Fri, Oct 11, 2024 at 12:14:00AM +0800, Guan-Yu Lin wrote:
> > On Thu, Oct 10, 2024 at 2:33 PM Greg KH <gregkh@...uxfoundation.org> 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@...uxfoundation.org> 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.
>
> You are talking about an "internal" hub, right?  And isn't this already
> covered by the original sideband patchset?  If not, how is power
> management being handled there?
>

I'm referring to both internal and external hubs. As a sideband is
designed to handle the transfers on specific endpoints, I think
there's a possibility the usb device controlled by the sideband is
connected to the host controller by a hierarchy of usb hubs. The
current proposal of sideband, AFAIK, only supports sideband accessing
usb devices when the system is active, whereas now we're proposing
patchset to enable sideband access when the system is in sleep.

> > > > 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.
>
> I think 2 is better, as this is infrequent and should be pretty fast to
> do when needed, right?  But I really don't care, just don't combine
> things together that shouldn't be combined.
>

Thanks for the clarification, I'll renew the patchset based on the discussions.

> > > > 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.
>
> Then why doesn't the sideband code just properly mark the "downstream"
> device a being used like any other normal device?  Why is this acting
> "special"?
>
> thanks,
>
> greg k-h

In runtime power management, sidebands could mark a device active by
runtime pm apis as we usually did. However, there will be
ambiguity when we're doing device power management in system suspend.
A usb device being marked as runtime active could be either:
1) actively function through the existing usb driver stacks.
2) accessed by a sideband, where the usb driver stacks in the linux
system aren't involved.
In 1) system should suspend the devices, ports, controllers as normal
because usb transfers are also suspended during system suspend. On the
other hand, in 2) we should keep the necessary device, port,
controller active to support sideband access during system suspend.
Hence, we need the sideband access information of each usb_device
during system power state transition.

Regards,
Guan-Yu

Powered by blists - more mailing lists