[PATCH 1/3] PCI: rcar: Replace unsigned long with u32 for register values

[PATCH 1/3] PCI: rcar: Replace unsigned long with u32 for register values

[marek.vasut]

From: Marek Vasut <marek.vasut+renesas@gmail.com>

Replace unsigned long with u32 type for variables holding
register values, since the registers are 32bit. Note that
rcar_pcie_msi_irq() still uses unsigned long because both
find_first_bit() and __fls() require unsigned long as an
argument.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Phil Edworthy <phil.edworthy@renesas.com>
Cc: Simon Horman <horms+renesas@verge.net.au>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: linux-renesas-soc@vger.kernel.org
To: linux-pci@vger.kernel.org
---
 drivers/pci/controller/pcie-rcar.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
index 1408c8aa758b..857d88fd03d5 100644
--- a/drivers/pci/controller/pcie-rcar.c
+++ b/drivers/pci/controller/pcie-rcar.c
@@ -169,7 +169,7 @@ enum {
 
 static void rcar_rmw32(struct rcar_pcie *pcie, int where, u32 mask, u32 data)
 {
-	int shift = 8 * (where & 3);
+	u32 shift = 8 * (where & 3);
 	u32 val = rcar_pci_read_reg(pcie, where & ~3);
 
 	val &= ~(mask << shift);
@@ -179,7 +179,7 @@ static void rcar_rmw32(struct rcar_pcie *pcie, int where, u32 mask, u32 data)
 
 static u32 rcar_read_conf(struct rcar_pcie *pcie, int where)
 {
-	int shift = 8 * (where & 3);
+	u32 shift = 8 * (where & 3);
 	u32 val = rcar_pci_read_reg(pcie, where & ~3);
 
 	return val >> shift;
@@ -190,7 +190,7 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie,
 		unsigned char access_type, struct pci_bus *bus,
 		unsigned int devfn, int where, u32 *data)
 {
-	int dev, func, reg, index;
+	u32 dev, func, reg, index;
 
 	dev = PCI_SLOT(devfn);
 	func = PCI_FUNC(devfn);
@@ -508,7 +508,7 @@ static void phy_write_reg(struct rcar_pcie *pcie,
 				 unsigned int rate, unsigned int addr,
 				 unsigned int lane, unsigned int data)
 {
-	unsigned long phyaddr;
+	u32 phyaddr;
 
 	phyaddr = WRITE_CMD |
 		((rate & 1) << RATE_POS) |
@@ -1116,7 +1116,7 @@ static int rcar_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct rcar_pcie *pcie;
-	unsigned int data;
+	u32 data;
 	int err;
 	int (*phy_init_fn)(struct rcar_pcie *);
 	struct pci_host_bridge *bridge;
-- 
2.20.1

[PATCH 2/3] PCI: rcar: Allow 64bit MSI addresses

[marek.vasut]

From: Marek Vasut <marek.vasut+renesas@gmail.com>

The MSI address can be 64bit. Switch the data type used to hold the
result of virt_to_phys() to phys_addr_t to reflect it's properties
correctly and program the top 32bits of PA into PCIEMSIAUR.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Phil Edworthy <phil.edworthy@renesas.com>
Cc: Simon Horman <horms+renesas@verge.net.au>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: linux-renesas-soc@vger.kernel.org
To: linux-pci@vger.kernel.org
---
 drivers/pci/controller/pcie-rcar.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
index 857d88fd03d5..801925a136ae 100644
--- a/drivers/pci/controller/pcie-rcar.c
+++ b/drivers/pci/controller/pcie-rcar.c
@@ -888,7 +888,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
 	struct rcar_msi *msi = &pcie->msi;
-	unsigned long base;
+	phys_addr_t base;
 	int err, i;
 
 	mutex_init(&msi->lock);
@@ -930,7 +930,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie)
 	base = virt_to_phys((void *)msi->pages);
 
 	rcar_pci_write_reg(pcie, base | MSIFE, PCIEMSIALR);
-	rcar_pci_write_reg(pcie, 0, PCIEMSIAUR);
+	rcar_pci_write_reg(pcie, base >> 32, PCIEMSIAUR);
 
 	/* enable all MSI interrupts */
 	rcar_pci_write_reg(pcie, 0xffffffff, PCIEMSIIER);
-- 
2.20.1

[PATCH 3/3] PCI: rcar: Clean up debug messages

[marek.vasut]

From: Marek Vasut <marek.vasut+renesas@gmail.com>

Drop useless casts from debug messages, they are no longer needed
due to the data type cleanup.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Phil Edworthy <phil.edworthy@renesas.com>
Cc: Simon Horman <horms+renesas@verge.net.au>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: linux-renesas-soc@vger.kernel.org
To: linux-pci@vger.kernel.org
---
 drivers/pci/controller/pcie-rcar.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
index 801925a136ae..2a012654686e 100644
--- a/drivers/pci/controller/pcie-rcar.c
+++ b/drivers/pci/controller/pcie-rcar.c
@@ -283,8 +283,8 @@ static int rcar_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,
 	else if (size == 2)
 		*val = (*val >> (8 * (where & 2))) & 0xffff;
 
-	dev_dbg(&bus->dev, "pcie-config-read: bus=%3d devfn=0x%04x where=0x%04x size=%d val=0x%08lx\n",
-		bus->number, devfn, where, size, (unsigned long)*val);
+	dev_dbg(&bus->dev, "pcie-config-read: bus=%3d devfn=0x%04x where=0x%04x size=%d val=0x%08x\n",
+		bus->number, devfn, where, size, *val);
 
 	return ret;
 }
@@ -302,8 +302,8 @@ static int rcar_pcie_write_conf(struct pci_bus *bus, unsigned int devfn,
 	if (ret != PCIBIOS_SUCCESSFUL)
 		return ret;
 
-	dev_dbg(&bus->dev, "pcie-config-write: bus=%3d devfn=0x%04x where=0x%04x size=%d val=0x%08lx\n",
-		bus->number, devfn, where, size, (unsigned long)val);
+	dev_dbg(&bus->dev, "pcie-config-write: bus=%3d devfn=0x%04x where=0x%04x size=%d val=0x%08x\n",
+		bus->number, devfn, where, size, val);
 
 	if (size == 1) {
 		shift = 8 * (where & 3);
-- 
2.20.1

Re: [PATCH 2/3] PCI: rcar: Allow 64bit MSI addresses

[Sergei Shtylyov]

Hello!

On 17.03.2019 3:06, marek.vasut@gmail.com wrote:

> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> 
> The MSI address can be 64bit. Switch the data type used to hold the
> result of virt_to_phys() to phys_addr_t to reflect it's properties

    Its.

> correctly and program the top 32bits of PA into PCIEMSIAUR.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Phil Edworthy <phil.edworthy@renesas.com>
> Cc: Simon Horman <horms+renesas@verge.net.au>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: linux-renesas-soc@vger.kernel.org
> To: linux-pci@vger.kernel.org
[...]

MBR, Sergei

Re: [PATCH 1/3] PCI: rcar: Replace unsigned long with u32 for register values

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 775 bytes --]

On Sun, Mar 17, 2019 at 01:06:06AM +0100, marek.vasut@gmail.com wrote:
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> 
> Replace unsigned long with u32 type for variables holding

s/unsigned long/various variable types/

> register values, since the registers are 32bit. Note that
> rcar_pcie_msi_irq() still uses unsigned long because both
> find_first_bit() and __fls() require unsigned long as an
> argument.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
...

> -	int shift = 8 * (where & 3);
> +	u32 shift = 8 * (where & 3);

Minor nit: Since this is about shifting, maybe replace 8 with << 3 while
we are here?

There is also a 'shift' var in rcar_pcie_write_conf(). I think we should
convert this for consistency, too?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/3] PCI: rcar: Allow 64bit MSI addresses

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 549 bytes --]

On Sun, Mar 17, 2019 at 01:06:07AM +0100, marek.vasut@gmail.com wrote:
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> 
> The MSI address can be 64bit. Switch the data type used to hold the
> result of virt_to_phys() to phys_addr_t to reflect it's properties
> correctly and program the top 32bits of PA into PCIEMSIAUR.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>

Looks sane. Not being a PCI expert, I wonder: Were we just lucky to not
hit a 64-bit MSI address before? Is this tied to our 32-bit limitation?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] PCI: rcar: Clean up debug messages

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 363 bytes --]

