[PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space

[PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space

[marek.vasut]

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

This fixes MSI operation on legacy PCI cards, which cannot issue 64bit MSIs.
The R-Car controller only has one MSI trigger address instead of two, one
for 64bit and one for 32bit MSI, set the address to 32bit PCIe space so that
legacy PCI cards can also trigger MSIs.

Fixes: 290c1fb35860 ("PCI: rcar: Add MSI support for PCIe")
Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: linux-renesas-soc@vger.kernel.org
---
 drivers/pci/controller/pcie-rcar-host.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c
index 1194d5f3341b..ac5c7d7573a6 100644
--- a/drivers/pci/controller/pcie-rcar-host.c
+++ b/drivers/pci/controller/pcie-rcar-host.c
@@ -753,7 +753,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie_host *host)
 	}
 
 	/* setup MSI data target */
-	msi->pages = __get_free_pages(GFP_KERNEL, 0);
+	msi->pages = __get_free_pages(GFP_KERNEL | GFP_DMA32, 0);
 	rcar_pcie_hw_enable_msi(host);
 
 	return 0;
-- 
2.28.0

Re: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space

[Geert Uytterhoeven]

Hi Marek,

On Fri, Oct 16, 2020 at 2:04 PM <marek.vasut@gmail.com> wrote:
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>
> This fixes MSI operation on legacy PCI cards, which cannot issue 64bit MSIs.
> The R-Car controller only has one MSI trigger address instead of two, one
> for 64bit and one for 32bit MSI, set the address to 32bit PCIe space so that
> legacy PCI cards can also trigger MSIs.
>
> Fixes: 290c1fb35860 ("PCI: rcar: Add MSI support for PCIe")
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>

Thanks for your patch!

Seems to work, on both R-Car M2-W and M3-W, as
virt_to_phys((void *)msi->pages) points to RAM below the 4 GiB limit, so
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/drivers/pci/controller/pcie-rcar-host.c
> +++ b/drivers/pci/controller/pcie-rcar-host.c
> @@ -753,7 +753,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie_host *host)
>         }
>
>         /* setup MSI data target */
> -       msi->pages = __get_free_pages(GFP_KERNEL, 0);
> +       msi->pages = __get_free_pages(GFP_KERNEL | GFP_DMA32, 0);

BTW, can this fail, especially now this is allocated from a more
limited pool?

>         rcar_pcie_hw_enable_msi(host);
>
>         return 0;

Regardless:
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] PCI: rcar: Always allocate MSI addresses in 32bit space

[Marek Vasut]

On 10/20/20 9:47 AM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi,

[...]

>> --- a/drivers/pci/controller/pcie-rcar-host.c
>> +++ b/drivers/pci/controller/pcie-rcar-host.c
>> @@ -753,7 +753,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie_host *host)
>>         }
>>
>>         /* setup MSI data target */
>> -       msi->pages = __get_free_pages(GFP_KERNEL, 0);
>> +       msi->pages = __get_free_pages(GFP_KERNEL | GFP_DMA32, 0);
> 
> BTW, can this fail, especially now this is allocated from a more
> limited pool?

I am pretty sure this can fail on systems that don't have DRAM below 4
GiB , but that is never the case on any hardware with this controller.

[...]

RE: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space

[Yoshihiro Shimoda]

Hello Marek-san,

> From: marek.vasut@gmail.com, Sent: Friday, October 16, 2020 9:05 PM
> 
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> 
> This fixes MSI operation on legacy PCI cards, which cannot issue 64bit MSIs.
> The R-Car controller only has one MSI trigger address instead of two, one
> for 64bit and one for 32bit MSI, set the address to 32bit PCIe space so that
> legacy PCI cards can also trigger MSIs.
> 
> Fixes: 290c1fb35860 ("PCI: rcar: Add MSI support for PCIe")
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Cc: linux-renesas-soc@vger.kernel.org

Thank you for the patch!

Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

And I tested on R-Car H3. So,

Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Best regards,
Yoshihiro Shimoda

Re: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space

[Lorenzo Pieralisi]

