1. Patchew
  2. linux
  3. View series

[RFC PATCH] driver core: platform: Call iommu_release_device in dma_cleanup

Sachin Parekh posted 1 patch 1 year, 5 months ago 
drivers/base/platform.c | 3 +++
drivers/iommu/iommu.c   | 3 +--
include/linux/iommu.h   | 1 +
3 files changed, 5 insertions(+), 2 deletions(-)
[RFC PATCH] driver core: platform: Call iommu_release_device in dma_cleanup
Posted by Sachin Parekh 1 year, 5 months ago 
Installing a kernel module that has an iommu node in its
device tree increments corresponding iommu kernel module
reference count during driver_register.
Removing the same kernel module doesn't decrement the
iommu reference count resulting in error while
removing the iommu kernel module

    $ modprobe arm-smmu-v3
    $ modprobe test_module
    $ modprobe -r test_module
    $ modprobe -r arm-smmu-v3
    modprobe: can't unload module arm_smmu_v3: Resource temporarily unavailable

Cause:
    platform_driver_register
        ...
        -> platform_dma_configure
            ...
            -> iommu_probe_device (Increments reference count)

    platform_driver_unregister
        ...
        -> platform_dma_cleanup
            ...
            -> No corresponding iommu_release_device call

Fix:
    Call iommu_release_device in platform_dma_cleanup to remove the
    iommu from the corresponding kernel module

Signed-off-by: Sachin Parekh <sachinparekh@google.com>
---
 drivers/base/platform.c | 3 +++
 drivers/iommu/iommu.c   | 3 +--
 include/linux/iommu.h   | 1 +
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 4c3ee6521ba5..c8125325a5e9 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -1467,6 +1467,9 @@ static void platform_dma_cleanup(struct device *dev)
 
 	if (!drv->driver_managed_dma)
 		iommu_device_unuse_default_domain(dev);
