linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space
@ 2020-10-16 12:04 marek.vasut
  2020-10-20  7:47 ` Geert Uytterhoeven
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: marek.vasut @ 2020-10-16 12:04 UTC (permalink / raw)
  To: linux-pci
  Cc: Marek Vasut, Bjorn Helgaas, Geert Uytterhoeven,
	Lorenzo Pieralisi, Wolfram Sang, Yoshihiro Shimoda,
	linux-renesas-soc

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space
  2020-10-16 12:04 [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space marek.vasut
@ 2020-10-20  7:47 ` Geert Uytterhoeven
  2020-10-25 15:37   ` Marek Vasut
  2020-10-27 12:04 ` Yoshihiro Shimoda
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Geert Uytterhoeven @ 2020-10-20  7:47 UTC (permalink / raw)
  To: Marek Vasut
  Cc: linux-pci, Marek Vasut, Bjorn Helgaas, Lorenzo Pieralisi,
	Wolfram Sang, Yoshihiro Shimoda, Linux-Renesas

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space
  2020-10-20  7:47 ` Geert Uytterhoeven
@ 2020-10-25 15:37   ` Marek Vasut
  0 siblings, 0 replies; 12+ messages in thread
From: Marek Vasut @ 2020-10-25 15:37 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: linux-pci, Bjorn Helgaas, Lorenzo Pieralisi, Wolfram Sang,
	Yoshihiro Shimoda, Linux-Renesas

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.

[...]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space
  2020-10-16 12:04 [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space marek.vasut
  2020-10-20  7:47 ` Geert Uytterhoeven
@ 2020-10-27 12:04 ` Yoshihiro Shimoda
  2020-12-10 18:11 ` Lorenzo Pieralisi
  2021-01-15 12:12 ` Lorenzo Pieralisi
  3 siblings, 0 replies; 12+ messages in thread
From: Yoshihiro Shimoda @ 2020-10-27 12:04 UTC (permalink / raw)
  To: marek.vasut, linux-pci
  Cc: Marek Vasut, Bjorn Helgaas, Geert Uytterhoeven,
	Lorenzo Pieralisi, Wolfram Sang, linux-renesas-soc

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space
  2020-10-16 12:04 [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space marek.vasut
  2020-10-20  7:47 ` Geert Uytterhoeven
  2020-10-27 12:04 ` Yoshihiro Shimoda
@ 2020-12-10 18:11 ` Lorenzo Pieralisi
  2020-12-12 19:13   ` Marek Vasut
  2021-01-15 12:12 ` Lorenzo Pieralisi
  3 siblings, 1 reply; 12+ messages in thread
From: Lorenzo Pieralisi @ 2020-12-10 18:11 UTC (permalink / raw)
  To: marek.vasut
  Cc: linux-pci, Marek Vasut, Bjorn Helgaas, Geert Uytterhoeven,
	Wolfram Sang, Yoshihiro Shimoda, linux-renesas-soc

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
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space
  2020-12-10 18:11 ` Lorenzo Pieralisi
@ 2020-12-12 19:13   ` Marek Vasut
  2020-12-14 16:08     ` Lorenzo Pieralisi
  0 siblings, 1 reply; 12+ messages in thread
From: Marek Vasut @ 2020-12-12 19:13 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: linux-pci, Bjorn Helgaas, Geert Uytterhoeven, Wolfram Sang,
	Yoshihiro Shimoda, linux-renesas-soc

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.

[...]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space
  2020-12-12 19:13   ` Marek Vasut
@ 2020-12-14 16:08     ` Lorenzo Pieralisi
  2020-12-16 17:49       ` Marek Vasut
  0 siblings, 1 reply; 12+ messages in thread
From: Lorenzo Pieralisi @ 2020-12-14 16:08 UTC (permalink / raw)
  To: Marek Vasut
  Cc: linux-pci, Bjorn Helgaas, Geert Uytterhoeven, Wolfram Sang,
	Yoshihiro Shimoda, linux-renesas-soc

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space
  2020-12-14 16:08     ` Lorenzo Pieralisi
@ 2020-12-16 17:49       ` Marek Vasut
  2020-12-21 10:01         ` Lorenzo Pieralisi
  0 siblings, 1 reply; 12+ messages in thread
From: Marek Vasut @ 2020-12-16 17:49 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: linux-pci, Bjorn Helgaas, Geert Uytterhoeven, Wolfram Sang,
	Yoshihiro Shimoda, linux-renesas-soc

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space
  2020-12-16 17:49       ` Marek Vasut
@ 2020-12-21 10:01         ` Lorenzo Pieralisi
  2020-12-30 12:47           ` Marek Vasut
  0 siblings, 1 reply; 12+ messages in thread
From: Lorenzo Pieralisi @ 2020-12-21 10:01 UTC (permalink / raw)
  To: Marek Vasut
  Cc: linux-pci, Bjorn Helgaas, Geert Uytterhoeven, Wolfram Sang,
	Yoshihiro Shimoda, linux-renesas-soc

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space
  2020-12-21 10:01         ` Lorenzo Pieralisi
@ 2020-12-30 12:47           ` Marek Vasut
  2021-01-04 12:21             ` Lorenzo Pieralisi
  0 siblings, 1 reply; 12+ messages in thread
From: Marek Vasut @ 2020-12-30 12:47 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: linux-pci, Bjorn Helgaas, Geert Uytterhoeven, Wolfram Sang,
	Yoshihiro Shimoda, linux-renesas-soc

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space
  2020-12-30 12:47           ` Marek Vasut
@ 2021-01-04 12:21             ` Lorenzo Pieralisi
  0 siblings, 0 replies; 12+ messages in thread
From: Lorenzo Pieralisi @ 2021-01-04 12:21 UTC (permalink / raw)
  To: Marek Vasut
  Cc: linux-pci, Bjorn Helgaas, Geert Uytterhoeven, Wolfram Sang,
	Yoshihiro Shimoda, linux-renesas-soc

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space
  2020-10-16 12:04 [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space marek.vasut
                   ` (2 preceding siblings ...)
  2020-12-10 18:11 ` Lorenzo Pieralisi
@ 2021-01-15 12:12 ` Lorenzo Pieralisi
  3 siblings, 0 replies; 12+ messages in thread
From: Lorenzo Pieralisi @ 2021-01-15 12:12 UTC (permalink / raw)
  To: linux-pci, marek.vasut
  Cc: Lorenzo Pieralisi, linux-renesas-soc, Marek Vasut, Bjorn Helgaas,
	Wolfram Sang, Geert Uytterhoeven, Yoshihiro Shimoda

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-01-15 12:14 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-16 12:04 [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space marek.vasut
2020-10-20  7:47 ` Geert Uytterhoeven
2020-10-25 15:37   ` Marek Vasut
2020-10-27 12:04 ` Yoshihiro Shimoda
2020-12-10 18:11 ` Lorenzo Pieralisi
2020-12-12 19:13   ` Marek Vasut
2020-12-14 16:08     ` Lorenzo Pieralisi
2020-12-16 17:49       ` Marek Vasut
2020-12-21 10:01         ` Lorenzo Pieralisi
2020-12-30 12:47           ` Marek Vasut
2021-01-04 12:21             ` Lorenzo Pieralisi
2021-01-15 12:12 ` Lorenzo Pieralisi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).