Re: [PATCH v3 08/11] PCI: Clean up ATS error handling

[Joerg Roedel <jroedel@suse.de>]

Hi Bjoern,

On Tue, Aug 11, 2015 at 10:52:02AM -0500, Bjorn Helgaas wrote:
> There's no need to BUG() if we enable ATS when it's already enabled.  We
> don't need to BUG() when disabling ATS on a device that doesn't support ATS
> or if it's already disabled.  If ATS is enabled, certainly we found an ATS
> capability in the past, so it should still be there now.
> 
> Clean up these error paths.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Two comments inline. With these changes:

Reviewed-by: Joerg Roedel <jroedel@suse.de>

> ---
>  drivers/pci/ats.c |   11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index 0b5b0ed..0f05274 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -44,11 +44,13 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
>  	u16 ctrl;
>  	struct pci_dev *pdev;
>  
> -	BUG_ON(dev->ats_cap && dev->ats_enabled);
> -
>  	if (!dev->ats_cap)
>  		return -EINVAL;
>  
> +	WARN_ON(pci_ats_enabled(dev));
> +	if (pci_ats_enabled(dev))
> +		return -EBUSY;
> +

Could be written as

	if (WARN_ON(pci_ats_enabled(dev)))
		return -EBUSY;

>  	if (ps < PCI_ATS_MIN_STU)
>  		return -EINVAL;
>  
> @@ -83,7 +85,8 @@ void pci_disable_ats(struct pci_dev *dev)
>  	struct pci_dev *pdev;
>  	u16 ctrl;
>  
> -	BUG_ON(!dev->ats_cap || !dev->ats_enabled);
> +	if (!pci_ats_enabled(dev))
> +		return;

Probably also:

	if (WARN_ON(!pci_ats_enabled(dev)))

?