| Message ID | f423dc9cc90e345680d289d5df7ff469e9b89c60.1633972263.git.naveennaidu479@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Unify PCI error response checking | expand |
On Monday 11 October 2021 23:26:33 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, when a faulty > read occurs. > > This helps unify PCI error response checking and make error check > consistent and easier to find. > > Compile tested only. > > Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com> > --- > drivers/pci/controller/pci-aardvark.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c > index 596ebcfcc82d..dc2f820ef55f 100644 > --- a/drivers/pci/controller/pci-aardvark.c > +++ b/drivers/pci/controller/pci-aardvark.c > @@ -894,7 +894,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, > int ret; > > if (!advk_pcie_valid_device(pcie, bus, devfn)) { > - *val = 0xffffffff; > + SET_PCI_ERROR_RESPONSE(val); Hello! Now I'm looking at this macro, and should not it depends on "size" argument? If doing 8-bit or 16-bit read operation then should not it rather sets only low 8 bits or low 16 bits to ones? > return PCIBIOS_DEVICE_NOT_FOUND; > } > > @@ -920,7 +920,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, > *val = CFG_RD_CRS_VAL; > return PCIBIOS_SUCCESSFUL; > } > - *val = 0xffffffff; > + SET_PCI_ERROR_RESPONSE(val); > return PCIBIOS_SET_FAILED; > } > > @@ -955,14 +955,14 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, > *val = CFG_RD_CRS_VAL; > return PCIBIOS_SUCCESSFUL; > } > - *val = 0xffffffff; > + SET_PCI_ERROR_RESPONSE(val); > return PCIBIOS_SET_FAILED; > } > > /* Check PIO status and get the read result */ > ret = advk_pcie_check_pio_status(pcie, allow_crs, val); > if (ret < 0) { > - *val = 0xffffffff; > + SET_PCI_ERROR_RESPONSE(val); > return PCIBIOS_SET_FAILED; > } > > -- > 2.25.1 >
On 11/10, Pali Rohár wrote: > On Monday 11 October 2021 23:26:33 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, when a faulty > > read occurs. > > > > This helps unify PCI error response checking and make error check > > consistent and easier to find. > > > > Compile tested only. > > > > Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com> > > --- > > drivers/pci/controller/pci-aardvark.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c > > index 596ebcfcc82d..dc2f820ef55f 100644 > > --- a/drivers/pci/controller/pci-aardvark.c > > +++ b/drivers/pci/controller/pci-aardvark.c > > @@ -894,7 +894,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, > > int ret; > > > > if (!advk_pcie_valid_device(pcie, bus, devfn)) { > > - *val = 0xffffffff; > > + SET_PCI_ERROR_RESPONSE(val); > > Hello! Now I'm looking at this macro, and should not it depends on > "size" argument? If doing 8-bit or 16-bit read operation then should not > it rather sets only low 8 bits or low 16 bits to ones? > Hello o/, Thank you for the review. Yes! you are right that it should indeed depend on the "size" argument. And that is what the SET_PCI_ERROR_RESPONSE macro does. The macro is defined as: #define PCI_ERROR_RESPONSE (~0ULL) #define SET_PCI_ERROR_RESPONSE(val) (*val = ((typeof(*val))PCI_ERROR_RESPONSE)) The macro was part of "Patch 1/22" and is present here [1]. Apologies if I added the receipient incorrectly. [1]: https://lore.kernel.org/linux-pci/d8e423386aad3d78bca575a7521b138508638e3b.1633972263.git.naveennaidu479@gmail.com/T/#m37295a0dcfe0d7e0f67efce3633efd7b891949c4 IIUC, the typeof(*val) helps in setting the value according to the size of the argument. Please let me know if my understanding is wrong. > > return PCIBIOS_DEVICE_NOT_FOUND; > > } > > > > @@ -920,7 +920,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, > > *val = CFG_RD_CRS_VAL; > > return PCIBIOS_SUCCESSFUL; > > } > > - *val = 0xffffffff; > > + SET_PCI_ERROR_RESPONSE(val); > > return PCIBIOS_SET_FAILED; > > } > > > > @@ -955,14 +955,14 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, > > *val = CFG_RD_CRS_VAL; > > return PCIBIOS_SUCCESSFUL; > > } > > - *val = 0xffffffff; > > + SET_PCI_ERROR_RESPONSE(val); > > return PCIBIOS_SET_FAILED; > > } > > > > /* Check PIO status and get the read result */ > > ret = advk_pcie_check_pio_status(pcie, allow_crs, val); > > if (ret < 0) { > > - *val = 0xffffffff; > > + SET_PCI_ERROR_RESPONSE(val); > > return PCIBIOS_SET_FAILED; > > } > > > > -- > > 2.25.1 > >
On Monday 11 October 2021 23:55:35 Naveen Naidu wrote: > On 11/10, Pali Rohár wrote: > > On Monday 11 October 2021 23:26:33 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, when a faulty > > > read occurs. > > > > > > This helps unify PCI error response checking and make error check > > > consistent and easier to find. > > > > > > Compile tested only. > > > > > > Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com> > > > --- > > > drivers/pci/controller/pci-aardvark.c | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c > > > index 596ebcfcc82d..dc2f820ef55f 100644 > > > --- a/drivers/pci/controller/pci-aardvark.c > > > +++ b/drivers/pci/controller/pci-aardvark.c > > > @@ -894,7 +894,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, > > > int ret; > > > > > > if (!advk_pcie_valid_device(pcie, bus, devfn)) { > > > - *val = 0xffffffff; > > > + SET_PCI_ERROR_RESPONSE(val); > > > > Hello! Now I'm looking at this macro, and should not it depends on > > "size" argument? If doing 8-bit or 16-bit read operation then should not > > it rather sets only low 8 bits or low 16 bits to ones? > > > > Hello o/, Thank you for the review. > > Yes! you are right that it should indeed depend on the "size" argument. > And that is what the SET_PCI_ERROR_RESPONSE macro does. The macro is > defined as: > > #define PCI_ERROR_RESPONSE (~0ULL) > #define SET_PCI_ERROR_RESPONSE(val) (*val = ((typeof(*val))PCI_ERROR_RESPONSE)) > > The macro was part of "Patch 1/22" and is present here [1]. Apologies if > I added the receipient incorrectly. > > [1]: > https://lore.kernel.org/linux-pci/d8e423386aad3d78bca575a7521b138508638e3b.1633972263.git.naveennaidu479@gmail.com/T/#m37295a0dcfe0d7e0f67efce3633efd7b891949c4 > > IIUC, the typeof(*val) helps in setting the value according to the size > of the argument. > > Please let me know if my understanding is wrong. Hello! I mean "size" function argument which is passed as variable. Function itself is declared as: static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where, int size, u32 *val); And in "size" argument is stored number of bytes, kind of read operation (read byte, read word, read dword). In *val is then stored read value. For byte operation, just low 8 bits in *val variable are set. Because *val is u32 it means that typeof(*val) is always 4 independently of the "size" argument. For example other project U-Boot has also pci-aardvark.c driver and U-Boot has for (probably same) purpose pci_get_ff() macro, see: https://source.denx.de/u-boot/u-boot/-/blob/v2021.10/drivers/pci/pci-aardvark.c#L367 I'm not saying if current approach to always sets 0xffffffff (independently of "size" argument) is correct or not as I do not know it too! I'm just giving example that this PCI code has very similar implementation of other project (U-Boot) which sets number of ones based on the size argument. So probably something for other people to decide. Anyway, I very like this your idea to have a macro which purpose is to explicitly indicate error during config read operation! And to unify all drivers to use same style for signalling config read error. > > > return PCIBIOS_DEVICE_NOT_FOUND; > > > } > > > > > > @@ -920,7 +920,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, > > > *val = CFG_RD_CRS_VAL; > > > return PCIBIOS_SUCCESSFUL; > > > } > > > - *val = 0xffffffff; > > > + SET_PCI_ERROR_RESPONSE(val); > > > return PCIBIOS_SET_FAILED; > > > } > > > > > > @@ -955,14 +955,14 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, > > > *val = CFG_RD_CRS_VAL; > > > return PCIBIOS_SUCCESSFUL; > > > } > > > - *val = 0xffffffff; > > > + SET_PCI_ERROR_RESPONSE(val); > > > return PCIBIOS_SET_FAILED; > > > } > > > > > > /* Check PIO status and get the read result */ > > > ret = advk_pcie_check_pio_status(pcie, allow_crs, val); > > > if (ret < 0) { > > > - *val = 0xffffffff; > > > + SET_PCI_ERROR_RESPONSE(val); > > > return PCIBIOS_SET_FAILED; > > > } > > > > > > -- > > > 2.25.1 > > >
On 11/10, Pali Rohár wrote: > On Monday 11 October 2021 23:55:35 Naveen Naidu wrote: > > On 11/10, Pali Rohár wrote: > > > On Monday 11 October 2021 23:26:33 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, when a faulty > > > > read occurs. > > > > > > > > This helps unify PCI error response checking and make error check > > > > consistent and easier to find. > > > > > > > > Compile tested only. > > > > > > > > Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com> > > > > --- > > > > drivers/pci/controller/pci-aardvark.c | 8 ++++---- > > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c > > > > index 596ebcfcc82d..dc2f820ef55f 100644 > > > > --- a/drivers/pci/controller/pci-aardvark.c > > > > +++ b/drivers/pci/controller/pci-aardvark.c > > > > @@ -894,7 +894,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, > > > > int ret; > > > > > > > > if (!advk_pcie_valid_device(pcie, bus, devfn)) { > > > > - *val = 0xffffffff; > > > > + SET_PCI_ERROR_RESPONSE(val); > > > > > > Hello! Now I'm looking at this macro, and should not it depends on > > > "size" argument? If doing 8-bit or 16-bit read operation then should not > > > it rather sets only low 8 bits or low 16 bits to ones? > > > > > > > Hello o/, Thank you for the review. > > > > Yes! you are right that it should indeed depend on the "size" argument. > > And that is what the SET_PCI_ERROR_RESPONSE macro does. The macro is > > defined as: > > > > #define PCI_ERROR_RESPONSE (~0ULL) > > #define SET_PCI_ERROR_RESPONSE(val) (*val = ((typeof(*val))PCI_ERROR_RESPONSE)) > > > > The macro was part of "Patch 1/22" and is present here [1]. Apologies if > > I added the receipient incorrectly. > > > > [1]: > > https://lore.kernel.org/linux-pci/d8e423386aad3d78bca575a7521b138508638e3b.1633972263.git.naveennaidu479@gmail.com/T/#m37295a0dcfe0d7e0f67efce3633efd7b891949c4 > > > > IIUC, the typeof(*val) helps in setting the value according to the size > > of the argument. > > > > Please let me know if my understanding is wrong. > > Hello! I mean "size" function argument which is passed as variable. > Thank you for explaining! Now I understand what you mean :), Apologies for not being not understanding this beforehand. > Function itself is declared as: > > static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where, int size, u32 *val); > > And in "size" argument is stored number of bytes, kind of read operation > (read byte, read word, read dword). In *val is then stored read value. > For byte operation, just low 8 bits in *val variable are set. > > Because *val is u32 it means that typeof(*val) is always 4 independently > of the "size" argument. > > For example other project U-Boot has also pci-aardvark.c driver and > U-Boot has for (probably same) purpose pci_get_ff() macro, see: > https://source.denx.de/u-boot/u-boot/-/blob/v2021.10/drivers/pci/pci-aardvark.c#L367 > > I'm not saying if current approach to always sets 0xffffffff > (independently of "size" argument) is correct or not as I do not know > it too! I'm just giving example that this PCI code has very similar > implementation of other project (U-Boot) which sets number of ones based > on the size argument. > I am not sure too, if we would like to have something like pci_get_ff() which sets the return mask based on the size. If we were to have something like pci_get_ff(), I can think of one problem, some of the functions such as pci_raw_set_power_state() which checks for errors does not have a "size" argument. An excerpt from that function is as follows: static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state) { pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr); if (pmcsr == (u16) ~0) { For these functions we wont be able to use pci_get_ff(), I mean we could definitely put the responsibility onto the programmers to write down the correct size. But that might lead to mistakes, I guess? Then for those cases, we might need to maintain both the SET_PCI_ERROR_RESPONSE macro and the pci_get_ff() functions, which then means that we might not have the same style for signalling config read error. I am pretty new to kernel development, so I am sure that whatever I said above might be totally wrong. If so, please correct me :) > So probably something for other people to decide. > > Anyway, I very like this your idea to have a macro which purpose is to > explicitly indicate error during config read operation! And to unify all > drivers to use same style for signalling config read error. > Thank you :D, I think I'll wait for other people to chime in here with their opinions and then I'll redo the patch with whatever will be decided. Thank again for the detailed reply. > > > > return PCIBIOS_DEVICE_NOT_FOUND; > > > > } > > > > > > > > @@ -920,7 +920,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, > > > > *val = CFG_RD_CRS_VAL; > > > > return PCIBIOS_SUCCESSFUL; > > > > } > > > > - *val = 0xffffffff; > > > > + SET_PCI_ERROR_RESPONSE(val); > > > > return PCIBIOS_SET_FAILED; > > > > } > > > > > > > > @@ -955,14 +955,14 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, > > > > *val = CFG_RD_CRS_VAL; > > > > return PCIBIOS_SUCCESSFUL; > > > > } > > > > - *val = 0xffffffff; > > > > + SET_PCI_ERROR_RESPONSE(val); > > > > return PCIBIOS_SET_FAILED; > > > > } > > > > > > > > /* Check PIO status and get the read result */ > > > > ret = advk_pcie_check_pio_status(pcie, allow_crs, val); > > > > if (ret < 0) { > > > > - *val = 0xffffffff; > > > > + SET_PCI_ERROR_RESPONSE(val); > > > > return PCIBIOS_SET_FAILED; > > > > } > > > > > > > > -- > > > > 2.25.1 > > > >
On Tue, Oct 12, 2021 at 09:29:28PM +0530, Naveen Naidu wrote: > On 11/10, Pali Rohár wrote: > > On Monday 11 October 2021 23:55:35 Naveen Naidu wrote: > > > On 11/10, Pali Rohár wrote: > > > > On Monday 11 October 2021 23:26:33 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, when a faulty > > > > > read occurs. > > > > > > > > > > This helps unify PCI error response checking and make error check > > > > > consistent and easier to find. > > > > > > > > > > Compile tested only. > > > > > > > > > > Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com> > > > > > --- > > > > > drivers/pci/controller/pci-aardvark.c | 8 ++++---- > > > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c > > > > > index 596ebcfcc82d..dc2f820ef55f 100644 > > > > > --- a/drivers/pci/controller/pci-aardvark.c > > > > > +++ b/drivers/pci/controller/pci-aardvark.c > > > > > @@ -894,7 +894,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, > > > > > int ret; > > > > > > > > > > if (!advk_pcie_valid_device(pcie, bus, devfn)) { > > > > > - *val = 0xffffffff; > > > > > + SET_PCI_ERROR_RESPONSE(val); > > > > > > > > Hello! Now I'm looking at this macro, and should not it depends on > > > > "size" argument? If doing 8-bit or 16-bit read operation then should not > > > > it rather sets only low 8 bits or low 16 bits to ones? > > Function itself is declared as: > > > > static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where, int size, u32 *val); > > > > And in "size" argument is stored number of bytes, kind of read operation > > (read byte, read word, read dword). In *val is then stored read value. > > For byte operation, just low 8 bits in *val variable are set. > > > > Because *val is u32 it means that typeof(*val) is always 4 independently > > of the "size" argument. > > > > For example other project U-Boot has also pci-aardvark.c driver and > > U-Boot has for (probably same) purpose pci_get_ff() macro, see: > > https://source.denx.de/u-boot/u-boot/-/blob/v2021.10/drivers/pci/pci-aardvark.c#L367 > > > > I'm not saying if current approach to always sets 0xffffffff > > (independently of "size" argument) is correct or not as I do not know > > it too! I'm just giving example that this PCI code has very similar > > implementation of other project (U-Boot) which sets number of ones based > > on the size argument. I don't think there's an issue here. advk_pcie_rd_conf() can set the whole u32 to 0xffffffff regardless of the "size" value because pci_bus_read_config_byte() and pci_bus_read_config_word() extract out the part they need: res = bus->ops->read(bus, devfn, pos, len, &data); \ *value = (type)data; \ where "type" is u8 or u16 (this is hard to grep for; it's in the PCI_OP_READ() macro in drivers/pci/access.c). > I am not sure too, if we would like to have something like pci_get_ff() > which sets the return mask based on the size. I'd like to see how pci_get_ff() works because this is potentially a widespread, invasive change and it's always better to copy a good existing design than to make up something new. > > Anyway, I very like this your idea to have a macro which purpose is to > > explicitly indicate error during config read operation! And to unify all > > drivers to use same style for signalling config read error. I definitely think there's a lot of value in making this consistent *somehow*, so thanks for working on this! Bjorn
On Tuesday 12 October 2021 21:13:10 Bjorn Helgaas wrote: > On Tue, Oct 12, 2021 at 09:29:28PM +0530, Naveen Naidu wrote: > > On 11/10, Pali Rohár wrote: > > > On Monday 11 October 2021 23:55:35 Naveen Naidu wrote: > > > > On 11/10, Pali Rohár wrote: > > > > > On Monday 11 October 2021 23:26:33 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, when a faulty > > > > > > read occurs. > > > > > > > > > > > > This helps unify PCI error response checking and make error check > > > > > > consistent and easier to find. > > > > > > > > > > > > Compile tested only. > > > > > > > > > > > > Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com> > > > > > > --- > > > > > > drivers/pci/controller/pci-aardvark.c | 8 ++++---- > > > > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c > > > > > > index 596ebcfcc82d..dc2f820ef55f 100644 > > > > > > --- a/drivers/pci/controller/pci-aardvark.c > > > > > > +++ b/drivers/pci/controller/pci-aardvark.c > > > > > > @@ -894,7 +894,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, > > > > > > int ret; > > > > > > > > > > > > if (!advk_pcie_valid_device(pcie, bus, devfn)) { > > > > > > - *val = 0xffffffff; > > > > > > + SET_PCI_ERROR_RESPONSE(val); > > > > > > > > > > Hello! Now I'm looking at this macro, and should not it depends on > > > > > "size" argument? If doing 8-bit or 16-bit read operation then should not > > > > > it rather sets only low 8 bits or low 16 bits to ones? > > > > Function itself is declared as: > > > > > > static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where, int size, u32 *val); > > > > > > And in "size" argument is stored number of bytes, kind of read operation > > > (read byte, read word, read dword). In *val is then stored read value. > > > For byte operation, just low 8 bits in *val variable are set. > > > > > > Because *val is u32 it means that typeof(*val) is always 4 independently > > > of the "size" argument. > > > > > > For example other project U-Boot has also pci-aardvark.c driver and > > > U-Boot has for (probably same) purpose pci_get_ff() macro, see: > > > https://source.denx.de/u-boot/u-boot/-/blob/v2021.10/drivers/pci/pci-aardvark.c#L367 > > > > > > I'm not saying if current approach to always sets 0xffffffff > > > (independently of "size" argument) is correct or not as I do not know > > > it too! I'm just giving example that this PCI code has very similar > > > implementation of other project (U-Boot) which sets number of ones based > > > on the size argument. > > I don't think there's an issue here. advk_pcie_rd_conf() can set the > whole u32 to 0xffffffff regardless of the "size" value because > pci_bus_read_config_byte() and pci_bus_read_config_word() extract out > the part they need: > > res = bus->ops->read(bus, devfn, pos, len, &data); \ > *value = (type)data; \ > > where "type" is u8 or u16 (this is hard to grep for; it's in the > PCI_OP_READ() macro in drivers/pci/access.c). Ok! No problem if this is something which is not going to be changed. > > I am not sure too, if we would like to have something like pci_get_ff() > > which sets the return mask based on the size. > > I'd like to see how pci_get_ff() works because this is potentially a > widespread, invasive change and it's always better to copy a good > existing design than to make up something new. Here is U-Boot implementation of that function, it is pretty simple: https://source.denx.de/u-boot/u-boot/-/blob/v2021.10/drivers/pci/pci-uclass.c#L103-113 > > > Anyway, I very like this your idea to have a macro which purpose is to > > > explicitly indicate error during config read operation! And to unify all > > > drivers to use same style for signalling config read error. > > I definitely think there's a lot of value in making this consistent > *somehow*, so thanks for working on this! +1
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c index 596ebcfcc82d..dc2f820ef55f 100644 --- a/drivers/pci/controller/pci-aardvark.c +++ b/drivers/pci/controller/pci-aardvark.c @@ -894,7 +894,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int ret; if (!advk_pcie_valid_device(pcie, bus, devfn)) { - *val = 0xffffffff; + SET_PCI_ERROR_RESPONSE(val); return PCIBIOS_DEVICE_NOT_FOUND; } @@ -920,7 +920,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, *val = CFG_RD_CRS_VAL; return PCIBIOS_SUCCESSFUL; } - *val = 0xffffffff; + SET_PCI_ERROR_RESPONSE(val); return PCIBIOS_SET_FAILED; } @@ -955,14 +955,14 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn, *val = CFG_RD_CRS_VAL; return PCIBIOS_SUCCESSFUL; } - *val = 0xffffffff; + SET_PCI_ERROR_RESPONSE(val); return PCIBIOS_SET_FAILED; } /* Check PIO status and get the read result */ ret = advk_pcie_check_pio_status(pcie, allow_crs, val); if (ret < 0) { - *val = 0xffffffff; + SET_PCI_ERROR_RESPONSE(val); return PCIBIOS_SET_FAILED; }
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, when a faulty read occurs. This helps unify PCI error response checking and make error check consistent and easier to find. Compile tested only. Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com> --- drivers/pci/controller/pci-aardvark.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)