On Fri, Oct 16, 2020 at 02:04:31PM +0200, marek.vasut@gmail.com wrote:
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> 
> This fixes MSI operation on legacy PCI cards, which cannot issue 64bit MSIs.
> The R-Car controller only has one MSI trigger address instead of two, one
> for 64bit and one for 32bit MSI, set the address to 32bit PCIe space so that
> legacy PCI cards can also trigger MSIs.
> 
> Fixes: 290c1fb35860 ("PCI: rcar: Add MSI support for PCIe")
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Cc: linux-renesas-soc@vger.kernel.org
> ---
>  drivers/pci/controller/pcie-rcar-host.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c
> index 1194d5f3341b..ac5c7d7573a6 100644
> --- a/drivers/pci/controller/pcie-rcar-host.c
> +++ b/drivers/pci/controller/pcie-rcar-host.c
> @@ -753,7 +753,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie_host *host)
>  	}
>  
>  	/* setup MSI data target */
> -	msi->pages = __get_free_pages(GFP_KERNEL, 0);
> +	msi->pages = __get_free_pages(GFP_KERNEL | GFP_DMA32, 0);

This does not do what you want on !CONFIG_ZONE_DMA32 (ie arm LPAE).

Can't you just pick up a non-DMA-able address < 4GB (ie outside the host
controller inbound window range) and use it as doorbell address instead ?

Thanks,
Lorenzo

>  	rcar_pcie_hw_enable_msi(host);
>  
>  	return 0;
> -- 
> 2.28.0
> 

Re: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space

[Marek Vasut]

On 12/10/20 7:11 PM, Lorenzo Pieralisi wrote:

[...]

>> diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c
>> index 1194d5f3341b..ac5c7d7573a6 100644
>> --- a/drivers/pci/controller/pcie-rcar-host.c
>> +++ b/drivers/pci/controller/pcie-rcar-host.c
>> @@ -753,7 +753,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie_host *host)
>>   	}
>>   
>>   	/* setup MSI data target */
>> -	msi->pages = __get_free_pages(GFP_KERNEL, 0);
>> +	msi->pages = __get_free_pages(GFP_KERNEL | GFP_DMA32, 0);
> 
> This does not do what you want on !CONFIG_ZONE_DMA32 (ie arm LPAE).

How come? I would expect GFP_DMA32 allocates a buffer below 4 GiB in any 
case.

[...]

Re: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space

[Lorenzo Pieralisi]

On Sat, Dec 12, 2020 at 08:13:54PM +0100, Marek Vasut wrote:
> On 12/10/20 7:11 PM, Lorenzo Pieralisi wrote:
> 
> [...]
> 
> > > diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c
> > > index 1194d5f3341b..ac5c7d7573a6 100644
> > > --- a/drivers/pci/controller/pcie-rcar-host.c
> > > +++ b/drivers/pci/controller/pcie-rcar-host.c
> > > @@ -753,7 +753,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie_host *host)
> > >   	}
> > >   	/* setup MSI data target */
> > > -	msi->pages = __get_free_pages(GFP_KERNEL, 0);
> > > +	msi->pages = __get_free_pages(GFP_KERNEL | GFP_DMA32, 0);
> > 
> > This does not do what you want on !CONFIG_ZONE_DMA32 (ie arm LPAE).
> 
> How come? I would expect GFP_DMA32 allocates a buffer below 4 GiB in any
> case.

For ARM LPAE allocation falls back to ZONE_NORMAL that happens to work
because if there is memory > 4GB it ends up in ZONE_HIGHMEM, so this
patch should still work on ARM LPAE too.

Regardless, thoughts above the alternative approach (that saves you
a page allocation) ?

Lorenzo

Re: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space

[Marek Vasut]

On 12/14/20 5:08 PM, Lorenzo Pieralisi wrote:
> On Sat, Dec 12, 2020 at 08:13:54PM +0100, Marek Vasut wrote:
>> On 12/10/20 7:11 PM, Lorenzo Pieralisi wrote:
>>
>> [...]
>>
>>>> diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c
>>>> index 1194d5f3341b..ac5c7d7573a6 100644
>>>> --- a/drivers/pci/controller/pcie-rcar-host.c
>>>> +++ b/drivers/pci/controller/pcie-rcar-host.c
>>>> @@ -753,7 +753,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie_host *host)
>>>>    	}
>>>>    	/* setup MSI data target */
>>>> -	msi->pages = __get_free_pages(GFP_KERNEL, 0);
>>>> +	msi->pages = __get_free_pages(GFP_KERNEL | GFP_DMA32, 0);
>>>
>>> This does not do what you want on !CONFIG_ZONE_DMA32 (ie arm LPAE).
>>
>> How come? I would expect GFP_DMA32 allocates a buffer below 4 GiB in any
>> case.
> 
> For ARM LPAE allocation falls back to ZONE_NORMAL that happens to work
> because if there is memory > 4GB it ends up in ZONE_HIGHMEM, so this
> patch should still work on ARM LPAE too.
> 
> Regardless, thoughts above the alternative approach (that saves you
> a page allocation) ?

