Re: [PATCH v2 2/4] PCI: Move PCIe ports to D3 during suspend

[Mika Westerberg <mika.westerberg@linux.intel.com>]

On Fri, Apr 08, 2016 at 08:07:23AM -0700, Greg Kroah-Hartman wrote:
> On Fri, Apr 08, 2016 at 01:36:28PM +0300, Mika Westerberg wrote:
> > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> > index f0640b7a1c42..aafc1b093bc8 100644
> > --- a/drivers/usb/host/xhci-pci.c
> > +++ b/drivers/usb/host/xhci-pci.c
> > @@ -379,7 +379,7 @@ static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
> >  	 * need to have the registers polled during D3, so avoid D3cold.
> >  	 */
> >  	if (xhci->quirks & XHCI_COMP_MODE_QUIRK)
> > -		pdev->no_d3cold = true;
> > +		pci_enable_d3cold(pdev, false);
> >  
> >  	if (xhci->quirks & XHCI_PME_STUCK_QUIRK)
> >  		xhci_pme_quirk(hcd);
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 004b8133417d..d8801587b4ca 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -296,6 +296,7 @@ struct pci_dev {
> >  	unsigned int	d2_support:1;	/* Low power state D2 is supported */
> >  	unsigned int	no_d1d2:1;	/* D1 and D2 are forbidden */
> >  	unsigned int	no_d3cold:1;	/* D3cold is forbidden */
> > +	unsigned int	bridge_d3:1;	/* Allow D3 for bridge */
> >  	unsigned int	d3cold_allowed:1;	/* D3cold is allowed by user */
> >  	unsigned int	mmio_always_on:1;	/* disallow turning off io/mem
> >  						   decoding during bar sizing */
> > @@ -1085,6 +1086,7 @@ int pci_back_from_sleep(struct pci_dev *dev);
> >  bool pci_dev_run_wake(struct pci_dev *dev);
> >  bool pci_check_pme_status(struct pci_dev *dev);
> >  void pci_pme_wakeup_bus(struct pci_bus *bus);
> > +void pci_enable_d3cold(struct pci_dev *dev, bool enable);
> 
> That's an ackward api, as is seen in the above use in the xhci driver
> (enable false?)
> 
> Why not just make 2 functions:
> 	pci_dc3cold_enable(struct pci_dev *dev)
> 	pci_dc3cold_disable(struct pci_dev *dev)
> which makes it obvious what is going on.
> 
> Whenever you add a bool to a function, it requires the developer to go
> look up the implementation to see what it means and how to use it, to
> check if the usage is correct.  Just make it obvious in the name itself
> so everyone knows exactly what is going on.
> 
> Same thing goes for pci_bridge_pm_update(), even with the documentation
> I'm not quite sure what the bool flag is for.  Please split this into
> two calls as well.

Will, do. Thanks for the excellent explanation :)