On 11/10, Pali Rohár wrote:

On Monday 11 October 2021 23:55:35 Naveen Naidu wrote:

quoted

On 11/10, Pali Rohár wrote:

quoted

On Monday 11 October 2021 23:26:33 Naveen Naidu wrote:

quoted

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 <redacted>
---
 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 (local)

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.

quoted

quoted

quoted

 		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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel