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

["Mario Limonciello (AMD) (kernel.org)" <superm1@kernel.org>]

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