| Message ID | 20241007041218.157516-5-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,01/12] PCI: rockchip-ep: Fix address translation unit programming | expand |
On Mon, Oct 07, 2024 at 01:12:10PM +0900, Damien Le Moal wrote: > Add a check to verify that the outbound region to be used for mapping an > address is not already in use. > > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > drivers/pci/controller/pcie-rockchip-ep.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c > index 89ebdf3e4737..edb84fb1ba39 100644 > --- a/drivers/pci/controller/pcie-rockchip-ep.c > +++ b/drivers/pci/controller/pcie-rockchip-ep.c > @@ -243,6 +243,9 @@ static int rockchip_pcie_ep_map_addr(struct pci_epc *epc, u8 fn, u8 vfn, > struct rockchip_pcie *pcie = &ep->rockchip; > u32 r = rockchip_ob_region(addr); > > + if (test_bit(r, &ep->ob_region_map)) > + return -EBUSY; > + > rockchip_pcie_prog_ep_ob_atu(pcie, fn, r, addr, pci_addr, size); > > set_bit(r, &ep->ob_region_map); > -- > 2.46.2 >
On Thu, Oct 10, 2024 at 12:43:57PM +0530, Manivannan Sadhasivam wrote: > On Mon, Oct 07, 2024 at 01:12:10PM +0900, Damien Le Moal wrote: > > Add a check to verify that the outbound region to be used for mapping an > > address is not already in use. > > > > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > I'm trying to understand the ob window mapping here. So if rockchip_ob_region() returns same index for different *CPU* addresses, then you cannot map both of them? Is this a hardware ATU limitation? Also rockchip_pcie_prog_ep_ob_atu() is not taking into account of the cpu_addr. So I'm not sure how the mapping happens either. - Mani > - Mani > > > --- > > drivers/pci/controller/pcie-rockchip-ep.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c > > index 89ebdf3e4737..edb84fb1ba39 100644 > > --- a/drivers/pci/controller/pcie-rockchip-ep.c > > +++ b/drivers/pci/controller/pcie-rockchip-ep.c > > @@ -243,6 +243,9 @@ static int rockchip_pcie_ep_map_addr(struct pci_epc *epc, u8 fn, u8 vfn, > > struct rockchip_pcie *pcie = &ep->rockchip; > > u32 r = rockchip_ob_region(addr); > > > > + if (test_bit(r, &ep->ob_region_map)) > > + return -EBUSY; > > + > > rockchip_pcie_prog_ep_ob_atu(pcie, fn, r, addr, pci_addr, size); > > > > set_bit(r, &ep->ob_region_map); > > -- > > 2.46.2 > > > > -- > மணிவண்ணன் சதாசிவம்
On 10/12/24 18:31, Manivannan Sadhasivam wrote: > On Thu, Oct 10, 2024 at 12:43:57PM +0530, Manivannan Sadhasivam wrote: >> On Mon, Oct 07, 2024 at 01:12:10PM +0900, Damien Le Moal wrote: >>> Add a check to verify that the outbound region to be used for mapping an >>> address is not already in use. >>> >>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >> >> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> >> > > I'm trying to understand the ob window mapping here. So if rockchip_ob_region() > returns same index for different *CPU* addresses, then you cannot map both of > them? Is this a hardware ATU limitation? The CPU addresses mapped are (under normal use) addresses from one of the 32 1MB memory windows that pci_epc_alloc_addr() will give us. To map a memory window address, we use the ATU entry at the index given by: static inline u32 rockchip_ob_region(phys_addr_t addr) { return (addr >> ilog2(SZ_1M)) & 0x1f; } So each memory window always use the same ATU entry and addresses from different windows cannot use the same entry, ever. But if there is a bug in the endpoint driver and map() or unmap() are called with an invalid address (e.g. one that is not from a memory window), we will still get a valid ATU entry number for that address. Hence the check that the address passed to unmap is the one that is actually mapped, and also why we check that the entry for an address to map is not being used. > Also rockchip_pcie_prog_ep_ob_atu() is not taking into account of the cpu_addr. > So I'm not sure how the mapping happens either. Each ATU entry is for the corresponding memory window at the same index in the controller memory. So the cpu address is not needed when programming the ATU as it is implied from the entry we are programming. So we could remove the cpu_addr argument of this function. I also suspect that we potentially could play games with the .align_addr() to assume a fixed 1MB alignment constraint for a PCI address mapping and always pass 20 to the num_bits field of the ADDR0 register. However, I have not tried that.
On Sat, Oct 12, 2024 at 09:02:43PM +0900, Damien Le Moal wrote: > On 10/12/24 18:31, Manivannan Sadhasivam wrote: > > On Thu, Oct 10, 2024 at 12:43:57PM +0530, Manivannan Sadhasivam wrote: > >> On Mon, Oct 07, 2024 at 01:12:10PM +0900, Damien Le Moal wrote: > >>> Add a check to verify that the outbound region to be used for mapping an > >>> address is not already in use. > >>> > >>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > >> > >> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > >> > > > > I'm trying to understand the ob window mapping here. So if rockchip_ob_region() > > returns same index for different *CPU* addresses, then you cannot map both of > > them? Is this a hardware ATU limitation? > > The CPU addresses mapped are (under normal use) addresses from one of the 32 1MB > memory windows that pci_epc_alloc_addr() will give us. To map a memory window > address, we use the ATU entry at the index given by: > > static inline u32 rockchip_ob_region(phys_addr_t addr) > { > return (addr >> ilog2(SZ_1M)) & 0x1f; > } > > So each memory window always use the same ATU entry and addresses from different > windows cannot use the same entry, ever. > > But if there is a bug in the endpoint driver and map() or unmap() are called > with an invalid address (e.g. one that is not from a memory window), we will > still get a valid ATU entry number for that address. Hence the check that the > address passed to unmap is the one that is actually mapped, and also why we > check that the entry for an address to map is not being used. > > > Also rockchip_pcie_prog_ep_ob_atu() is not taking into account of the cpu_addr. > > So I'm not sure how the mapping happens either. > > Each ATU entry is for the corresponding memory window at the same index in the > controller memory. So the cpu address is not needed when programming the ATU as > it is implied from the entry we are programming. > Ah okay, thanks a lot for the explanation. > So we could remove the cpu_addr argument of this function. > yeah, and may be a comment would also help. > I also suspect that we potentially could play games with the .align_addr() to > assume a fixed 1MB alignment constraint for a PCI address mapping and always > pass 20 to the num_bits field of the ADDR0 register. However, I have not tried that. > Ok! - Mani
diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c index 89ebdf3e4737..edb84fb1ba39 100644 --- a/drivers/pci/controller/pcie-rockchip-ep.c +++ b/drivers/pci/controller/pcie-rockchip-ep.c @@ -243,6 +243,9 @@ static int rockchip_pcie_ep_map_addr(struct pci_epc *epc, u8 fn, u8 vfn, struct rockchip_pcie *pcie = &ep->rockchip; u32 r = rockchip_ob_region(addr); + if (test_bit(r, &ep->ob_region_map)) + return -EBUSY; + rockchip_pcie_prog_ep_ob_atu(pcie, fn, r, addr, pci_addr, size); set_bit(r, &ep->ob_region_map);
Add a check to verify that the outbound region to be used for mapping an address is not already in use. Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/pci/controller/pcie-rockchip-ep.c | 3 +++ 1 file changed, 3 insertions(+)