Re: [PATCH v8 4/4] USB: Pass PMSG_POWEROFF event to suspend_common()

Mario Limonciello (AMD) (kernel.org)
2025-09-18T21:01:25Z

On 9/18/2025 2:55 PM, Bjorn Helgaas wrote:
> On Wed, Sep 17, 2025 at 10:44:27PM -0500, Mario Limonciello (AMD) wrote:
>> suspend_common() passes the a PM message indicating what type of event
>> is occurring. Add a new callback hcd_pci_poweroff() which will
>> determine if target state is power off and pass PMSG_POWEROFF as the
>> message.
>
> Something is missing in "passes the a".

A wild 'a' appeared.
>
>> There are no functional changes in this patch.
>
> Maybe so, but the .poweroff() path previously called
> "suspend_common(dev, PMSG_SUSPEND)" and now may call
> "suspend_common(dev, PMSG_POWEROFF)", so it's not completely obvious
> that this is a functional no-op.
>
> It seems sort of weird that apparently we can call .poweroff() for
> either a "suspend" or a "power-off" event.
>

It's actually either a hibernate or a power off event. The nomenclature
can be a bit confusing.

I'll add a comment to the top of the function to explain it.

> Maybe it would be helpful to explain how we get to .poweroff() when
> system_state is something other than SYSTEM_POWER_OFF, and what that
> means?

Well right now it's not a possible path, but per Rafael's guidance of
splitting the series into 3 parts across 3 cycles the path that leads
here won't exist for a while. It was just plumbing.

I'll adjust the commit message to allude to the future.

>
>> +static int hcd_pci_poweroff(struct device *dev)
>> +{
>> + if (system_state == SYSTEM_POWER_OFF)
>> + return suspend_common(dev, PMSG_POWEROFF);
>> + return suspend_common(dev, PMSG_SUSPEND);
>> +}
>> +
>> static int hcd_pci_suspend_noirq(struct device *dev)
>> {
>> struct pci_dev *pci_dev = to_pci_dev(dev);
>> @@ -602,6 +610,7 @@ static int hcd_pci_restore(struct device *dev)
>> #define hcd_pci_suspend NULL
>> #define hcd_pci_freeze NULL
>> #define hcd_pci_suspend_noirq NULL
>> +#define hcd_pci_poweroff NULL
>> #define hcd_pci_poweroff_late NULL
>> #define hcd_pci_resume_noirq NULL
>> #define hcd_pci_resume NULL
>> @@ -639,7 +648,7 @@ const struct dev_pm_ops usb_hcd_pci_pm_ops = {
>> .freeze_noirq = check_root_hub_suspended,
>> .thaw_noirq = NULL,
>> .thaw = hcd_pci_resume,
>> - .poweroff = hcd_pci_suspend,
>> + .poweroff = hcd_pci_poweroff,
>> .poweroff_late = hcd_pci_poweroff_late,
>> .poweroff_noirq = hcd_pci_suspend_noirq,
>> .restore_noirq = hcd_pci_resume_noirq,
>> --
>> 2.51.0
>>