diff mbox series

[v3,08/20] iommu/sprd: Remove detach_dev callback

Message ID 20221128064648.1934720-9-baolu.lu@linux.intel.com
State New
Headers
Series iommu: Retire detach_dev callback |

Commit Message

Baolu Lu Nov. 28, 2022, 6:46 a.m. UTC 
  The IOMMU driver supports default domain, so the detach_dev op will never
be called. Remove it to avoid dead code.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/sprd-iommu.c | 16 ----------------
 1 file changed, 16 deletions(-)

Comments

Jason Gunthorpe Nov. 28, 2022, 1:53 p.m. UTC | #1 
On Mon, Nov 28, 2022 at 02:46:36PM +0800, Lu Baolu wrote:
> The IOMMU driver supports default domain, so the detach_dev op will never
> be called. Remove it to avoid dead code.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/sprd-iommu.c | 16 ----------------
>  1 file changed, 16 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason
Tian, Kevin Nov. 29, 2022, 3:34 a.m. UTC | #2 
> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Monday, November 28, 2022 2:47 PM
> 
> The IOMMU driver supports default domain, so the detach_dev op will never
> be called. Remove it to avoid dead code.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/sprd-iommu.c | 16 ----------------
>  1 file changed, 16 deletions(-)
> 
> diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
> index 219bfa11f7f4..ae94d74b73f4 100644
> --- a/drivers/iommu/sprd-iommu.c
> +++ b/drivers/iommu/sprd-iommu.c
> @@ -255,21 +255,6 @@ static int sprd_iommu_attach_device(struct
> iommu_domain *domain,
>  	return 0;
>  }
> 
> -static void sprd_iommu_detach_device(struct iommu_domain *domain,
> -					     struct device *dev)
> -{
> -	struct sprd_iommu_domain *dom = to_sprd_domain(domain);
> -	struct sprd_iommu_device *sdev = dom->sdev;
> -	size_t pgt_size = sprd_iommu_pgt_size(domain);
> -
> -	if (!sdev)
> -		return;
> -
> -	dma_free_coherent(sdev->dev, pgt_size, dom->pgt_va, dom-
> >pgt_pa);
> -	sprd_iommu_hw_en(sdev, false);
> -	dom->sdev = NULL;
> -}
> -

Looks this reveals a bug in this driver (not caused by this removal).

sprd_iommu_attach_device() doesn't check whether the device has
been already attached to a domain and do auto detach.

It's written in a way that .detach_dev() must be called to free the
dma buffer but ignores the fact that it's not called when default
domain support is claimed.

Then the dma buffer allocated for the previous domain was left
unhandled, causing memory leak.
Chunyan Zhang Nov. 30, 2022, 9:02 a.m. UTC | #3 
On Tue, 29 Nov 2022 at 11:35, Tian, Kevin <kevin.tian@intel.com> wrote:
>
> > From: Lu Baolu <baolu.lu@linux.intel.com>
> > Sent: Monday, November 28, 2022 2:47 PM
> >
> > The IOMMU driver supports default domain, so the detach_dev op will never
> > be called. Remove it to avoid dead code.
> >
> > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > ---
> >  drivers/iommu/sprd-iommu.c | 16 ----------------
> >  1 file changed, 16 deletions(-)
> >
> > diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
> > index 219bfa11f7f4..ae94d74b73f4 100644
> > --- a/drivers/iommu/sprd-iommu.c
> > +++ b/drivers/iommu/sprd-iommu.c
> > @@ -255,21 +255,6 @@ static int sprd_iommu_attach_device(struct
> > iommu_domain *domain,
> >       return 0;
> >  }
> >
> > -static void sprd_iommu_detach_device(struct iommu_domain *domain,
> > -                                          struct device *dev)
> > -{
> > -     struct sprd_iommu_domain *dom = to_sprd_domain(domain);
> > -     struct sprd_iommu_device *sdev = dom->sdev;
> > -     size_t pgt_size = sprd_iommu_pgt_size(domain);
> > -
> > -     if (!sdev)
> > -             return;
> > -
> > -     dma_free_coherent(sdev->dev, pgt_size, dom->pgt_va, dom-
> > >pgt_pa);
> > -     sprd_iommu_hw_en(sdev, false);
> > -     dom->sdev = NULL;
> > -}
> > -
>
> Looks this reveals a bug in this driver (not caused by this removal).
>
> sprd_iommu_attach_device() doesn't check whether the device has
> been already attached to a domain and do auto detach.
>
> It's written in a way that .detach_dev() must be called to free the
> dma buffer but ignores the fact that it's not called when default
> domain support is claimed.
>
> Then the dma buffer allocated for the previous domain was left
> unhandled, causing memory leak.

