Messages in this thread Patch in this message | | | Date | Thu, 9 Mar 2023 10:50:31 +0800 | Subject | Re: [PATCH V2] iommu: sprd: release dma buffer to avoid memory leak | From | Baolu Lu <> |
| |
On 3/8/23 8:37 PM, Robin Murphy wrote: > On 2023-03-08 03:41, Chunyan Zhang wrote: >> Release page table DMA buffer when the IOMMU domain is not used: >> >> - Domain freed. >> >> - IOMMU is attaching to a new domain. >> Since one sprd IOMMU servers only one client device, if the IOMMU has >> been attached to other domain, it has to be detached first, that's >> saying the DMA buffer should be released, otherwise that would >> cause memory leak issue. > > This is clearly wrong; domain resources should only be freed when the > domain is freed.
Agreed. Perhaps, in the attach path:
if (!dom->pgt_va) dom->pgt_va = dma_alloc_coherent(sdev->dev, pgt_size, &dom->pgt_pa, GFP_KERNEL); ?
Best regards, baolu
> Just because a caller has detached from a domain > doesn't mean that they can't reattach to it later and expect the > previous mappings to still be in place - it has nothing to do with how > many devices or domains can be active at once. > > Thanks, > Robin. > >> Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com> >> --- >> V2: >> * Added some comment in sprd_iommu_attach_device() for the reason >> of calling sprd_iommu_cleanup(). >> >> V1: https://lkml.org/lkml/2023/2/10/198 >> --- >> drivers/iommu/sprd-iommu.c | 46 ++++++++++++++++++++++++++++++++------ >> 1 file changed, 39 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c >> index ae94d74b73f4..fb2f96df3bca 100644 >> --- a/drivers/iommu/sprd-iommu.c >> +++ b/drivers/iommu/sprd-iommu.c >> @@ -62,6 +62,7 @@ enum sprd_iommu_version { >> * @eb: gate clock which controls IOMMU access >> */ >> struct sprd_iommu_device { >> + struct sprd_iommu_domain *dom; >> enum sprd_iommu_version ver; >> u32 *prot_page_va; >> dma_addr_t prot_page_pa; >> @@ -151,13 +152,6 @@ static struct iommu_domain >> *sprd_iommu_domain_alloc(unsigned int domain_type) >> return &dom->domain; >> } >> -static void sprd_iommu_domain_free(struct iommu_domain *domain) >> -{ >> - struct sprd_iommu_domain *dom = to_sprd_domain(domain); >> - >> - kfree(dom); >> -} >> - >> static void sprd_iommu_first_vpn(struct sprd_iommu_domain *dom) >> { >> struct sprd_iommu_device *sdev = dom->sdev; >> @@ -230,6 +224,29 @@ static void sprd_iommu_hw_en(struct >> sprd_iommu_device *sdev, bool en) >> sprd_iommu_update_bits(sdev, reg_cfg, mask, 0, val); >> } >> +static void sprd_iommu_cleanup(struct sprd_iommu_device *sdev) >> +{ >> + struct sprd_iommu_domain *dom = sdev->dom; >> + size_t pgt_size = sprd_iommu_pgt_size(&dom->domain); >> + >> + dma_free_coherent(sdev->dev, pgt_size, dom->pgt_va, dom->pgt_pa); >> + dom->sdev = NULL; >> + sdev->dom = NULL; >> + sprd_iommu_hw_en(sdev, false); >> +} >> + >> +static void sprd_iommu_domain_free(struct iommu_domain *domain) >> +{ >> + struct sprd_iommu_domain *dom = to_sprd_domain(domain); >> + struct sprd_iommu_device *sdev = dom->sdev; >> + >> + /* Free DMA buffer first if the domain has been attached */ >> + if (sdev) >> + sprd_iommu_cleanup(sdev); >> + >> + kfree(dom); >> +} >> + >> static int sprd_iommu_attach_device(struct iommu_domain *domain, >> struct device *dev) >> { >> @@ -237,14 +254,29 @@ static int sprd_iommu_attach_device(struct >> iommu_domain *domain, >> struct sprd_iommu_domain *dom = to_sprd_domain(domain); >> size_t pgt_size = sprd_iommu_pgt_size(domain); >> + /* Return directly if the domain attached to IOMMU already */ >> if (dom->sdev) >> return -EINVAL; >> + /* The IOMMU already attached to a domain */ >> + if (sdev->dom) { >> + if (sdev->dom == dom) >> + return 0; >> + >> + /* >> + * Clean up the previous domain, one sprd IOMMU servers only >> + * one client device, if the IOMMU has been attached to other >> + * domain, it has to be detached first. >> + */ >> + sprd_iommu_cleanup(sdev); >> + } >> + >> dom->pgt_va = dma_alloc_coherent(sdev->dev, pgt_size, >> &dom->pgt_pa, GFP_KERNEL); >> if (!dom->pgt_va) >> return -ENOMEM; >> dom->sdev = sdev; >> + sdev->dom = dom; >> sprd_iommu_first_ppn(dom); >> sprd_iommu_first_vpn(dom); >
| |