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