I'll look into the issue, thanks for pointing this out.

Chunyan
Chunyan Zhang Nov. 30, 2022, 9:03 a.m. UTC | #4 
On Mon, 28 Nov 2022 at 14:55, Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
> The IOMMU driver supports default domain, so the detach_dev op will never
> be called. Remove it to avoid dead code.
>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>

Acked-by: Chunyan Zhang <zhang.lyra@gmail.com>

Thanks,
Chunyan

> ---
>  drivers/iommu/sprd-iommu.c | 16 ----------------
>  1 file changed, 16 deletions(-)
>
> diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
> index 219bfa11f7f4..ae94d74b73f4 100644
> --- a/drivers/iommu/sprd-iommu.c
> +++ b/drivers/iommu/sprd-iommu.c
> @@ -255,21 +255,6 @@ static int sprd_iommu_attach_device(struct iommu_domain *domain,
>         return 0;
>  }
>
> -static void sprd_iommu_detach_device(struct iommu_domain *domain,
> -                                            struct device *dev)
> -{
> -       struct sprd_iommu_domain *dom = to_sprd_domain(domain);
> -       struct sprd_iommu_device *sdev = dom->sdev;
> -       size_t pgt_size = sprd_iommu_pgt_size(domain);
> -
> -       if (!sdev)
> -               return;
> -
> -       dma_free_coherent(sdev->dev, pgt_size, dom->pgt_va, dom->pgt_pa);
> -       sprd_iommu_hw_en(sdev, false);
> -       dom->sdev = NULL;
> -}
> -
>  static int sprd_iommu_map(struct iommu_domain *domain, unsigned long iova,
>                           phys_addr_t paddr, size_t pgsize, size_t pgcount,
>                           int prot, gfp_t gfp, size_t *mapped)
> @@ -414,7 +399,6 @@ static const struct iommu_ops sprd_iommu_ops = {
>         .owner          = THIS_MODULE,
>         .default_domain_ops = &(const struct iommu_domain_ops) {
>                 .attach_dev     = sprd_iommu_attach_device,
> -               .detach_dev     = sprd_iommu_detach_device,
>                 .map_pages      = sprd_iommu_map,
>                 .unmap_pages    = sprd_iommu_unmap,
>                 .iotlb_sync_map = sprd_iommu_sync_map,
> --
> 2.34.1
>
diff mbox series

Patch

diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
index 219bfa11f7f4..ae94d74b73f4 100644
--- a/drivers/iommu/sprd-iommu.c
+++ b/drivers/iommu/sprd-iommu.c
@@ -255,21 +255,6 @@  static int sprd_iommu_attach_device(struct iommu_domain *domain,
 	return 0;
 }
 
-static void sprd_iommu_detach_device(struct iommu_domain *domain,
-					     struct device *dev)
-{
-	struct sprd_iommu_domain *dom = to_sprd_domain(domain);
-	struct sprd_iommu_device *sdev = dom->sdev;
-	size_t pgt_size = sprd_iommu_pgt_size(domain);
-
-	if (!sdev)
-		return;
-
-	dma_free_coherent(sdev->dev, pgt_size, dom->pgt_va, dom->pgt_pa);
-	sprd_iommu_hw_en(sdev, false);
-	dom->sdev = NULL;
-}
-
 static int sprd_iommu_map(struct iommu_domain *domain, unsigned long iova,
 			  phys_addr_t paddr, size_t pgsize, size_t pgcount,
 			  int prot, gfp_t gfp, size_t *mapped)
@@ -414,7 +399,6 @@  static const struct iommu_ops sprd_iommu_ops = {
 	.owner		= THIS_MODULE,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= sprd_iommu_attach_device,
-		.detach_dev	= sprd_iommu_detach_device,
 		.map_pages	= sprd_iommu_map,
 		.unmap_pages	= sprd_iommu_unmap,
 		.iotlb_sync_map	= sprd_iommu_sync_map,