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;
-}
-
 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

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
>

> 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.

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

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