When populating "ranges" property for a PCI bridge, of_pci_prop_ranges()
incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI
bridge, the window should instead be in PCI address space. Call
pci_bus_address() on the resource in order to obtain the PCI bus
address.
Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
---
drivers/pci/of_property.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
index 5a0b98e69795..886c236e5de6 100644
--- a/drivers/pci/of_property.c
+++ b/drivers/pci/of_property.c
@@ -126,7 +126,7 @@ static int of_pci_prop_ranges(struct pci_dev *pdev, struct of_changeset *ocs,
if (of_pci_get_addr_flags(&res[j], &flags))
continue;
- val64 = res[j].start;
+ val64 = pci_bus_address(pdev, &res[j] - pdev->resource);
of_pci_set_address(pdev, rp[i].parent_addr, val64, 0, flags,
false);
if (pci_is_bridge(pdev)) {
--
2.35.3Hi Andrea, On Mon, 28 Oct 2024 15:07:22 +0100 Andrea della Porta <andrea.porta@suse.com> wrote: > When populating "ranges" property for a PCI bridge, of_pci_prop_ranges() > incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI > bridge, the window should instead be in PCI address space. Call > pci_bus_address() on the resource in order to obtain the PCI bus > address. > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> Tested ok with my LAN966x PCI device. Tested-by: Herve Codina <herve.codina@bootlin.com> Best regards, Hervé
Hi Herve, On 09:06 Mon 04 Nov , Herve Codina wrote: > Hi Andrea, > > On Mon, 28 Oct 2024 15:07:22 +0100 > Andrea della Porta <andrea.porta@suse.com> wrote: > > > When populating "ranges" property for a PCI bridge, of_pci_prop_ranges() > > incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI > > bridge, the window should instead be in PCI address space. Call > > pci_bus_address() on the resource in order to obtain the PCI bus > > address. > > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com> > > Tested ok with my LAN966x PCI device. > > Tested-by: Herve Codina <herve.codina@bootlin.com> Thanks for testing that! Regards, Andrea > > Best regards, > Hervé
On Mon, Oct 28, 2024 at 03:07:22PM +0100, Andrea della Porta wrote:
> When populating "ranges" property for a PCI bridge, of_pci_prop_ranges()
> incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI
> bridge, the window should instead be in PCI address space. Call
> pci_bus_address() on the resource in order to obtain the PCI bus
> address.
>
of_pci_prop_ranges() could be called for PCI devices also (not just PCI
bridges), right?
- Mani
> Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> ---
> drivers/pci/of_property.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
> index 5a0b98e69795..886c236e5de6 100644
> --- a/drivers/pci/of_property.c
> +++ b/drivers/pci/of_property.c
> @@ -126,7 +126,7 @@ static int of_pci_prop_ranges(struct pci_dev *pdev, struct of_changeset *ocs,
> if (of_pci_get_addr_flags(&res[j], &flags))
> continue;
>
> - val64 = res[j].start;
> + val64 = pci_bus_address(pdev, &res[j] - pdev->resource);
> of_pci_set_address(pdev, rp[i].parent_addr, val64, 0, flags,
> false);
> if (pci_is_bridge(pdev)) {
> --
> 2.35.3
>
--
மணிவண்ணன் சதாசிவம்
Hi Manivannan,
On 22:39 Sat 02 Nov , Manivannan Sadhasivam wrote:
> On Mon, Oct 28, 2024 at 03:07:22PM +0100, Andrea della Porta wrote:
> > When populating "ranges" property for a PCI bridge, of_pci_prop_ranges()
> > incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI
> > bridge, the window should instead be in PCI address space. Call
> > pci_bus_address() on the resource in order to obtain the PCI bus
> > address.
> >
>
> of_pci_prop_ranges() could be called for PCI devices also (not just PCI
> bridges), right?
Correct. Please note however that while the PCI-PCI bridge has the parent
address in CPU space, an endpoint device has it in PCI space: here we're
focusing on the bridge part. It probably used to work before since in many
cases the CPU and PCI address are the same, but it breaks down when they
differ.
Many thanks,
Andrea
>
> - Mani
>
> > Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> > ---
> > drivers/pci/of_property.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
> > index 5a0b98e69795..886c236e5de6 100644
> > --- a/drivers/pci/of_property.c
> > +++ b/drivers/pci/of_property.c
> > @@ -126,7 +126,7 @@ static int of_pci_prop_ranges(struct pci_dev *pdev, struct of_changeset *ocs,
> > if (of_pci_get_addr_flags(&res[j], &flags))
> > continue;
> >
> > - val64 = res[j].start;
> > + val64 = pci_bus_address(pdev, &res[j] - pdev->resource);
> > of_pci_set_address(pdev, rp[i].parent_addr, val64, 0, flags,
> > false);
> > if (pci_is_bridge(pdev)) {
> > --
> > 2.35.3
> >
>
> --
> மணிவண்ணன் சதாசிவம்
On Mon, Nov 04, 2024 at 09:54:57AM +0100, Andrea della Porta wrote: > Hi Manivannan, > > On 22:39 Sat 02 Nov , Manivannan Sadhasivam wrote: > > On Mon, Oct 28, 2024 at 03:07:22PM +0100, Andrea della Porta wrote: > > > When populating "ranges" property for a PCI bridge, of_pci_prop_ranges() > > > incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI > > > bridge, the window should instead be in PCI address space. Call > > > pci_bus_address() on the resource in order to obtain the PCI bus > > > address. > > > > > > > of_pci_prop_ranges() could be called for PCI devices also (not just PCI > > bridges), right? > > Correct. Please note however that while the PCI-PCI bridge has the parent > address in CPU space, an endpoint device has it in PCI space: here we're > focusing on the bridge part. It probably used to work before since in many > cases the CPU and PCI address are the same, but it breaks down when they > differ. > When you say 'focusing', you are specifically referring to the bridge part of this API I believe. But I don't see a check for the bridge in your change, which is what concerning me. Am I missing something? - Mani -- மணிவண்ணன் சதாசிவம்
On Mon, Nov 04, 2024 at 08:35:21PM +0530, Manivannan Sadhasivam wrote: > On Mon, Nov 04, 2024 at 09:54:57AM +0100, Andrea della Porta wrote: > > On 22:39 Sat 02 Nov , Manivannan Sadhasivam wrote: > > > On Mon, Oct 28, 2024 at 03:07:22PM +0100, Andrea della Porta wrote: > > > > When populating "ranges" property for a PCI bridge, of_pci_prop_ranges() > > > > incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI > > > > bridge, the window should instead be in PCI address space. Call > > > > pci_bus_address() on the resource in order to obtain the PCI bus > > > > address. > > > > > > of_pci_prop_ranges() could be called for PCI devices also (not just PCI > > > bridges), right? > > > > Correct. Please note however that while the PCI-PCI bridge has the parent > > address in CPU space, an endpoint device has it in PCI space: here we're > > focusing on the bridge part. It probably used to work before since in many > > cases the CPU and PCI address are the same, but it breaks down when they > > differ. > > When you say 'focusing', you are specifically referring to the > bridge part of this API I believe. But I don't see a check for the > bridge in your change, which is what concerning me. Am I missing > something? I think we want this change for all devices in the PCI address domain, including PCI-PCI bridges and endpoints, don't we? All those "ranges" addresses should be in the PCI domain. Bjorn
On Mon, Nov 04, 2024 at 05:49:37PM -0600, Bjorn Helgaas wrote: > On Mon, Nov 04, 2024 at 08:35:21PM +0530, Manivannan Sadhasivam wrote: > > On Mon, Nov 04, 2024 at 09:54:57AM +0100, Andrea della Porta wrote: > > > On 22:39 Sat 02 Nov , Manivannan Sadhasivam wrote: > > > > On Mon, Oct 28, 2024 at 03:07:22PM +0100, Andrea della Porta wrote: > > > > > When populating "ranges" property for a PCI bridge, of_pci_prop_ranges() > > > > > incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI > > > > > bridge, the window should instead be in PCI address space. Call > > > > > pci_bus_address() on the resource in order to obtain the PCI bus > > > > > address. > > > > > > > > of_pci_prop_ranges() could be called for PCI devices also (not just PCI > > > > bridges), right? > > > > > > Correct. Please note however that while the PCI-PCI bridge has the parent > > > address in CPU space, an endpoint device has it in PCI space: here we're > > > focusing on the bridge part. It probably used to work before since in many > > > cases the CPU and PCI address are the same, but it breaks down when they > > > differ. > > > > When you say 'focusing', you are specifically referring to the > > bridge part of this API I believe. But I don't see a check for the > > bridge in your change, which is what concerning me. Am I missing > > something? > > I think we want this change for all devices in the PCI address > domain, including PCI-PCI bridges and endpoints, don't we? All those > "ranges" addresses should be in the PCI domain. > Yeah, right. I was slightly confused by the commit message. Maybe including a sentence about how the change will work fine for endpoint devices would help. Also, why it went unnoticed till now (ie., both CPU and PCI addresses are same in many SoCs). Also there should be a fixes tag (also CC stable) since this is a potential bug fix. - Mani -- மணிவண்ணன் சதாசிவம்
Hi Mani, On 11/6/24 16:35, Manivannan Sadhasivam wrote: > On Mon, Nov 04, 2024 at 05:49:37PM -0600, Bjorn Helgaas wrote: >> On Mon, Nov 04, 2024 at 08:35:21PM +0530, Manivannan Sadhasivam wrote: >>> On Mon, Nov 04, 2024 at 09:54:57AM +0100, Andrea della Porta wrote: >>>> On 22:39 Sat 02 Nov , Manivannan Sadhasivam wrote: >>>>> On Mon, Oct 28, 2024 at 03:07:22PM +0100, Andrea della Porta wrote: >>>>>> When populating "ranges" property for a PCI bridge, of_pci_prop_ranges() >>>>>> incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI >>>>>> bridge, the window should instead be in PCI address space. Call >>>>>> pci_bus_address() on the resource in order to obtain the PCI bus >>>>>> address. >>>>> >>>>> of_pci_prop_ranges() could be called for PCI devices also (not just PCI >>>>> bridges), right? >>>> >>>> Correct. Please note however that while the PCI-PCI bridge has the parent >>>> address in CPU space, an endpoint device has it in PCI space: here we're >>>> focusing on the bridge part. It probably used to work before since in many >>>> cases the CPU and PCI address are the same, but it breaks down when they >>>> differ. >>> >>> When you say 'focusing', you are specifically referring to the >>> bridge part of this API I believe. But I don't see a check for the >>> bridge in your change, which is what concerning me. Am I missing >>> something? >> >> I think we want this change for all devices in the PCI address >> domain, including PCI-PCI bridges and endpoints, don't we? All those >> "ranges" addresses should be in the PCI domain. >> > > Yeah, right. I was slightly confused by the commit message. Maybe including a > sentence about how the change will work fine for endpoint devices would help. > Also, why it went unnoticed till now (ie., both CPU and PCI addresses are same > in many SoCs). Most probably it is unnoticed because until now nobody has enabled /selected CONFIG_PCI_DYNAMIC_OF_NODES ? ~Stan
Hi Manivannan, On 14:35 Wed 06 Nov , Manivannan Sadhasivam wrote: > On Mon, Nov 04, 2024 at 05:49:37PM -0600, Bjorn Helgaas wrote: > > On Mon, Nov 04, 2024 at 08:35:21PM +0530, Manivannan Sadhasivam wrote: > > > On Mon, Nov 04, 2024 at 09:54:57AM +0100, Andrea della Porta wrote: > > > > On 22:39 Sat 02 Nov , Manivannan Sadhasivam wrote: > > > > > On Mon, Oct 28, 2024 at 03:07:22PM +0100, Andrea della Porta wrote: > > > > > > When populating "ranges" property for a PCI bridge, of_pci_prop_ranges() > > > > > > incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI > > > > > > bridge, the window should instead be in PCI address space. Call > > > > > > pci_bus_address() on the resource in order to obtain the PCI bus > > > > > > address. > > > > > > > > > > of_pci_prop_ranges() could be called for PCI devices also (not just PCI > > > > > bridges), right? > > > > > > > > Correct. Please note however that while the PCI-PCI bridge has the parent > > > > address in CPU space, an endpoint device has it in PCI space: here we're > > > > focusing on the bridge part. It probably used to work before since in many > > > > cases the CPU and PCI address are the same, but it breaks down when they > > > > differ. > > > > > > When you say 'focusing', you are specifically referring to the > > > bridge part of this API I believe. But I don't see a check for the > > > bridge in your change, which is what concerning me. Am I missing > > > something? > > > > I think we want this change for all devices in the PCI address > > domain, including PCI-PCI bridges and endpoints, don't we? All those > > "ranges" addresses should be in the PCI domain. > > > > Yeah, right. I was slightly confused by the commit message. Maybe including a > sentence about how the change will work fine for endpoint devices would help. > Also, why it went unnoticed till now (ie., both CPU and PCI addresses are same > in many SoCs). Sorry for the (admittedly) confusing explanation from my side. What I would have really liked to convey is that although the root complex (that is itself a bridge) is the ultimate 'translator' between CPU and PCI addresses, all the other entities are of course under PCI address space. In fact, any resource submitted to of_pci_set_address() is intended to be a PCI bus address, and this is valid for both sub-bridges and EPs. > > Also there should be a fixes tag (also CC stable) since this is a potential bug > fix. Sure. I think it could be better to resend this specific patch (and maybe also the patch "of: address: Preserve the flags portion on 1:1 dma-ranges mapping", which is also a kind of bugfix) as standalone ones instead of prerequisites for the RP1 patchset, if it's not a concern to anyone... Regards, Andrea > > - Mani > > -- > மணிவண்ணன் சதாசிவம்
On Thu, Nov 07, 2024 at 10:06:53AM +0100, Andrea della Porta wrote: > Hi Manivannan, > > On 14:35 Wed 06 Nov , Manivannan Sadhasivam wrote: > > On Mon, Nov 04, 2024 at 05:49:37PM -0600, Bjorn Helgaas wrote: > > > On Mon, Nov 04, 2024 at 08:35:21PM +0530, Manivannan Sadhasivam wrote: > > > > On Mon, Nov 04, 2024 at 09:54:57AM +0100, Andrea della Porta wrote: > > > > > On 22:39 Sat 02 Nov , Manivannan Sadhasivam wrote: > > > > > > On Mon, Oct 28, 2024 at 03:07:22PM +0100, Andrea della Porta wrote: > > > > > > > When populating "ranges" property for a PCI bridge, of_pci_prop_ranges() > > > > > > > incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI > > > > > > > bridge, the window should instead be in PCI address space. Call > > > > > > > pci_bus_address() on the resource in order to obtain the PCI bus > > > > > > > address. > > > > > > > > > > > > of_pci_prop_ranges() could be called for PCI devices also (not just PCI > > > > > > bridges), right? > > > > > > > > > > Correct. Please note however that while the PCI-PCI bridge has the parent > > > > > address in CPU space, an endpoint device has it in PCI space: here we're > > > > > focusing on the bridge part. It probably used to work before since in many > > > > > cases the CPU and PCI address are the same, but it breaks down when they > > > > > differ. > > > > > > > > When you say 'focusing', you are specifically referring to the > > > > bridge part of this API I believe. But I don't see a check for the > > > > bridge in your change, which is what concerning me. Am I missing > > > > something? > > > > > > I think we want this change for all devices in the PCI address > > > domain, including PCI-PCI bridges and endpoints, don't we? All those > > > "ranges" addresses should be in the PCI domain. > > > > > > > Yeah, right. I was slightly confused by the commit message. Maybe including a > > sentence about how the change will work fine for endpoint devices would help. > > Also, why it went unnoticed till now (ie., both CPU and PCI addresses are same > > in many SoCs). > > Sorry for the (admittedly) confusing explanation from my side. What I would > have really liked to convey is that although the root complex (that is itself > a bridge) is the ultimate 'translator' between CPU and PCI addresses, all the > other entities are of course under PCI address space. In fact, any resource > submitted to of_pci_set_address() is intended to be a PCI bus address, > and this is valid for both sub-bridges and EPs. > Sounds good. We usually have empty ranges for PCI bridges (1:1 mapping), so that also lead to the confusion at my end. > > > > Also there should be a fixes tag (also CC stable) since this is a potential bug > > fix. > > Sure. I think it could be better to resend this specific patch (and maybe also the > patch "of: address: Preserve the flags portion on 1:1 dma-ranges mapping", which > is also a kind of bugfix) as standalone ones instead of prerequisites for the RP1 > patchset, if it's not a concern to anyone... > In fact, it is recommended to send fixes separately (or at the start of the series). So there should be no concerns. - Mani -- மணிவண்ணன் சதாசிவம்
On Mon, Oct 28, 2024 at 03:07:22PM +0100, Andrea della Porta wrote:
> When populating "ranges" property for a PCI bridge, of_pci_prop_ranges()
> incorrectly use the CPU bus address of the resource. Since this is a PCI-PCI
> bridge, the window should instead be in PCI address space. Call
> pci_bus_address() on the resource in order to obtain the PCI bus
> address.
>
> Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Thanks for working through this!
> ---
> drivers/pci/of_property.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
> index 5a0b98e69795..886c236e5de6 100644
> --- a/drivers/pci/of_property.c
> +++ b/drivers/pci/of_property.c
> @@ -126,7 +126,7 @@ static int of_pci_prop_ranges(struct pci_dev *pdev, struct of_changeset *ocs,
> if (of_pci_get_addr_flags(&res[j], &flags))
> continue;
>
> - val64 = res[j].start;
> + val64 = pci_bus_address(pdev, &res[j] - pdev->resource);
> of_pci_set_address(pdev, rp[i].parent_addr, val64, 0, flags,
> false);
> if (pci_is_bridge(pdev)) {
> --
> 2.35.3
>
© 2016 - 2024 Red Hat, Inc.