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 20:45:53 +0530
From:   Naveen Naidu <naveennaidu479@gmail.com>
To:     Rob Herring <robh@kernel.org>
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: <20211020151553.4pu7ib6uo6pk6sr4@theprophet>
References: <cover.1634306198.git.naveennaidu479@gmail.com>
 <da939a6cdb2dea96d16392ae152e1232212877d1.1634306198.git.naveennaidu479@gmail.com>
 <YXAV4bGehjAKK8XO@robh.at.kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YXAV4bGehjAKK8XO@robh.at.kernel.org>
Precedence: bulk

On 20/10, Rob Herring wrote:
> 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.
> 

Thank you for the review. I'll fix this up when I send v3 for this patch
series.

> >  			*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
> > 
> >