Since this is a bugfix, I would prefer it to be minimal.

Also, in case there was some yet undiscovered hardware bug which would 
let the MSI write through, having unused memory as the MSI destination 
address would only lead to write into that memory -- instead of a write 
into some other address.

Changing this to some hard-coded address (any suggestions?) can be a 
subsequent patch.

Re: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space

[Lorenzo Pieralisi]

On Wed, Dec 16, 2020 at 06:49:54PM +0100, Marek Vasut wrote:
> On 12/14/20 5:08 PM, Lorenzo Pieralisi wrote:
> > On Sat, Dec 12, 2020 at 08:13:54PM +0100, Marek Vasut wrote:
> > > On 12/10/20 7:11 PM, Lorenzo Pieralisi wrote:
> > > 
> > > [...]
> > > 
> > > > > diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c
> > > > > index 1194d5f3341b..ac5c7d7573a6 100644
> > > > > --- a/drivers/pci/controller/pcie-rcar-host.c
> > > > > +++ b/drivers/pci/controller/pcie-rcar-host.c
> > > > > @@ -753,7 +753,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie_host *host)
> > > > >    	}
> > > > >    	/* setup MSI data target */
> > > > > -	msi->pages = __get_free_pages(GFP_KERNEL, 0);
> > > > > +	msi->pages = __get_free_pages(GFP_KERNEL | GFP_DMA32, 0);
> > > > 
> > > > This does not do what you want on !CONFIG_ZONE_DMA32 (ie arm LPAE).
> > > 
> > > How come? I would expect GFP_DMA32 allocates a buffer below 4 GiB in any
> > > case.
> > 
> > For ARM LPAE allocation falls back to ZONE_NORMAL that happens to work
> > because if there is memory > 4GB it ends up in ZONE_HIGHMEM, so this
> > patch should still work on ARM LPAE too.
> > 
> > Regardless, thoughts above the alternative approach (that saves you
> > a page allocation) ?
> 
> Since this is a bugfix, I would prefer it to be minimal.

Yes, I agree with you on that.

> Also, in case there was some yet undiscovered hardware bug which would
> let the MSI write through, having unused memory as the MSI destination
> address would only lead to write into that memory -- instead of a
> write into some other address.
> 
> Changing this to some hard-coded address (any suggestions?) can be a
> subsequent patch.

The idea was taking the address from the host controller inbound window
(ie an address outside the dma-ranges ~(dma-ranges) and < 4GB), it
should not matter which one. I agree though that this can be a
subsequent patch even though usually we send for -rc* only fixes for
patches that hit the previous merge window - this seems a quite
longstanding (I traced it back to v3.16) one so it would wait till
v5.12, there is time to refactor it.

Thanks,
Lorenzo

Re: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space

[Marek Vasut]

On 12/21/20 11:01 AM, Lorenzo Pieralisi wrote:

[...]

>>>>>> --- a/drivers/pci/controller/pcie-rcar-host.c
>>>>>> +++ b/drivers/pci/controller/pcie-rcar-host.c
>>>>>> @@ -753,7 +753,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie_host *host)
>>>>>>     	}
>>>>>>     	/* setup MSI data target */
>>>>>> -	msi->pages = __get_free_pages(GFP_KERNEL, 0);
>>>>>> +	msi->pages = __get_free_pages(GFP_KERNEL | GFP_DMA32, 0);
>>>>>
>>>>> This does not do what you want on !CONFIG_ZONE_DMA32 (ie arm LPAE).
>>>>
>>>> How come? I would expect GFP_DMA32 allocates a buffer below 4 GiB in any
>>>> case.
>>>
>>> For ARM LPAE allocation falls back to ZONE_NORMAL that happens to work
>>> because if there is memory > 4GB it ends up in ZONE_HIGHMEM, so this
>>> patch should still work on ARM LPAE too.
>>>
>>> Regardless, thoughts above the alternative approach (that saves you
>>> a page allocation) ?
>>
>> Since this is a bugfix, I would prefer it to be minimal.
> 
> Yes, I agree with you on that.

Then maybe it makes sense to apply this bugfix so others can benefit 
from it too ?

