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

[Roger Quadros <rogerq@kernel.org>]

Hello Théo,

On 10/09/2024 16:50, Théo Lebrun wrote:
> 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.
> 

I would still vote to keep logic tied to separate flags.

so XHCI_RESET_ON_RESUME to always resume on RESET
xhci->lost_power, reset based on runtime checks.

Which implies that platforms using xhci->lost_power should not
set XHCI_RESET_ON_RESUME.

But XHCI maintainers should give their opinion on this.

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

-- 
cheers,
-roger