| Message ID | 20230418140817.3651909-2-Basavaraj.Natikar@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Handle PM events for pci resume | expand |
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. > 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); > 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. > > 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. Alan Stern
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. > > Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> > --- > drivers/usb/core/hcd-pci.c | 14 ++++++++------ > drivers/usb/host/ehci-pci.c | 3 ++- > drivers/usb/host/ohci-pci.c | 8 +++++++- > drivers/usb/host/uhci-pci.c | 10 +++++++--- > drivers/usb/host/xhci-pci.c | 4 ++-- > drivers/usb/host/xhci.c | 3 ++- > drivers/usb/host/xhci.h | 2 +- > include/linux/usb/hcd.h | 2 +- > 8 files changed, 30 insertions(+), 16 deletions(-) > > diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c > index ab2f3737764e..bef092da477a 100644 > --- a/drivers/usb/core/hcd-pci.c > +++ b/drivers/usb/core/hcd-pci.c > @@ -415,12 +415,15 @@ static int check_root_hub_suspended(struct device *dev) > return 0; > } > > -static int suspend_common(struct device *dev, bool do_wakeup) > +static int suspend_common(struct device *dev, int event) Shouldn't there be a PM_EVENT_* type for this so that we can properly type-check that it is being used properly everywhere? Much like we can do for GFP_* flags? Not the fault of this patch, just a general comment... thanks, greg k-h
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
On 4/18/2023 8:53 PM, Greg KH 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. >> >> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> >> --- >> drivers/usb/core/hcd-pci.c | 14 ++++++++------ >> drivers/usb/host/ehci-pci.c | 3 ++- >> drivers/usb/host/ohci-pci.c | 8 +++++++- >> drivers/usb/host/uhci-pci.c | 10 +++++++--- >> drivers/usb/host/xhci-pci.c | 4 ++-- >> drivers/usb/host/xhci.c | 3 ++- >> drivers/usb/host/xhci.h | 2 +- >> include/linux/usb/hcd.h | 2 +- >> 8 files changed, 30 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c >> index ab2f3737764e..bef092da477a 100644 >> --- a/drivers/usb/core/hcd-pci.c >> +++ b/drivers/usb/core/hcd-pci.c >> @@ -415,12 +415,15 @@ static int check_root_hub_suspended(struct device *dev) >> return 0; >> } >> >> -static int suspend_common(struct device *dev, bool do_wakeup) >> +static int suspend_common(struct device *dev, int event) > Shouldn't there be a PM_EVENT_* type for this so that we can properly > type-check that it is being used properly everywhere? Much like we can > do for GFP_* flags? yes correct , will change in all place accordingly by using pm_message_t type. Thanks, -- Basavaraj > Not the fault of this patch, just a general comment... > > thanks, > > greg k-h
diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c index ab2f3737764e..bef092da477a 100644 --- a/drivers/usb/core/hcd-pci.c +++ b/drivers/usb/core/hcd-pci.c @@ -415,12 +415,15 @@ static int check_root_hub_suspended(struct device *dev) return 0; } -static int suspend_common(struct device *dev, bool do_wakeup) +static int suspend_common(struct device *dev, int event) { struct pci_dev *pci_dev = to_pci_dev(dev); struct usb_hcd *hcd = pci_get_drvdata(pci_dev); + bool do_wakeup; int retval; + do_wakeup = event == PM_EVENT_AUTO_SUSPEND ? true : device_may_wakeup(dev); + /* Root hub suspend should have stopped all downstream traffic, * and all bus master traffic. And done so for both the interface * and the stub usb_device (which we check here). But maybe it @@ -447,7 +450,7 @@ static int suspend_common(struct device *dev, bool do_wakeup) (retval == 0 && do_wakeup && hcd->shared_hcd && HCD_WAKEUP_PENDING(hcd->shared_hcd))) { if (hcd->driver->pci_resume) - hcd->driver->pci_resume(hcd, false); + hcd->driver->pci_resume(hcd, event); retval = -EBUSY; } if (retval) @@ -502,8 +505,7 @@ static int resume_common(struct device *dev, int event) for_each_companion(pci_dev, hcd, ehci_wait_for_companions); - retval = hcd->driver->pci_resume(hcd, - event == PM_EVENT_RESTORE); + retval = hcd->driver->pci_resume(hcd, event); if (retval) { dev_err(dev, "PCI post-resume error %d!\n", retval); usb_hc_died(hcd); @@ -516,7 +518,7 @@ static int resume_common(struct device *dev, int event) static int hcd_pci_suspend(struct device *dev) { - return suspend_common(dev, device_may_wakeup(dev)); + return suspend_common(dev, PM_EVENT_SUSPEND); } static int hcd_pci_suspend_noirq(struct device *dev) @@ -600,7 +602,7 @@ static int hcd_pci_runtime_suspend(struct device *dev) { int retval; - retval = suspend_common(dev, true); + retval = suspend_common(dev, PM_EVENT_AUTO_SUSPEND); if (retval == 0) powermac_set_asic(to_pci_dev(dev), 0); dev_dbg(dev, "hcd_pci_runtime_suspend: %d\n", retval); 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; if (ehci_resume(hcd, hibernated) != 0) (void) ehci_pci_reinit(ehci, pdev); diff --git a/drivers/usb/host/ohci-pci.c b/drivers/usb/host/ohci-pci.c index d7b4f40f9ff4..923ed502414b 100644 --- a/drivers/usb/host/ohci-pci.c +++ b/drivers/usb/host/ohci-pci.c @@ -301,6 +301,12 @@ static struct pci_driver ohci_pci_driver = { #endif }; +#ifdef CONFIG_PM +static int ohci_pci_resume(struct usb_hcd *hcd, int event) +{ + return ohci_resume(hcd, event == PM_EVENT_RESTORE); +} +#endif static int __init ohci_pci_init(void) { if (usb_disabled()) @@ -311,7 +317,7 @@ static int __init ohci_pci_init(void) #ifdef CONFIG_PM /* Entries for the PCI suspend/resume callbacks are special */ ohci_pci_hc_driver.pci_suspend = ohci_suspend; - ohci_pci_hc_driver.pci_resume = ohci_resume; + ohci_pci_hc_driver.pci_resume = ohci_pci_resume; #endif return pci_register_driver(&ohci_pci_driver); 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); 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); +} #endif static const struct hc_driver uhci_driver = { diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c index 6db07ca419c3..ebdf9f32d128 100644 --- a/drivers/usb/host/xhci-pci.c +++ b/drivers/usb/host/xhci-pci.c @@ -628,7 +628,7 @@ static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup) return ret; } -static int xhci_pci_resume(struct usb_hcd *hcd, bool hibernated) +static int xhci_pci_resume(struct usb_hcd *hcd, int event) { struct xhci_hcd *xhci = hcd_to_xhci(hcd); struct pci_dev *pdev = to_pci_dev(hcd->self.controller); @@ -663,7 +663,7 @@ static int xhci_pci_resume(struct usb_hcd *hcd, bool hibernated) if (xhci->quirks & XHCI_PME_STUCK_QUIRK) xhci_pme_quirk(hcd); - retval = xhci_resume(xhci, hibernated); + retval = xhci_resume(xhci, event); return retval; } diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 6307bae9cddf..a539e4dd54f7 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -1157,8 +1157,9 @@ EXPORT_SYMBOL_GPL(xhci_suspend); * This is called when the machine transition from S3/S4 mode. * */ -int xhci_resume(struct xhci_hcd *xhci, bool hibernated) +int xhci_resume(struct xhci_hcd *xhci, int event) { + bool hibernated = event == PM_EVENT_RESTORE; u32 command, temp = 0; struct usb_hcd *hcd = xhci_to_hcd(xhci); int retval = 0; diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 786002bb35db..948fc2d7b1f0 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -2139,7 +2139,7 @@ int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id); int xhci_ext_cap_init(struct xhci_hcd *xhci); int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup); -int xhci_resume(struct xhci_hcd *xhci, bool hibernated); +int xhci_resume(struct xhci_hcd *xhci, int event); irqreturn_t xhci_irq(struct usb_hcd *hcd); irqreturn_t xhci_msi_irq(int irq, void *hcd); diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h index b51c07111729..337575dd8665 100644 --- a/include/linux/usb/hcd.h +++ b/include/linux/usb/hcd.h @@ -267,7 +267,7 @@ struct hc_driver { int (*pci_suspend)(struct usb_hcd *hcd, bool do_wakeup); /* called after entering D0 (etc), before resuming the hub */ - int (*pci_resume)(struct usb_hcd *hcd, bool hibernated); + int (*pci_resume)(struct usb_hcd *hcd, int event); /* called just before hibernate final D3 state, allows host to poweroff parts */ int (*pci_poweroff_late)(struct usb_hcd *hcd, bool do_wakeup);
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. Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> --- drivers/usb/core/hcd-pci.c | 14 ++++++++------ drivers/usb/host/ehci-pci.c | 3 ++- drivers/usb/host/ohci-pci.c | 8 +++++++- drivers/usb/host/uhci-pci.c | 10 +++++++--- drivers/usb/host/xhci-pci.c | 4 ++-- drivers/usb/host/xhci.c | 3 ++- drivers/usb/host/xhci.h | 2 +- include/linux/usb/hcd.h | 2 +- 8 files changed, 30 insertions(+), 16 deletions(-)