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@xxxxxxxxxx>
> >
> > 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
> >
