Re: [PATCH v5 09/12] xhci: introduce xhci->lost_power flag

["Théo Lebrun" <theo.lebrun@bootlin.com>]

On Mon Aug 5, 2024 at 3:41 PM CEST, Roger Quadros wrote:
> On 26/07/2024 21:17, Théo Lebrun wrote:
> > The XHCI_RESET_ON_RESUME quirk allows wrappers to signal that they
> > expect a reset after resume. It is also used by some to enforce a XHCI
> > reset on resume (see needs-reset-on-resume DT prop).
> > 
> > Some wrappers are unsure beforehands if they will reset. Add a mechanism
> > to signal *at resume* if power has been lost. Parent devices can set
> > this flag, that defaults to the XHCI_RESET_ON_RESUME value.
> > 
> > The XHCI_RESET_ON_RESUME quirk still triggers a runtime_pm_get() on the
> > controller. This is required as we do not know if a suspend will
> > trigger a reset, so the best guess is to avoid runtime PM.
> > 
> > Reset the xhci->lost_power value each time in xhci_resume(), making it
> > safe for devices to only set lost_power on some resumes.
> > 
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > ---
> >  drivers/usb/host/xhci.c | 8 +++++++-
> >  drivers/usb/host/xhci.h | 6 ++++++
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index 0a8cf6c17f82..2c9b32d339f9 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -1029,9 +1029,12 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
> >  
> >  	spin_lock_irq(&xhci->lock);
> >  
> > -	if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken_suspend)
> > +	if (hibernated || xhci->lost_power || xhci->broken_suspend)
>
> Why not treat xhci->lost_power and xhci->quriks & XHCI_RESET_ON_RESUME independently?
>
> XHCI_RESET_ON_RESUME is sued by devices that know they always need to be reset on resume.
>
> xhci->lost_power is used by devices that don't have consistent behavior.

The goal is to avoid almost-duplicate functionality. I feel like:

    XHCI_RESET_ON_RESUME is the default value of xhci->lost_power,
    which might be modified at resume.

Is a more straight forward solution than:

    Both XHCI_RESET_ON_RESUME and xhci->lost_power define if power was
    lost at resume. First must be statically known, second can be
    updated during runtime. If second is used, first one must NOT be
    set.

Indeed, the first solution brings two additional lines of code as you
commented below. I'd argue the easier-to-wrap-your-head-around logic is
more important.

Tell me if you are convinced the second approach is better.

>
>
> >  		reinit_xhc = true;
> >  
> > +	/* Reset to default value, parent devices might correct it at next resume. */
> > +	xhci->lost_power = !!(xhci->quirks & XHCI_RESET_ON_RESUME);
> > +
>
> then you don't need to do this.

To be honest, I added this line out of rigor. We could remove it and say
that any device that modifies xhci->lost_power once at resume must set
it at each later resume.

The above line felt like a small safety net to avoid logic mistakes.

>
> >  	if (!reinit_xhc) {
> >  		/*
> >  		 * Some controllers might lose power during suspend, so wait
> > @@ -5228,6 +5231,9 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
> >  	if (get_quirks)
> >  		get_quirks(dev, xhci);
> >  
> > +	/* Default value, that can be corrected at resume. */
> > +	xhci->lost_power = !!(xhci->quirks & XHCI_RESET_ON_RESUME);
> > + 
>
> nor this.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com