| Message ID | 20201016120431.7062-1-marek.vasut@gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: rcar: Always allocate MSI addresses in 32bit space | expand |
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
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. [...]
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
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 >
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. [...]
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
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.
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
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.
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.
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
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;