+
+	if (dev_of_node(dev))
+		iommu_release_device(dev);
 }
 
 static const struct dev_pm_ops platform_dev_pm_ops = {
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index ed6c5cb60c5a..495f548fd4b9 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -92,7 +92,6 @@ static const char * const iommu_group_resv_type_string[] = {
 
 static int iommu_bus_notifier(struct notifier_block *nb,
 			      unsigned long action, void *data);
-static void iommu_release_device(struct device *dev);
 static struct iommu_domain *
 __iommu_group_domain_alloc(struct iommu_group *group, unsigned int type);
 static int __iommu_attach_device(struct iommu_domain *domain,
@@ -663,7 +662,7 @@ static void __iommu_group_remove_device(struct device *dev)
 	iommu_group_put(group);
 }
 
-static void iommu_release_device(struct device *dev)
+void iommu_release_device(struct device *dev)
 {
 	struct iommu_group *group = dev->iommu_group;
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 04cbdae0052e..2f9cb7d9dadf 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -1049,6 +1049,7 @@ void dev_iommu_priv_set(struct device *dev, void *priv);
 
 extern struct mutex iommu_probe_device_lock;
 int iommu_probe_device(struct device *dev);
+void iommu_release_device(struct device *dev);
 
 int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features f);
 int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features f);
-- 
2.46.0.295.g3b9ea8a38a-goog
Re: [RFC PATCH] driver core: platform: Call iommu_release_device in dma_cleanup
Posted by Robin Murphy 1 year, 5 months ago 
On 2024-08-29 1:05 pm, Sachin Parekh wrote:
> Installing a kernel module that has an iommu node in its
> device tree increments corresponding iommu kernel module
> reference count during driver_register.
> Removing the same kernel module doesn't decrement the
> iommu reference count resulting in error while
> removing the iommu kernel module
> 
>      $ modprobe arm-smmu-v3
>      $ modprobe test_module
>      $ modprobe -r test_module
>      $ modprobe -r arm-smmu-v3
>      modprobe: can't unload module arm_smmu_v3: Resource temporarily unavailable
> 
> Cause:
>      platform_driver_register
>          ...
>          -> platform_dma_configure
>              ...
>              -> iommu_probe_device (Increments reference count)
> 
>      platform_driver_unregister
>          ...
>          -> platform_dma_cleanup
>              ...
>              -> No corresponding iommu_release_device call
> 
> Fix:
>      Call iommu_release_device in platform_dma_cleanup to remove the
>      iommu from the corresponding kernel module

There is nothing to fix here, the current behaviour is correct and 
expected. It appears the comment in of_iommu_configure() got lost, but 
the equivalent in acpi_iommu_configure_id() is still there - the dodgy 
iommu_probe_device() calls at those points still logically represent the 
same operation which should have happened via iommu_bus_notifier() or 
iommu_device_register(), it's just being replayed later for hacky reasons.

IOMMU groups are supposed to be tied to the lifetime of the struct 
device, and unbinding a driver does not make the device itself go away - 
if the devicetree says it exists, then the IOMMU layer knows that. 
Modular IOMMU drivers are still not really intended to be removable, 
unless all their client devices can actually be removed form the system 
(and/or the IOMMU driver is being tested without any clients present).

Thanks,
Robin.

> Signed-off-by: Sachin Parekh <sachinparekh@google.com>
> ---
>   drivers/base/platform.c | 3 +++
>   drivers/iommu/iommu.c   | 3 +--
>   include/linux/iommu.h   | 1 +
>   3 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 4c3ee6521ba5..c8125325a5e9 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -1467,6 +1467,9 @@ static void platform_dma_cleanup(struct device *dev)
>   
>   	if (!drv->driver_managed_dma)
>   		iommu_device_unuse_default_domain(dev);
> +
> +	if (dev_of_node(dev))
> +		iommu_release_device(dev);
>   }
>   
>   static const struct dev_pm_ops platform_dev_pm_ops = {
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index ed6c5cb60c5a..495f548fd4b9 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -92,7 +92,6 @@ static const char * const iommu_group_resv_type_string[] = {
>   
>   static int iommu_bus_notifier(struct notifier_block *nb,
>   			      unsigned long action, void *data);
> -static void iommu_release_device(struct device *dev);
>   static struct iommu_domain *
>   __iommu_group_domain_alloc(struct iommu_group *group, unsigned int type);
>   static int __iommu_attach_device(struct iommu_domain *domain,
> @@ -663,7 +662,7 @@ static void __iommu_group_remove_device(struct device *dev)
>   	iommu_group_put(group);
>   }
>   
> -static void iommu_release_device(struct device *dev)
> +void iommu_release_device(struct device *dev)
>   {
>   	struct iommu_group *group = dev->iommu_group;
>   
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 04cbdae0052e..2f9cb7d9dadf 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -1049,6 +1049,7 @@ void dev_iommu_priv_set(struct device *dev, void *priv);
>   
>   extern struct mutex iommu_probe_device_lock;
>   int iommu_probe_device(struct device *dev);
> +void iommu_release_device(struct device *dev);
>   
>   int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features f);
>   int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features f);
Re: [RFC PATCH] driver core: platform: Call iommu_release_device in dma_cleanup
Posted by Sachin Parekh 1 year, 5 months ago 
On Thu, Aug 29, 2024 at 7:28 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2024-08-29 1:05 pm, Sachin Parekh wrote:
> > Installing a kernel module that has an iommu node in its
> > device tree increments corresponding iommu kernel module
> > reference count during driver_register.
> > Removing the same kernel module doesn't decrement the
> > iommu reference count resulting in error while
> > removing the iommu kernel module
> >
> >      $ modprobe arm-smmu-v3
> >      $ modprobe test_module
> >      $ modprobe -r test_module
> >      $ modprobe -r arm-smmu-v3
> >      modprobe: can't unload module arm_smmu_v3: Resource temporarily unavailable
> >
> > Cause:
> >      platform_driver_register
> >          ...
> >          -> platform_dma_configure
> >              ...
> >              -> iommu_probe_device (Increments reference count)
> >
> >      platform_driver_unregister
> >          ...
> >          -> platform_dma_cleanup
> >              ...
> >              -> No corresponding iommu_release_device call
> >
> > Fix:
> >      Call iommu_release_device in platform_dma_cleanup to remove the
> >      iommu from the corresponding kernel module
>
> There is nothing to fix here, the current behaviour is correct and
> expected. It appears the comment in of_iommu_configure() got lost, but
> the equivalent in acpi_iommu_configure_id() is still there - the dodgy
> iommu_probe_device() calls at those points still logically represent the
> same operation which should have happened via iommu_bus_notifier() or
> iommu_device_register(), it's just being replayed later for hacky reasons.
>
> IOMMU groups are supposed to be tied to the lifetime of the struct
> device, and unbinding a driver does not make the device itself go away -
> if the devicetree says it exists, then the IOMMU layer knows that.
> Modular IOMMU drivers are still not really intended to be removable,
> unless all their client devices can actually be removed form the system
> (and/or the IOMMU driver is being tested without any clients present).
>

Thanks Robin for explaining the intended behaviour and the context.

Does this mean that the iommu reference count should not be decremented
when we call platform_driver_unregister ?
In my scenario, platform_driver_register is incrementing the reference
count so I expected platform_driver_unregister to decrement the reference count

Even if the IOMMU drivers are not intended to be removed, the reference count
should be in accordance with the drivers that register the iommu group ?

Thanks,
Sachin
Re: [RFC PATCH] driver core: platform: Call iommu_release_device in dma_cleanup
Posted by Greg KH 1 year, 5 months ago 
On Thu, Aug 29, 2024 at 12:05:04PM +0000, Sachin Parekh wrote:
> Installing a kernel module that has an iommu node in its
> device tree increments corresponding iommu kernel module
> reference count during driver_register.
> Removing the same kernel module doesn't decrement the
> iommu reference count resulting in error while
> removing the iommu kernel module

Please wrap kernel changelog text at 72 columns like your editor asked
you to :)