On Sun, Mar 17, 2019 at 01:06:08AM +0100, marek.vasut@gmail.com wrote:
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> 
> Drop useless casts from debug messages, they are no longer needed
> due to the data type cleanup.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] PCI: rcar: Replace unsigned long with u32 for register values

[Marek Vasut]

On 3/17/19 10:09 AM, Wolfram Sang wrote:
> On Sun, Mar 17, 2019 at 01:06:06AM +0100, marek.vasut@gmail.com wrote:
>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>
>> Replace unsigned long with u32 type for variables holding
> 
> s/unsigned long/various variable types/

Fixed

>> register values, since the registers are 32bit. Note that
>> rcar_pcie_msi_irq() still uses unsigned long because both
>> find_first_bit() and __fls() require unsigned long as an
>> argument.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> ...
> 
>> -	int shift = 8 * (where & 3);
>> +	u32 shift = 8 * (where & 3);
> 
> Minor nit: Since this is about shifting, maybe replace 8 with << 3 while
> we are here?
> 
> There is also a 'shift' var in rcar_pcie_write_conf(). I think we should
> convert this for consistency, too?

OK, I might as well collect this and the other cleanup series and repost
it together as a V2 to make it easier to pick.

-- 
Best regards,
Marek Vasut

Re: [PATCH 2/3] PCI: rcar: Allow 64bit MSI addresses

