Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 20 Oct 2021 08:13:05 -0500
From:   Rob Herring <robh@kernel.org>
To:     Naveen Naidu <naveennaidu479@gmail.com>
Cc:     bhelgaas@google.com,
        linux-kernel-mentees@lists.linuxfoundation.org,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 03/24] PCI: Unify PCI error response checking
Message-ID: <YXAV4bGehjAKK8XO@robh.at.kernel.org>
References: <cover.1634306198.git.naveennaidu479@gmail.com>
 <da939a6cdb2dea96d16392ae152e1232212877d1.1634306198.git.naveennaidu479@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <da939a6cdb2dea96d16392ae152e1232212877d1.1634306198.git.naveennaidu479@gmail.com>
Precedence: bulk

On Fri, Oct 15, 2021 at 08:08:44PM +0530, Naveen Naidu wrote:
> An MMIO read from a PCI device that doesn't exist or doesn't respond
> causes a PCI error.  There's no real data to return to satisfy the
> CPU read, so most hardware fabricates ~0 data.
> 
> Use SET_PCI_ERROR_RESPONSE() to set the error response and
> RESPONSE_IS_PCI_ERROR() to check the error response during hardware
> read.
> 
> These definitions make error checks consistent and easier to find.
> 
> Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com>
> ---
>  drivers/pci/access.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index b3b2006ed1d2..03712866c818 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -417,10 +417,10 @@ int pcie_capability_read_word(struct pci_dev *dev, int pos, u16 *val)
>  		ret = pci_read_config_word(dev, pci_pcie_cap(dev) + pos, val);
>  		/*
>  		 * Reset *val to 0 if pci_read_config_word() fails, it may
> -		 * have been written as 0xFFFF if hardware error happens
> -		 * during pci_read_config_word().
> +		 * have been written as 0xFFFF (PCI_ERROR_RESPONSE) if hardware error
> +		 * happens during pci_read_config_word().
>  		 */
> -		if (ret)
> +		if (RESPONSE_IS_PCI_ERROR(val))

What if there is no error (in ret) and the register value was actually 
~0? We'd be corrupting the value.

In general, I think we should rely more on the error codes and less on 
the ~0 value.

>  			*val = 0;
>  		return ret;
>  	}
> @@ -452,10 +452,10 @@ int pcie_capability_read_dword(struct pci_dev *dev, int pos, u32 *val)
>  		ret = pci_read_config_dword(dev, pci_pcie_cap(dev) + pos, val);
>  		/*
>  		 * Reset *val to 0 if pci_read_config_dword() fails, it may
> -		 * have been written as 0xFFFFFFFF if hardware error happens
> -		 * during pci_read_config_dword().
> +		 * have been written as 0xFFFFFFFF (PCI_ERROR_RESPONSE) if hardware
> +		 * error happens during pci_read_config_dword().
>  		 */
> -		if (ret)
> +		if (RESPONSE_IS_PCI_ERROR(val))
>  			*val = 0;
>  		return ret;
>  	}
> @@ -529,7 +529,7 @@ EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
>  int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
>  {
>  	if (pci_dev_is_disconnected(dev)) {
> -		*val = ~0;
> +		SET_PCI_ERROR_RESPONSE(val);
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  	}
>  	return pci_bus_read_config_byte(dev->bus, dev->devfn, where, val);
> @@ -539,7 +539,7 @@ EXPORT_SYMBOL(pci_read_config_byte);
>  int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val)
>  {
>  	if (pci_dev_is_disconnected(dev)) {
> -		*val = ~0;
> +		SET_PCI_ERROR_RESPONSE(val);
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  	}
>  	return pci_bus_read_config_word(dev->bus, dev->devfn, where, val);
> @@ -550,7 +550,7 @@ int pci_read_config_dword(const struct pci_dev *dev, int where,
>  					u32 *val)
>  {
>  	if (pci_dev_is_disconnected(dev)) {
> -		*val = ~0;
> +		SET_PCI_ERROR_RESPONSE(val);
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  	}
>  	return pci_bus_read_config_dword(dev->bus, dev->devfn, where, val);
> -- 
> 2.25.1
> 
>