>> Also, in case there was some yet undiscovered hardware bug which would
>> let the MSI write through, having unused memory as the MSI destination
>> address would only lead to write into that memory -- instead of a
>> write into some other address.
>>
>> Changing this to some hard-coded address (any suggestions?) can be a
>> subsequent patch.
> 
> The idea was taking the address from the host controller inbound window
> (ie an address outside the dma-ranges ~(dma-ranges) and < 4GB), it
> should not matter which one.

Wouldn't that make the code quite unnecessarily complex for no gain ?
The above fix does just that in one line, unless there is some code in 
the PCI subsystem to select such an address already ?

> I agree though that this can be a
> subsequent patch even though usually we send for -rc* only fixes for
> patches that hit the previous merge window - this seems a quite
> longstanding (I traced it back to v3.16) one so it would wait till
> v5.12, there is time to refactor it.

I see, I was not aware of this policy toward bugfixes.

Re: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space

[Lorenzo Pieralisi]

On Wed, Dec 30, 2020 at 01:47:25PM +0100, Marek Vasut wrote:
> On 12/21/20 11:01 AM, Lorenzo Pieralisi wrote:
> 
> [...]
> 
> > > > > > > --- a/drivers/pci/controller/pcie-rcar-host.c
> > > > > > > +++ b/drivers/pci/controller/pcie-rcar-host.c
> > > > > > > @@ -753,7 +753,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie_host *host)
> > > > > > >     	}
> > > > > > >     	/* setup MSI data target */
> > > > > > > -	msi->pages = __get_free_pages(GFP_KERNEL, 0);
> > > > > > > +	msi->pages = __get_free_pages(GFP_KERNEL | GFP_DMA32, 0);
> > > > > > 
> > > > > > This does not do what you want on !CONFIG_ZONE_DMA32 (ie arm LPAE).
> > > > > 
> > > > > How come? I would expect GFP_DMA32 allocates a buffer below 4 GiB in any
> > > > > case.
> > > > 
> > > > For ARM LPAE allocation falls back to ZONE_NORMAL that happens to work
> > > > because if there is memory > 4GB it ends up in ZONE_HIGHMEM, so this
> > > > patch should still work on ARM LPAE too.
> > > > 
> > > > Regardless, thoughts above the alternative approach (that saves you
> > > > a page allocation) ?
> > > 
> > > Since this is a bugfix, I would prefer it to be minimal.
> > 
> > Yes, I agree with you on that.
> 
> Then maybe it makes sense to apply this bugfix so others can benefit from it
> too ?

I will apply it shortly, thanks.

> > > Also, in case there was some yet undiscovered hardware bug which would
> > > let the MSI write through, having unused memory as the MSI destination
> > > address would only lead to write into that memory -- instead of a
> > > write into some other address.
> > > 
> > > Changing this to some hard-coded address (any suggestions?) can be a
> > > subsequent patch.
> > 
> > The idea was taking the address from the host controller inbound window
> > (ie an address outside the dma-ranges ~(dma-ranges) and < 4GB), it
> > should not matter which one.
> 
> Wouldn't that make the code quite unnecessarily complex for no gain ?

Well, there is a gain, current code is allocating a page of memory -
there is no need to do that and I don't think that what I am asking is
complex.

Again, I will merge this patch but please have a look to check if what I
ask above is a possibility.

Thanks,
Lorenzo

> The above fix does just that in one line, unless there is some code in the
> PCI subsystem to select such an address already ?
> 
> > I agree though that this can be a
> > subsequent patch even though usually we send for -rc* only fixes for
> > patches that hit the previous merge window - this seems a quite
> > longstanding (I traced it back to v3.16) one so it would wait till
> > v5.12, there is time to refactor it.
> 
> I see, I was not aware of this policy toward bugfixes.

Re: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space

[Lorenzo Pieralisi]

On Fri, 16 Oct 2020 14:04:31 +0200, marek.vasut@gmail.com wrote:
> This fixes MSI operation on legacy PCI cards, which cannot issue 64bit MSIs.
> The R-Car controller only has one MSI trigger address instead of two, one
> for 64bit and one for 32bit MSI, set the address to 32bit PCIe space so that
> legacy PCI cards can also trigger MSIs.

Applied to pci/rcar, thanks!

[1/1] PCI: rcar: Always allocate MSI addresses in 32bit space
      https://git.kernel.org/lpieralisi/pci/c/c4e0fec2f7

Thanks,
Lorenzo