[Marek Vasut]

On 3/17/19 9:03 AM, Sergei Shtylyov wrote:
> Hello!
> 
> On 17.03.2019 3:06, marek.vasut@gmail.com wrote:
> 
>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>
>> The MSI address can be 64bit. Switch the data type used to hold the
>> result of virt_to_phys() to phys_addr_t to reflect it's properties
> 
>    Its.

Fixed

>> correctly and program the top 32bits of PA into PCIEMSIAUR.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>> Cc: Phil Edworthy <phil.edworthy@renesas.com>
>> Cc: Simon Horman <horms+renesas@verge.net.au>
>> Cc: Wolfram Sang <wsa@the-dreams.de>
>> Cc: linux-renesas-soc@vger.kernel.org
>> To: linux-pci@vger.kernel.org
> [...]
> 
> MBR, Sergei


-- 
Best regards,
Marek Vasut

Re: [PATCH 2/3] PCI: rcar: Allow 64bit MSI addresses

[Marek Vasut]

On 3/17/19 10:12 AM, Wolfram Sang wrote:
> On Sun, Mar 17, 2019 at 01:06:07AM +0100, marek.vasut@gmail.com wrote:
>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>
>> The MSI address can be 64bit. Switch the data type used to hold the
>> result of virt_to_phys() to phys_addr_t to reflect it's properties
>> correctly and program the top 32bits of PA into PCIEMSIAUR.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> 
> Looks sane. Not being a PCI expert, I wonder: Were we just lucky to not
> hit a 64-bit MSI address before?

I wonder about that, virt_to_phys(__get_free_pages(GFP_KERNEL, 0)) would
happily return 64bit address, but with the cards I tested (a few intel
NICs [igb, e1000e], PCIe NVME SSDs and xHCI HCD), I am getting the MSIs
either way.

> Is this tied to our 32-bit limitation?

This might be a question for the HW team. I would be tempted to
cautiously say yes.

-- 
Best regards,
Marek Vasut

Re: [PATCH 1/3] PCI: rcar: Replace unsigned long with u32 for register values

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 268 bytes --]


> > There is also a 'shift' var in rcar_pcie_write_conf(). I think we should
> > convert this for consistency, too?
> 
> OK, I might as well collect this and the other cleanup series and repost
> it together as a V2 to make it easier to pick.

Sounds good!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] PCI: rcar: Clean up debug messages

[Geert Uytterhoeven]

On Sun, Mar 17, 2019 at 1:06 AM <marek.vasut@gmail.com> wrote:
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>
> Drop useless casts from debug messages, they are no longer needed
> due to the data type cleanup.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/3] PCI: rcar: Allow 64bit MSI addresses

[Geert Uytterhoeven]

On Sun, Mar 17, 2019 at 1:06 AM <marek.vasut@gmail.com> wrote:
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>
> The MSI address can be 64bit. Switch the data type used to hold the
> result of virt_to_phys() to phys_addr_t to reflect it's properties

Side note: probably this should use a proper DMA API instead of
get_free_pages()/virt_to_phys().

> correctly and program the top 32bits of PA into PCIEMSIAUR.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/drivers/pci/controller/pcie-rcar.c
> +++ b/drivers/pci/controller/pcie-rcar.c

