Hello Mathias,
On Wed Jan 8, 2025 at 7:43 PM CET, Mathias Nyman wrote:
> This would be a fourth way the upper layers inform xhci_resume() that the
> xHC host should be reset instead of restoring the registers.
>
> option 1 creates the first dynamic xhci->quirk flag,
> option 2 adds a xhci->lost_power flag that is touched outside of xhci driver.
>
> Neither seem like a good idea just to get rid of a dev_warn() message.
>
> Maybe its time to split xhci_resume() into xhci_reset_resume()
> and xhci_restore_resume(), and let those upper layers decide what they need.
>
> Doesn't cdns driver already have a xhci_plat_priv resume_quirk() function
> called during xhci_plat_resume(), before xhci_resume()?
> Can that be used?
I agree with your feeling: another solution is needed. Discussing the
topic gave some new ideas and I have a new solution that feels much
more appropriate.
### Opinion on splitting xhci_resume() into two implementations
About splitting xhci_resume() into two different implementations that is
picked by above layer: I am not convinced.
What would go into xhci_reset_resume() and xhci_restore_resume()? There
isn't a clear cut inbetween the reset procedure and the normal restore
procedure, as we might do a reset if the normal restore procedure
fails (with some logging that reset was unexpected).
We would probably end up with many small functions called by either,
which would blur the overall step sequence.
### Proposal
Instead, I propose we keep xhci_resume() as a single function but change
its signature from the current:
int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg);
To this:
int xhci_resume(struct xhci_hcd *xhci, bool power_lost,
bool is_auto_resume);
The key insight is that xhci_resume() extracts two information out of
the message:
- whether we are after hibernation: msg.event == PM_EVENT_RESTORE,
- whether this is an auto resume: msg.event == PM_EVENT_AUTO_RESUME.
First bulletpoint is somewhat wrong: driver wants to know if the device
did lose power, it doesn't care about hibernation per se. It just
happened to be that hibernation was the only case of power loss.
Knowing that, we can refactor to ask upper layers the right questions:
(1) "did we lose power?" and, (2) "is this an auto resume?".
Then, each caller is responsible for handing those booleans. If they
themselves receive pm_message_t messages (eg xhci-pci), then they do
the simple conversion:
bool power_lost = msg.event == PM_EVENT_RESTORE;
bool is_auto_resume = msg.event == PM_EVENT_AUTO_RESUME;
Others can do more more or less computation to pick a power_lost value.
### About xhci-plat power_lost value
In the case of xhci-plat, I think it will be:
- A new power_lost field in `struct xhci_plat_priv`.
- That gets set inside the cdns_role_driver::resume() callback of
drivers/usb/cdns3/host.c.
- Then xhci_plat_resume_common() computes power_lost being:
power_lost = is_restore || priv->power_lost;
### About xhci_plat_priv::resume_quirk()
It isn't really useful to use. drivers/usb/cdns3/host.c can know whether
power was lost through its USB role resume() callback. From there to
the resume_quirk(), the boolean must be stored somewhere. That
somewhere can only be... inside xhci_plat_priv. So it is simpler if
xhci-plat retrieves the boolean directly.
xhci_plat_priv::resume_quirk() can change the power_lost value if it
wants to, that is fine. But in our case, the info isn't available from
there.
###
I am unsure if the above explanation is clear, so I'll be sending my
current work-in-progress series as an answer to this. The goal being
that you can give me your opinion on the proposal: ACK and we continue
in this direction, NACK and we keep digging.
I'll wait for the merge window to end before sending a proper revision.
Thanks!
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
