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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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.

thanks,

greg k-h