> @@ -930,7 +930,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie *pcie)
>         base = virt_to_phys((void *)msi->pages);
>
>         rcar_pci_write_reg(pcie, base | MSIFE, PCIEMSIALR);
> -       rcar_pci_write_reg(pcie, 0, PCIEMSIAUR);
> +       rcar_pci_write_reg(pcie, base >> 32, PCIEMSIAUR);

Note that his register is now non-zero.
According to the documentation, clearing PCIEMSIALR.MSIFE should be
sufficient to disable MSI, and thus there's no need to zero PCIEMSIAUR
in rcar_pcie_teardown_msi().
So nothing to change there, good.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/3] PCI: rcar: Allow 64bit MSI addresses

[Geert Uytterhoeven]

Hi Marek,

On Mon, Mar 18, 2019 at 12:39 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 3/17/19 10:12 AM, Wolfram Sang wrote:
> > On Sun, Mar 17, 2019 at 01:06:07AM +0100, marek.vasut@gmail.com wrote:
> >> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>
> >> The MSI address can be 64bit. Switch the data type used to hold the
> >> result of virt_to_phys() to phys_addr_t to reflect it's properties
> >> correctly and program the top 32bits of PA into PCIEMSIAUR.
> >>
> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >
> > Looks sane. Not being a PCI expert, I wonder: Were we just lucky to not
> > hit a 64-bit MSI address before?
>
> I wonder about that, virt_to_phys(__get_free_pages(GFP_KERNEL, 0)) would
> happily return 64bit address, but with the cards I tested (a few intel
> NICs [igb, e1000e], PCIe NVME SSDs and xHCI HCD), I am getting the MSIs
> either way.

No doubt you would be receiving the MSIs, if you have RAM at the truncated
address, but wouldn't that cause memory corruption?

Fixes: 290c1fb358605402 ("PCI: rcar: Add MSI support for PCIe")

When MSI support was added, only R-Car H1 and Gen2 were supported.
H1 doesn't have LPAE. Gen2 has, but it might have been disabled.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/3] PCI: rcar: Replace unsigned long with u32 for register values

[Geert Uytterhoeven]

Hi Marek,

