[PATCH] PCI: aardvark: fix big endian support

[PATCH] PCI: aardvark: fix big endian support

[Grzegorz Jaszczyk]

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;
 
@@ -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;
-- 
2.7.4

Re: [PATCH] PCI: aardvark: fix big endian support

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] PCI: aardvark: fix big endian support

[Russell King - ARM Linux admin]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] PCI: aardvark: fix big endian support

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] PCI: aardvark: fix big endian support

[Grzegorz Jaszczyk]

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

Re: [PATCH] PCI: aardvark: fix big endian support

[Russell King - ARM Linux admin]

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...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up