| Message ID | f6b445b764312fd8ab96745fe4e97fb22f91ae4c.1730123575.git.andrea.porta@suse.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for RaspberryPi RP1 PCI device using a DT overlay | expand |
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 >
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 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
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)) {
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(-)