| Message ID | 20241104205144.409236-2-cassel@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI: dwc: ep: Do not map more memory than needed to raise a MSI/MSI-X IRQ | expand |
Hello, > In dw_pcie_ep_init() we allocate memory from the EPC address space that we > will later use to raise a MSI/MSI-X IRQ. This memory is only freed in > dw_pcie_ep_deinit(). > > Performing this allocation in dw_pcie_ep_init() is to ensure that we will > not fail to allocate memory from the EPC address space when trying to raise > a MSI/MSI-X IRQ. > > We still map/unmap this memory every time we raise an IRQ, in order to not > constantly occupy an iATU, especially for controllers with few iATUs. > (So we can still fail to raise an MSI/MSI-X IRQ if all iATUs are occupied.) > > When allocating this memory in dw_pcie_ep_init(), we allocate > epc->mem->window.page_size memory, which is the smallest unit that we can > allocate from the EPC address space. > > However, when writing/sending the msg data, which is only 2 bytes for MSI, > 4 bytes for MSI-X, in either case a single writel() is sufficient. Thus, > the size that we need to map is a single PCI DWORD (4 bytes). > > This is the size that we should send in to the pci_epc_ops::align_addr() > API. It is align_addr()'s responsibility to return a size that is aligned > to the EPC page size, for platforms that need a special alignment. > > Modify the align_addr() call to send in the proper size that we need to > map. > > Before this patch on a system with a EPC page size 64k, we would > incorrectly map 128k (which is larger than our allocation) instead of 64k. > > After this patch, we will correctly map 64k (a single page). (We should > never need to map more than a page to write a single DWORD.) [...] > Feel free to squash this with the patch that it fixes, if you so prefer. Squashed with the rest of the pending changes, per: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=endpoint&id=d2d9f84914e147d6ee399e0ed8d938fea7f0c35c Let me know if anything needs to be updated there. Thank you! Krzysztof
On Tue, Nov 05, 2024 at 06:13:54AM +0900, Krzysztof Wilczyński wrote: > Hello, > > > In dw_pcie_ep_init() we allocate memory from the EPC address space that we > > will later use to raise a MSI/MSI-X IRQ. This memory is only freed in > > dw_pcie_ep_deinit(). > > > > Performing this allocation in dw_pcie_ep_init() is to ensure that we will > > not fail to allocate memory from the EPC address space when trying to raise > > a MSI/MSI-X IRQ. > > > > We still map/unmap this memory every time we raise an IRQ, in order to not > > constantly occupy an iATU, especially for controllers with few iATUs. > > (So we can still fail to raise an MSI/MSI-X IRQ if all iATUs are occupied.) > > > > When allocating this memory in dw_pcie_ep_init(), we allocate > > epc->mem->window.page_size memory, which is the smallest unit that we can > > allocate from the EPC address space. > > > > However, when writing/sending the msg data, which is only 2 bytes for MSI, > > 4 bytes for MSI-X, in either case a single writel() is sufficient. Thus, > > the size that we need to map is a single PCI DWORD (4 bytes). > > > > This is the size that we should send in to the pci_epc_ops::align_addr() > > API. It is align_addr()'s responsibility to return a size that is aligned > > to the EPC page size, for platforms that need a special alignment. > > > > Modify the align_addr() call to send in the proper size that we need to > > map. > > > > Before this patch on a system with a EPC page size 64k, we would > > incorrectly map 128k (which is larger than our allocation) instead of 64k. > > > > After this patch, we will correctly map 64k (a single page). (We should > > never need to map more than a page to write a single DWORD.) > [...] > > Feel free to squash this with the patch that it fixes, if you so prefer. > > Squashed with the rest of the pending changes, per: > > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=endpoint&id=d2d9f84914e147d6ee399e0ed8d938fea7f0c35c > > Let me know if anything needs to be updated there. Thank you Krzysztof! I do not see any point in you adding my Co-developed-by tag since I'm the Author: of the original commit (the commit that you squashed with). As far as I know, the Co-developed-by tag should only be added since there can only be one person marked as Author: And perhaps: [kwilczynski: squashed patch that fixes memory map sizes on platforms with EPC page size of 64k] should be: [kwilczynski: squashed patch that fixes memory map sizes on all platforms] or just: [kwilczynski: squashed patch that fixes memory map sizes] Since I (embarrassingly) always mapped twice as much memory as needed in the original commit. The squashed commit itself looks correct :) Kind regards, Niklas
Hello, [...] > > > Feel free to squash this with the patch that it fixes, if you so prefer. > > > > Squashed with the rest of the pending changes, per: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=endpoint&id=d2d9f84914e147d6ee399e0ed8d938fea7f0c35c > > > > Let me know if anything needs to be updated there. > > Thank you Krzysztof! So thing. > I do not see any point in you adding my Co-developed-by tag since I'm > the Author: of the original commit (the commit that you squashed with). [...] Doh. Sorry! For some reason I took it as if it was another of Damien's patches on the branch. Apologies! That said, we should be good now. > And perhaps: [...] > [kwilczynski: squashed patch that fixes memory map sizes] I'll take this succinct version, thank you! :) > Since I (embarrassingly) always mapped twice as much memory as needed in > the original commit. No worries. > The squashed commit itself looks correct :) Thank you for looking over it! Appreciated. Krzysztof
On Mon, Nov 04, 2024 at 09:51:44PM +0100, Niklas Cassel wrote: > In dw_pcie_ep_init() we allocate memory from the EPC address space that we > will later use to raise a MSI/MSI-X IRQ. This memory is only freed in > dw_pcie_ep_deinit(). > > Performing this allocation in dw_pcie_ep_init() is to ensure that we will > not fail to allocate memory from the EPC address space when trying to raise > a MSI/MSI-X IRQ. > > We still map/unmap this memory every time we raise an IRQ, in order to not > constantly occupy an iATU, especially for controllers with few iATUs. > (So we can still fail to raise an MSI/MSI-X IRQ if all iATUs are occupied.) > > When allocating this memory in dw_pcie_ep_init(), we allocate > epc->mem->window.page_size memory, which is the smallest unit that we can > allocate from the EPC address space. > > However, when writing/sending the msg data, which is only 2 bytes for MSI, > 4 bytes for MSI-X, in either case a single writel() is sufficient. Thus, > the size that we need to map is a single PCI DWORD (4 bytes). > > This is the size that we should send in to the pci_epc_ops::align_addr() > API. It is align_addr()'s responsibility to return a size that is aligned > to the EPC page size, for platforms that need a special alignment. > > Modify the align_addr() call to send in the proper size that we need to > map. > > Before this patch on a system with a EPC page size 64k, we would > incorrectly map 128k (which is larger than our allocation) instead of 64k. > > After this patch, we will correctly map 64k (a single page). (We should > never need to map more than a page to write a single DWORD.) > > Fixes: f68da9a67173 ("PCI: dwc: ep: Use align addr function for dw_pcie_ep_raise_{msi,msix}_irq()") > Signed-off-by: Niklas Cassel <cassel@kernel.org> FWIW, Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > Feel free to squash this with the patch that it fixes, if you so prefer. > > drivers/pci/controller/dwc/pcie-designware-ep.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > index 9bafa62bed1dc..507e40bd18c8f 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > @@ -503,7 +503,7 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, > u32 msg_addr_lower, msg_addr_upper, reg; > struct dw_pcie_ep_func *ep_func; > struct pci_epc *epc = ep->epc; > - size_t msi_mem_size = epc->mem->window.page_size; > + size_t map_size = sizeof(u32); > size_t offset; > u16 msg_ctrl, msg_data; > bool has_upper; > @@ -532,9 +532,9 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, > } > msg_addr = ((u64)msg_addr_upper) << 32 | msg_addr_lower; > > - msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &msi_mem_size, &offset); > + msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &map_size, &offset); > ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr, > - msi_mem_size); > + map_size); > if (ret) > return ret; > > @@ -589,7 +589,7 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no, > struct pci_epf_msix_tbl *msix_tbl; > struct dw_pcie_ep_func *ep_func; > struct pci_epc *epc = ep->epc; > - size_t msi_mem_size = epc->mem->window.page_size; > + size_t map_size = sizeof(u32); > size_t offset; > u32 reg, msg_data, vec_ctrl; > u32 tbl_offset; > @@ -616,9 +616,9 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no, > return -EPERM; > } > > - msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &msi_mem_size, &offset); > + msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &map_size, &offset); > ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr, > - epc->mem->window.page_size); > + map_size); > if (ret) > return ret; > > -- > 2.47.0 >
Hello, > > In dw_pcie_ep_init() we allocate memory from the EPC address space that we > > will later use to raise a MSI/MSI-X IRQ. This memory is only freed in > > dw_pcie_ep_deinit(). > > > > Performing this allocation in dw_pcie_ep_init() is to ensure that we will > > not fail to allocate memory from the EPC address space when trying to raise > > a MSI/MSI-X IRQ. > > > > We still map/unmap this memory every time we raise an IRQ, in order to not > > constantly occupy an iATU, especially for controllers with few iATUs. > > (So we can still fail to raise an MSI/MSI-X IRQ if all iATUs are occupied.) > > > > When allocating this memory in dw_pcie_ep_init(), we allocate > > epc->mem->window.page_size memory, which is the smallest unit that we can > > allocate from the EPC address space. > > > > However, when writing/sending the msg data, which is only 2 bytes for MSI, > > 4 bytes for MSI-X, in either case a single writel() is sufficient. Thus, > > the size that we need to map is a single PCI DWORD (4 bytes). > > > > This is the size that we should send in to the pci_epc_ops::align_addr() > > API. It is align_addr()'s responsibility to return a size that is aligned > > to the EPC page size, for platforms that need a special alignment. > > > > Modify the align_addr() call to send in the proper size that we need to > > map. > > > > Before this patch on a system with a EPC page size 64k, we would > > incorrectly map 128k (which is larger than our allocation) instead of 64k. > > > > After this patch, we will correctly map 64k (a single page). (We should > > never need to map more than a page to write a single DWORD.) > > > > Fixes: f68da9a67173 ("PCI: dwc: ep: Use align addr function for dw_pcie_ep_raise_{msi,msix}_irq()") > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > FWIW, > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Picked this tag, too. Thank you! Krzysztof
diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c index 9bafa62bed1dc..507e40bd18c8f 100644 --- a/drivers/pci/controller/dwc/pcie-designware-ep.c +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c @@ -503,7 +503,7 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, u32 msg_addr_lower, msg_addr_upper, reg; struct dw_pcie_ep_func *ep_func; struct pci_epc *epc = ep->epc; - size_t msi_mem_size = epc->mem->window.page_size; + size_t map_size = sizeof(u32); size_t offset; u16 msg_ctrl, msg_data; bool has_upper; @@ -532,9 +532,9 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, } msg_addr = ((u64)msg_addr_upper) << 32 | msg_addr_lower; - msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &msi_mem_size, &offset); + msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &map_size, &offset); ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr, - msi_mem_size); + map_size); if (ret) return ret; @@ -589,7 +589,7 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no, struct pci_epf_msix_tbl *msix_tbl; struct dw_pcie_ep_func *ep_func; struct pci_epc *epc = ep->epc; - size_t msi_mem_size = epc->mem->window.page_size; + size_t map_size = sizeof(u32); size_t offset; u32 reg, msg_data, vec_ctrl; u32 tbl_offset; @@ -616,9 +616,9 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no, return -EPERM; } - msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &msi_mem_size, &offset); + msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &map_size, &offset); ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr, - epc->mem->window.page_size); + map_size); if (ret) return ret;
In dw_pcie_ep_init() we allocate memory from the EPC address space that we will later use to raise a MSI/MSI-X IRQ. This memory is only freed in dw_pcie_ep_deinit(). Performing this allocation in dw_pcie_ep_init() is to ensure that we will not fail to allocate memory from the EPC address space when trying to raise a MSI/MSI-X IRQ. We still map/unmap this memory every time we raise an IRQ, in order to not constantly occupy an iATU, especially for controllers with few iATUs. (So we can still fail to raise an MSI/MSI-X IRQ if all iATUs are occupied.) When allocating this memory in dw_pcie_ep_init(), we allocate epc->mem->window.page_size memory, which is the smallest unit that we can allocate from the EPC address space. However, when writing/sending the msg data, which is only 2 bytes for MSI, 4 bytes for MSI-X, in either case a single writel() is sufficient. Thus, the size that we need to map is a single PCI DWORD (4 bytes). This is the size that we should send in to the pci_epc_ops::align_addr() API. It is align_addr()'s responsibility to return a size that is aligned to the EPC page size, for platforms that need a special alignment. Modify the align_addr() call to send in the proper size that we need to map. Before this patch on a system with a EPC page size 64k, we would incorrectly map 128k (which is larger than our allocation) instead of 64k. After this patch, we will correctly map 64k (a single page). (We should never need to map more than a page to write a single DWORD.) Fixes: f68da9a67173 ("PCI: dwc: ep: Use align addr function for dw_pcie_ep_raise_{msi,msix}_irq()") Signed-off-by: Niklas Cassel <cassel@kernel.org> --- Feel free to squash this with the patch that it fixes, if you so prefer. drivers/pci/controller/dwc/pcie-designware-ep.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)