| Message ID | 1563200122-8323-1-git-send-email-jaz@semihalf.com |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: aardvark: fix big endian support | expand |
Hello Grzegorz, Thanks for this work. I indeed never tested this code on BE platforms, and it is very possible that I overlooked endianness issues, so thanks for having a look at this and proposing some patches. See some questions/comments below. On Mon, 15 Jul 2019 16:15:22 +0200 Grzegorz Jaszczyk <jaz@semihalf.com> wrote: > Initialise every not-byte wide fields of emulated pci bridge config > space with proper cpu_to_le* macro. This is required since the structure > describing config space of emulated bridge assumes little-endian > convention. > > Signed-off-by: Grzegorz Jaszczyk <jaz@semihalf.com> > --- > drivers/pci/controller/pci-aardvark.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c > index 134e030..06a12749 100644 > --- a/drivers/pci/controller/pci-aardvark.c > +++ b/drivers/pci/controller/pci-aardvark.c > @@ -479,8 +479,10 @@ static void advk_sw_pci_bridge_init(struct advk_pcie *pcie) > { > struct pci_bridge_emul *bridge = &pcie->bridge; > > - bridge->conf.vendor = advk_readl(pcie, PCIE_CORE_DEV_ID_REG) & 0xffff; > - bridge->conf.device = advk_readl(pcie, PCIE_CORE_DEV_ID_REG) >> 16; > + bridge->conf.vendor = > + cpu_to_le16(advk_readl(pcie, PCIE_CORE_DEV_ID_REG) & 0xffff); > + bridge->conf.device = > + cpu_to_le16(advk_readl(pcie, PCIE_CORE_DEV_ID_REG) >> 16); > bridge->conf.class_revision = > advk_readl(pcie, PCIE_CORE_DEV_REV_REG) & 0xff; So conf.vendor and conf.device and stored as little-endian in the emulated config address space, but conf.class_revision is stored in the CPU endianness ? > > @@ -489,8 +491,8 @@ static void advk_sw_pci_bridge_init(struct advk_pcie *pcie) > bridge->conf.iolimit = PCI_IO_RANGE_TYPE_32; > > /* Support 64 bits memory pref */ > - bridge->conf.pref_mem_base = PCI_PREF_RANGE_TYPE_64; > - bridge->conf.pref_mem_limit = PCI_PREF_RANGE_TYPE_64; > + bridge->conf.pref_mem_base = cpu_to_le16(PCI_PREF_RANGE_TYPE_64); > + bridge->conf.pref_mem_limit = cpu_to_le16(PCI_PREF_RANGE_TYPE_64); Same here: why are conf.pref_mem_{base,limit} converted to LE, but not conf.iolimit ? Also, the advk_pci_bridge_emul_pcie_conf_read() and advk_pci_bridge_emul_pcie_conf_write() return values that are in the CPU endianness. Am I missing something ? Thomas
On Mon, Jul 15, 2019 at 05:08:40PM +0200, Thomas Petazzoni wrote: > Hello Grzegorz, > > Thanks for this work. I indeed never tested this code on BE platforms, > and it is very possible that I overlooked endianness issues, so thanks > for having a look at this and proposing some patches. See some > questions/comments below. > > On Mon, 15 Jul 2019 16:15:22 +0200 > Grzegorz Jaszczyk <jaz@semihalf.com> wrote: > > > Initialise every not-byte wide fields of emulated pci bridge config > > space with proper cpu_to_le* macro. This is required since the structure > > describing config space of emulated bridge assumes little-endian > > convention. > > > > Signed-off-by: Grzegorz Jaszczyk <jaz@semihalf.com> > > --- > > drivers/pci/controller/pci-aardvark.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c > > index 134e030..06a12749 100644 > > --- a/drivers/pci/controller/pci-aardvark.c > > +++ b/drivers/pci/controller/pci-aardvark.c > > @@ -479,8 +479,10 @@ static void advk_sw_pci_bridge_init(struct advk_pcie *pcie) > > { > > struct pci_bridge_emul *bridge = &pcie->bridge; > > > > - bridge->conf.vendor = advk_readl(pcie, PCIE_CORE_DEV_ID_REG) & 0xffff; > > - bridge->conf.device = advk_readl(pcie, PCIE_CORE_DEV_ID_REG) >> 16; > > + bridge->conf.vendor = > > + cpu_to_le16(advk_readl(pcie, PCIE_CORE_DEV_ID_REG) & 0xffff); > > + bridge->conf.device = > > + cpu_to_le16(advk_readl(pcie, PCIE_CORE_DEV_ID_REG) >> 16); > > bridge->conf.class_revision = > > advk_readl(pcie, PCIE_CORE_DEV_REV_REG) & 0xff; > > So conf.vendor and conf.device and stored as little-endian in the > emulated config address space, but conf.class_revision is stored in the > CPU endianness ? > > > > > @@ -489,8 +491,8 @@ static void advk_sw_pci_bridge_init(struct advk_pcie *pcie) > > bridge->conf.iolimit = PCI_IO_RANGE_TYPE_32; > > > > > /* Support 64 bits memory pref */ > > - bridge->conf.pref_mem_base = PCI_PREF_RANGE_TYPE_64; > > - bridge->conf.pref_mem_limit = PCI_PREF_RANGE_TYPE_64; > > + bridge->conf.pref_mem_base = cpu_to_le16(PCI_PREF_RANGE_TYPE_64); > > + bridge->conf.pref_mem_limit = cpu_to_le16(PCI_PREF_RANGE_TYPE_64); > > Same here: why are conf.pref_mem_{base,limit} converted to LE, but not > conf.iolimit ? > > Also, the advk_pci_bridge_emul_pcie_conf_read() and > advk_pci_bridge_emul_pcie_conf_write() return values that are in the > CPU endianness. > > Am I missing something ? Getting the types correct and then using Sparse to validate the code will help to identify issues exactly like this.
Hello, On Mon, 15 Jul 2019 16:10:16 +0100 Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > Also, the advk_pci_bridge_emul_pcie_conf_read() and > > advk_pci_bridge_emul_pcie_conf_write() return values that are in the > > CPU endianness. > > > > Am I missing something ? > > Getting the types correct and then using Sparse to validate the code > will help to identify issues exactly like this. Yes, I absolutely agree with your recommendation on the other thread. In fact, I am wondering if it really makes sense to store the "fake" config space in LE, since anyway the read/write accessors should return values in the CPU endianness. Best regards, Thomas
Hi Thomas, pon., 15 lip 2019 o 17:08 Thomas Petazzoni <thomas.petazzoni@bootlin.com> napisał(a): > > > > - bridge->conf.vendor = advk_readl(pcie, PCIE_CORE_DEV_ID_REG) & 0xffff; > > - bridge->conf.device = advk_readl(pcie, PCIE_CORE_DEV_ID_REG) >> 16; > > + bridge->conf.vendor = > > + cpu_to_le16(advk_readl(pcie, PCIE_CORE_DEV_ID_REG) & 0xffff); > > + bridge->conf.device = > > + cpu_to_le16(advk_readl(pcie, PCIE_CORE_DEV_ID_REG) >> 16); > > bridge->conf.class_revision = > > advk_readl(pcie, PCIE_CORE_DEV_REV_REG) & 0xff; > > So conf.vendor and conf.device and stored as little-endian in the > emulated config address space, but conf.class_revision is stored in the > CPU endianness ? Thank you it seems it slip over - after my change I dump whole config space in big and little endian and compere to be sure that there are no more fields that Iam missing and everything seemed ok - so it is probably '0' therefore the bug wasn't caught. In bridge emulation the conversion is done correctly: bridge->conf.class_revision |= cpu_to_le32(PCI_CLASS_BRIDGE_PCI << 16); > > > > > @@ -489,8 +491,8 @@ static void advk_sw_pci_bridge_init(struct advk_pcie *pcie) > > bridge->conf.iolimit = PCI_IO_RANGE_TYPE_32; > > > > > /* Support 64 bits memory pref */ > > - bridge->conf.pref_mem_base = PCI_PREF_RANGE_TYPE_64; > > - bridge->conf.pref_mem_limit = PCI_PREF_RANGE_TYPE_64; > > + bridge->conf.pref_mem_base = cpu_to_le16(PCI_PREF_RANGE_TYPE_64); > > + bridge->conf.pref_mem_limit = cpu_to_le16(PCI_PREF_RANGE_TYPE_64); > > Same here: why are conf.pref_mem_{base,limit} converted to LE, but not > conf.iolimit ? Here we are ok, since iobase and iolimit are 1byte wide. > > > Also, the advk_pci_bridge_emul_pcie_conf_read() and > advk_pci_bridge_emul_pcie_conf_write() return values that are in the > CPU endianness. > > Am I missing something ? Yes because we are mixing the 4byte accesses in advk_pci_bridge_emul_pcie_conf_read/write with u16 and u8 accesses when referring to structure fields directly. E.g. please see what will happen when in BE e.g. device id and vendor id are set via conf->vendor and conf->device and then read via advk_pci_bridge_emul_pcie_conf_read which first read whole 32bit value and then shift it. The same with other not u32 fields. Before my changes PCIe didn't work in BE mode at all - I've tested it on a3700. Nevertheless Russell advice about Sparse validation is really good - it helps to detect some bugs which slip over - I will send v2 soon. Best regards, Grzegorz
On Tue, Jul 16, 2019 at 08:32:04AM +0200, Thomas Petazzoni wrote: > Hello, > > On Mon, 15 Jul 2019 16:10:16 +0100 > Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > > > Also, the advk_pci_bridge_emul_pcie_conf_read() and > > > advk_pci_bridge_emul_pcie_conf_write() return values that are in the > > > CPU endianness. > > > > > > Am I missing something ? > > > > Getting the types correct and then using Sparse to validate the code > > will help to identify issues exactly like this. > > Yes, I absolutely agree with your recommendation on the other thread. > > In fact, I am wondering if it really makes sense to store the "fake" > config space in LE, since anyway the read/write accessors should return > values in the CPU endianness. Consider: u16 vendor; u16 device; and how they are laid out in LE and BE. Then consider what happens when you read "vendor" using a u32 accessor. This is where the problem lies. Using host endian means you'd have to read the members using an appropriately sized host access (in other words, not using a dword accessor) to end up with the correct result - but we don't want a large switch() statement here...
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c index 134e030..06a12749 100644 --- a/drivers/pci/controller/pci-aardvark.c +++ b/drivers/pci/controller/pci-aardvark.c @@ -479,8 +479,10 @@ static void advk_sw_pci_bridge_init(struct advk_pcie *pcie) { struct pci_bridge_emul *bridge = &pcie->bridge; - bridge->conf.vendor = advk_readl(pcie, PCIE_CORE_DEV_ID_REG) & 0xffff; - bridge->conf.device = advk_readl(pcie, PCIE_CORE_DEV_ID_REG) >> 16; + bridge->conf.vendor = + cpu_to_le16(advk_readl(pcie, PCIE_CORE_DEV_ID_REG) & 0xffff); + bridge->conf.device = + cpu_to_le16(advk_readl(pcie, PCIE_CORE_DEV_ID_REG) >> 16); bridge->conf.class_revision = advk_readl(pcie, PCIE_CORE_DEV_REV_REG) & 0xff; @@ -489,8 +491,8 @@ static void advk_sw_pci_bridge_init(struct advk_pcie *pcie) bridge->conf.iolimit = PCI_IO_RANGE_TYPE_32; /* Support 64 bits memory pref */ - bridge->conf.pref_mem_base = PCI_PREF_RANGE_TYPE_64; - bridge->conf.pref_mem_limit = PCI_PREF_RANGE_TYPE_64; + bridge->conf.pref_mem_base = cpu_to_le16(PCI_PREF_RANGE_TYPE_64); + bridge->conf.pref_mem_limit = cpu_to_le16(PCI_PREF_RANGE_TYPE_64); /* Support interrupt A for MSI feature */ bridge->conf.intpin = PCIE_CORE_INT_A_ASSERT_ENABLE;
Initialise every not-byte wide fields of emulated pci bridge config space with proper cpu_to_le* macro. This is required since the structure describing config space of emulated bridge assumes little-endian convention. Signed-off-by: Grzegorz Jaszczyk <jaz@semihalf.com> --- drivers/pci/controller/pci-aardvark.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)