>     $ modprobe arm-smmu-v3
>     $ modprobe test_module
>     $ modprobe -r test_module
>     $ modprobe -r arm-smmu-v3
>     modprobe: can't unload module arm_smmu_v3: Resource temporarily unavailable

Does this happen for any in-kernel driver?

Why has this not been noticed before?

> 
> Cause:
>     platform_driver_register
>         ...
>         -> platform_dma_configure
>             ...
>             -> iommu_probe_device (Increments reference count)
> 
>     platform_driver_unregister
>         ...
>         -> platform_dma_cleanup
>             ...
>             -> No corresponding iommu_release_device call
> 
> Fix:
>     Call iommu_release_device in platform_dma_cleanup to remove the
>     iommu from the corresponding kernel module
> 
> Signed-off-by: Sachin Parekh <sachinparekh@google.com>

What commit id does this fix?

> ---
>  drivers/base/platform.c | 3 +++
>  drivers/iommu/iommu.c   | 3 +--
>  include/linux/iommu.h   | 1 +
>  3 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 4c3ee6521ba5..c8125325a5e9 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -1467,6 +1467,9 @@ static void platform_dma_cleanup(struct device *dev)
>  
>  	if (!drv->driver_managed_dma)
>  		iommu_device_unuse_default_domain(dev);
> +
> +	if (dev_of_node(dev))
> +		iommu_release_device(dev);

Are you sure that all devices that pass this should have this call made?
How well was this tested on different systems?

thanks,

greg k-h
Re: [RFC PATCH] driver core: platform: Call iommu_release_device in dma_cleanup
Posted by Sachin Parekh 1 year, 5 months ago 
On Thu, Aug 29, 2024 at 7:22 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Aug 29, 2024 at 12:05:04PM +0000, Sachin Parekh wrote:
> > Installing a kernel module that has an iommu node in its
> > device tree increments corresponding iommu kernel module
> > reference count during driver_register.
> > Removing the same kernel module doesn't decrement the
> > iommu reference count resulting in error while
> > removing the iommu kernel module
>
> Please wrap kernel changelog text at 72 columns like your editor asked
> you to :)

I had wrapped while creating the patch but I suppose it got messed up
somewhere. Sorry about that, I will fix it next time.

> >     $ modprobe arm-smmu-v3
> >     $ modprobe test_module
> >     $ modprobe -r test_module
> >     $ modprobe -r arm-smmu-v3
> >     modprobe: can't unload module arm_smmu_v3: Resource temporarily unavailable
>
> Does this happen for any in-kernel driver?
>
> Why has this not been noticed before?
>

As per Robin's explanation, IOMMU drivers are not intended to be removed so
that could be the reason why this hasn't been noticed.

> >
> > Cause:
> >     platform_driver_register
> >         ...
> >         -> platform_dma_configure
> >             ...
> >             -> iommu_probe_device (Increments reference count)
> >
> >     platform_driver_unregister
> >         ...
> >         -> platform_dma_cleanup
> >             ...
> >             -> No corresponding iommu_release_device call
> >
> > Fix:
> >     Call iommu_release_device in platform_dma_cleanup to remove the
> >     iommu from the corresponding kernel module
> >
> > Signed-off-by: Sachin Parekh <sachinparekh@google.com>
>
> What commit id does this fix?
>

I think these two commit IDs are relevant in this context:
512881eacfa72c2136b27b9934b7b27504a9efc2
07397df29e57cde5799af16e8f148ae10ed75285


> > ---
> >  drivers/base/platform.c | 3 +++
> >  drivers/iommu/iommu.c   | 3 +--
> >  include/linux/iommu.h   | 1 +
> >  3 files changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 4c3ee6521ba5..c8125325a5e9 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -1467,6 +1467,9 @@ static void platform_dma_cleanup(struct device *dev)
> >
> >       if (!drv->driver_managed_dma)
> >               iommu_device_unuse_default_domain(dev);
> > +
> > +     if (dev_of_node(dev))
> > +             iommu_release_device(dev);
>
> Are you sure that all devices that pass this should have this call made?
> How well was this tested on different systems?
>

I am not sure hence I created this patch to understand if this is the
correct approach.

Thanks,
Sachin

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.