Re: [PATCH 1/2] USB: Extend pci resume function to handle PM events

[Basavaraj Natikar <bnatikar@amd.com>]

On 4/18/2023 8:36 PM, Alan Stern wrote:
> On Tue, Apr 18, 2023 at 07:38:16PM +0530, Basavaraj Natikar wrote:
>> Currently, the pci_resume method has only a flag indicating whether the
>> system is resuming from hibernation. In order to handle all PM events like
>> AUTO_RESUME, SUSPEND etc change the pci_resume method to handle all PM
>> events.
> You might want to make a different kind of distinction between the 
> various sorts of resume.  For example, a resume from runtime suspend 
> can occur either because of a request from the system (it needs to start 
> using the device) or a remote wakeup request from an attached device.  
> The different sorts of resume might have different requirements.

yes correct.

>
>> diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
>> index 4b148fe5e43b..1145c6e7fae5 100644
>> --- a/drivers/usb/host/ehci-pci.c
>> +++ b/drivers/usb/host/ehci-pci.c
>> @@ -354,10 +354,11 @@ static int ehci_pci_setup(struct usb_hcd *hcd)
>>   * Also they depend on separate root hub suspend/resume.
>>   */
>>  
>> -static int ehci_pci_resume(struct usb_hcd *hcd, bool hibernated)
>> +static int ehci_pci_resume(struct usb_hcd *hcd, int event)
>>  {
>>  	struct ehci_hcd		*ehci = hcd_to_ehci(hcd);
>>  	struct pci_dev		*pdev = to_pci_dev(hcd->self.controller);
>> +	bool hibernated = event == PM_EVENT_RESTORE;
> Please use the same indentation style as the surrounding code.  Also, 
> when a boolean expression is used in an assignment, I prefer to put it 
> in parentheses to help set it off from the assignment operator:
>
> 	bool			hibernated = (event == PM_EVENT_RESTORE);

Sure will change it accordingly.

>
>> diff --git a/drivers/usb/host/uhci-pci.c b/drivers/usb/host/uhci-pci.c
>> index 3592f757fe05..9b90c3221bd8 100644
>> --- a/drivers/usb/host/uhci-pci.c
>> +++ b/drivers/usb/host/uhci-pci.c
>> @@ -167,7 +167,7 @@ static void uhci_shutdown(struct pci_dev *pdev)
>>  
>>  #ifdef CONFIG_PM
>>  
>> -static int uhci_pci_resume(struct usb_hcd *hcd, bool hibernated);
>> +static int uhci_resume(struct usb_hcd *hcd, bool hibernated);
> There's no need to change the function's name.  After all, it is static.

sure will keep same function name.

>
>>  
>>  static int uhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
>>  {
>> @@ -202,13 +202,13 @@ static int uhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
>>  
>>  	/* Check for race with a wakeup request */
>>  	if (do_wakeup && HCD_WAKEUP_PENDING(hcd)) {
>> -		uhci_pci_resume(hcd, false);
>> +		uhci_resume(hcd, false);
>>  		rc = -EBUSY;
>>  	}
>>  	return rc;
>>  }
>>  
>> -static int uhci_pci_resume(struct usb_hcd *hcd, bool hibernated)
>> +static int uhci_resume(struct usb_hcd *hcd, bool hibernated)
>>  {
>>  	struct uhci_hcd *uhci = hcd_to_uhci(hcd);
>>  
>> @@ -252,6 +252,10 @@ static int uhci_pci_resume(struct usb_hcd *hcd, bool hibernated)
>>  	return 0;
>>  }
>>  
>> +static int uhci_pci_resume(struct usb_hcd *hcd, int event)
>> +{
>> +	return uhci_resume(hcd, event == PM_EVENT_RESTORE);
>> +}
> Again, try to avoid this wrapper.

Sure will avoid this change. 

Thanks,
--
Basavaraj

>
> Alan Stern