Re: [PATCH 1/3] PCI: Add PCI_ERROR_RESPONSE definition

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Aug 23, 2019 at 11:44:15AM +0100, Andrew Murray wrote:
> On Thu, Aug 22, 2019 at 03:05:49PM -0500, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > 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.
> > 
> > Add a PCI_ERROR_RESPONSE definition for that and use it where appropriate
> > to make these checks consistent and easier to find.
> > 
> > Note that successful reads *also* may return ~0 data, so additional
> > information (e.g., knowledge that ~0 is not a valid register value) is
> > needed to reliably identify errors.

> > -	 * generally synthesize ~0 data to complete the read (except when
> > -	 * CRS SV is enabled and the read was for the Vendor ID; in that
> > -	 * case it synthesizes 0x0001 data).
> > +	 * generally synthesize ~0 data (PCI_ERROR_RESPONSE) to complete
> > +	 * the read (except when CRS SV is enabled and the read was for the
> > +	 * Vendor ID; in that case it synthesizes 0x0001 data).
> 
> There are some other areas in drivers/pci where comments also refer to ~0
> and similar:
> 
> $ git grep -i ffffffff drivers/pci/ | grep \*
> drivers/pci/access.c:            * have been written as 0xFFFFFFFF if hardware error happens
> drivers/pci/controller/dwc/pci-keystone.c: * bus error instead of returning 0xffffffff. This handler always returns 0
> drivers/pci/controller/pci-xgene.c:      * ready") instead of 0xFFFFFFFF ("device does not exist").  This
> drivers/pci/controller/pcie-iproc.c:     * eventually return the wrong data (0xffffffff).
> drivers/pci/pci.c: * FFFFFFFFs on the command line.)
> 
> I've removed anything in the above list that doesn't look like a good candidate
> for PCI_ERROR_RESPONSE.
> 
> Perhaps there is some value for replacing "~0" with "~0 (PCI_ERROR_RESPONSE)"
> in the comments too?

Good idea, I'll take a look at those.

> >  		pdev->cfg_size = PCI_CFG_SPACE_EXP_SIZE;
> >  		if (pci_read_config_dword(pdev, PCI_CFG_SPACE_SIZE, &status) !=
> > -		    PCIBIOS_SUCCESSFUL || (status == 0xffffffff))
> > +		    PCIBIOS_SUCCESSFUL || (status == (u32) PCI_ERROR_RESPONSE))
> 
> The casts are necessary but slightly annoying. Have you considered something
> like:
> 
> #define SET_PCI_ERROR_RESPONSE(val)	(val = ((typeof(val))(~0ULL)))
> #define RESPONSE_IS_PCI_ERROR(val)	(val == ((typeof(val))(~0ULL)))

I hadn't thought of that, but I really like the idea.  Thanks, I think
I'll try that out!

> >  			pdev->cfg_size = PCI_CFG_SPACE_SIZE;
> >  
> >  		if (pci_find_saved_cap(pdev, PCI_CAP_ID_EXP))
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 9e700d9f9f28..d64fd3788061 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -123,6 +123,13 @@ enum pci_interrupt_pin {
> >  /* The number of legacy PCI INTx interrupts */
> >  #define PCI_NUM_INTX	4
> >  
> > +/*
> > + * Reading from a device that doesn't respond typically returns ~0.  A
> > + * successful read from a device may also return ~0, so you need additional
> > + * information to reliably identify errors.
> > + */
> > +#define PCI_ERROR_RESPONSE		(~0ULL)
> > +
> >  /*
> >   * pci_power_t values must match the bits in the Capabilities PME_Support
> >   * and Control/Status PowerState fields in the Power Management capability.
> > -- 
> > 2.23.0.187.g17f5b7556c-goog
> >