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

[Sachin Parekh]

On Thu, Aug 29, 2024 at 7:22 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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