| Message ID | da939a6cdb2dea96d16392ae152e1232212877d1.1634306198.git.naveennaidu479@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Unify PCI error response checking | expand |
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 > >
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 > > > >
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)) *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);
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(-)