On Sun, Mar 17, 2019 at 1:06 AM <marek.vasut@gmail.com> wrote:
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>
> Replace unsigned long with u32 type for variables holding
> register values, since the registers are 32bit. Note that
> rcar_pcie_msi_irq() still uses unsigned long because both
> find_first_bit() and __fls() require unsigned long as an
> argument.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Phil Edworthy <phil.edworthy@renesas.com>
> Cc: Simon Horman <horms+renesas@verge.net.au>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: linux-renesas-soc@vger.kernel.org
> To: linux-pci@vger.kernel.org
> ---
>  drivers/pci/controller/pcie-rcar.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> index 1408c8aa758b..857d88fd03d5 100644
> --- a/drivers/pci/controller/pcie-rcar.c
> +++ b/drivers/pci/controller/pcie-rcar.c
> @@ -169,7 +169,7 @@ enum {
>
>  static void rcar_rmw32(struct rcar_pcie *pcie, int where, u32 mask, u32 data)
>  {
> -       int shift = 8 * (where & 3);
> +       u32 shift = 8 * (where & 3);

shift is not a register value, so IMHO the original type is fine (the "int"
comes from the pci_ops API, BTW).

>         u32 val = rcar_pci_read_reg(pcie, where & ~3);
>
>         val &= ~(mask << shift);
> @@ -179,7 +179,7 @@ static void rcar_rmw32(struct rcar_pcie *pcie, int where, u32 mask, u32 data)
>
>  static u32 rcar_read_conf(struct rcar_pcie *pcie, int where)
>  {
> -       int shift = 8 * (where & 3);
> +       u32 shift = 8 * (where & 3);

Likewise

>         u32 val = rcar_pci_read_reg(pcie, where & ~3);
>
>         return val >> shift;
> @@ -190,7 +190,7 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie,
>                 unsigned char access_type, struct pci_bus *bus,
>                 unsigned int devfn, int where, u32 *data)
>  {
> -       int dev, func, reg, index;
> +       u32 dev, func, reg, index;

reg is the only register value, isn't it?

>
>         dev = PCI_SLOT(devfn);
>         func = PCI_FUNC(devfn);
> @@ -508,7 +508,7 @@ static void phy_write_reg(struct rcar_pcie *pcie,
>                                  unsigned int rate, unsigned int addr,
>                                  unsigned int lane, unsigned int data)
>  {
> -       unsigned long phyaddr;
> +       u32 phyaddr;
>
>         phyaddr = WRITE_CMD |
>                 ((rate & 1) << RATE_POS) |
> @@ -1116,7 +1116,7 @@ static int rcar_pcie_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
>         struct rcar_pcie *pcie;
> -       unsigned int data;
> +       u32 data;
>         int err;
>         int (*phy_init_fn)(struct rcar_pcie *);
>         struct pci_host_bridge *bridge;

For the last two changes:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/3] PCI: rcar: Allow 64bit MSI addresses

[Geert Uytterhoeven]

Hi Marek,

On Mon, Mar 18, 2019 at 9:39 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Mon, Mar 18, 2019 at 12:39 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> > On 3/17/19 10:12 AM, Wolfram Sang wrote:
> > > On Sun, Mar 17, 2019 at 01:06:07AM +0100, marek.vasut@gmail.com wrote:
> > >> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> > >>
> > >> The MSI address can be 64bit. Switch the data type used to hold the
> > >> result of virt_to_phys() to phys_addr_t to reflect it's properties
> > >> correctly and program the top 32bits of PA into PCIEMSIAUR.
> > >>
> > >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> > >
> > > Looks sane. Not being a PCI expert, I wonder: Were we just lucky to not
> > > hit a 64-bit MSI address before?
> >
> > I wonder about that, virt_to_phys(__get_free_pages(GFP_KERNEL, 0)) would
> > happily return 64bit address, but with the cards I tested (a few intel
> > NICs [igb, e1000e], PCIe NVME SSDs and xHCI HCD), I am getting the MSIs
> > either way.
>
> No doubt you would be receiving the MSIs, if you have RAM at the truncated
> address, but wouldn't that cause memory corruption?
>
> Fixes: 290c1fb358605402 ("PCI: rcar: Add MSI support for PCIe")
>
> When MSI support was added, only R-Car H1 and Gen2 were supported.
> H1 doesn't have LPAE. Gen2 has, but it might have been disabled.

Correction: as this is always mapped kernel memory, LPAE doesn't matter.
So the bug matters for arm64 only.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/3] PCI: rcar: Allow 64bit MSI addresses

[Marek Vasut]

On 3/18/19 10:30 AM, Geert Uytterhoeven wrote:
> Hi Marek,
> 
> On Mon, Mar 18, 2019 at 9:39 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Mon, Mar 18, 2019 at 12:39 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>>> On 3/17/19 10:12 AM, Wolfram Sang wrote:
>>>> On Sun, Mar 17, 2019 at 01:06:07AM +0100, marek.vasut@gmail.com wrote:
>>>>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>
>>>>> The MSI address can be 64bit. Switch the data type used to hold the
>>>>> result of virt_to_phys() to phys_addr_t to reflect it's properties
>>>>> correctly and program the top 32bits of PA into PCIEMSIAUR.
>>>>>
>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>
>>>> Looks sane. Not being a PCI expert, I wonder: Were we just lucky to not
>>>> hit a 64-bit MSI address before?
>>>
>>> I wonder about that, virt_to_phys(__get_free_pages(GFP_KERNEL, 0)) would
>>> happily return 64bit address, but with the cards I tested (a few intel
>>> NICs [igb, e1000e], PCIe NVME SSDs and xHCI HCD), I am getting the MSIs
>>> either way.
>>
>> No doubt you would be receiving the MSIs, if you have RAM at the truncated
>> address, but wouldn't that cause memory corruption?
>>
>> Fixes: 290c1fb358605402 ("PCI: rcar: Add MSI support for PCIe")
>>
>> When MSI support was added, only R-Car H1 and Gen2 were supported.
>> H1 doesn't have LPAE. Gen2 has, but it might have been disabled.
> 
> Correction: as this is always mapped kernel memory, LPAE doesn't matter.
> So the bug matters for arm64 only.

And since the address is in the 0x7_3xxx_xxxx range on H3 S-XS, there is
no visible memory corruption. Joy ...

-- 
Best regards,
Marek Vasut

Re: [PATCH 1/3] PCI: rcar: Replace unsigned long with u32 for register values

[Marek Vasut]

On 3/18/19 9:47 AM, Geert Uytterhoeven wrote:
> Hi Marek,
> 
> On Sun, Mar 17, 2019 at 1:06 AM <marek.vasut@gmail.com> wrote:
>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>
>> Replace unsigned long with u32 type for variables holding
>> register values, since the registers are 32bit. Note that
>> rcar_pcie_msi_irq() still uses unsigned long because both
>> find_first_bit() and __fls() require unsigned long as an
>> argument.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>> Cc: Phil Edworthy <phil.edworthy@renesas.com>
>> Cc: Simon Horman <horms+renesas@verge.net.au>
>> Cc: Wolfram Sang <wsa@the-dreams.de>
>> Cc: linux-renesas-soc@vger.kernel.org
>> To: linux-pci@vger.kernel.org
>> ---
>>  drivers/pci/controller/pcie-rcar.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
>> index 1408c8aa758b..857d88fd03d5 100644
>> --- a/drivers/pci/controller/pcie-rcar.c
>> +++ b/drivers/pci/controller/pcie-rcar.c
>> @@ -169,7 +169,7 @@ enum {
>>
>>  static void rcar_rmw32(struct rcar_pcie *pcie, int where, u32 mask, u32 data)
>>  {
>> -       int shift = 8 * (where & 3);
>> +       u32 shift = 8 * (where & 3);
> 
> shift is not a register value, so IMHO the original type is fine (the "int"
> comes from the pci_ops API, BTW).

I presume it should be at least unsigned ?

[...]

-- 
Best regards,
Marek Vasut

Re: [PATCH 1/3] PCI: rcar: Replace unsigned long with u32 for register values

[Geert Uytterhoeven]

Hi Marek,

On Thu, Mar 21, 2019 at 5:14 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 3/18/19 9:47 AM, Geert Uytterhoeven wrote:
> > On Sun, Mar 17, 2019 at 1:06 AM <marek.vasut@gmail.com> wrote:
> >> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>
> >> Replace unsigned long with u32 type for variables holding
> >> register values, since the registers are 32bit. Note that
> >> rcar_pcie_msi_irq() still uses unsigned long because both
> >> find_first_bit() and __fls() require unsigned long as an
> >> argument.
> >>
> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> >> Cc: Phil Edworthy <phil.edworthy@renesas.com>
> >> Cc: Simon Horman <horms+renesas@verge.net.au>
> >> Cc: Wolfram Sang <wsa@the-dreams.de>
> >> Cc: linux-renesas-soc@vger.kernel.org
> >> To: linux-pci@vger.kernel.org
> >> ---
> >>  drivers/pci/controller/pcie-rcar.c | 10 +++++-----
> >>  1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> >> index 1408c8aa758b..857d88fd03d5 100644
> >> --- a/drivers/pci/controller/pcie-rcar.c
> >> +++ b/drivers/pci/controller/pcie-rcar.c
> >> @@ -169,7 +169,7 @@ enum {
> >>
> >>  static void rcar_rmw32(struct rcar_pcie *pcie, int where, u32 mask, u32 data)
> >>  {
> >> -       int shift = 8 * (where & 3);
> >> +       u32 shift = 8 * (where & 3);
> >
> > shift is not a register value, so IMHO the original type is fine (the "int"
> > comes from the pci_ops API, BTW).
>
> I presume it should be at least unsigned ?

Yes, and "where" too.

Note that the other uses of "where" are also int, and IMHO should be
unsigned, too. But changing that means changing the PCI API and all
drivers, sigh...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/3] PCI: rcar: Allow 64bit MSI addresses

[Marek Vasut]

On 3/18/19 9:35 AM, Geert Uytterhoeven wrote:
> On Sun, Mar 17, 2019 at 1:06 AM <marek.vasut@gmail.com> wrote:
>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>
>> The MSI address can be 64bit. Switch the data type used to hold the
>> result of virt_to_phys() to phys_addr_t to reflect it's properties
> 
> Side note: probably this should use a proper DMA API instead of
> get_free_pages()/virt_to_phys().

In fact, I think it doesn't matter. The MSI message is never written
into this page as an actual memory write, it is interpreted by the
hardware as an interrupt when it arrives. So I believe this page is just
a safety net, which is never actually written.

>> correctly and program the top 32bits of PA into PCIEMSIAUR.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>

[...]

-- 
Best regards,
Marek Vasut