Re: [PATCH v3 1/6] ARM/dma-mapping: Add arm_iommu_release_device()

[Baolu Lu]

On 3/11/23 6:04 AM, Robin Murphy wrote:
> On 2023-03-06 02:57, Lu Baolu wrote:
> > It is like arm_iommu_detach_device() except it handles the special case
> > of being called under an iommu driver's release operation. In this case
> > the driver must have already detached the device from any attached
> > domain before calling this function.
> >
> > Replace arm_iommu_detach_device() with arm_iommu_release_device() in the
> > release path of the ipmmu-vmsa driver.
> >
> > The bonus is that it also removes a obstacle of arm_iommu_detach_device()
> > re-entering the iommu core during release_device. With this removed, the
> > iommu core code could be simplified a lot.
> >
> > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > ---
> >   arch/arm/include/asm/dma-iommu.h |  1 +
> >   arch/arm/mm/dma-mapping.c        | 25 +++++++++++++++++++++++++
> >   drivers/iommu/ipmmu-vmsa.c       | 15 +++++++++++++--
> >   3 files changed, 39 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/dma-iommu.h 
> > b/arch/arm/include/asm/dma-iommu.h
> > index fe9ef6f79e9c..ea7198a17861 100644
> > --- a/arch/arm/include/asm/dma-iommu.h
> > +++ b/arch/arm/include/asm/dma-iommu.h
> > @@ -31,6 +31,7 @@ void arm_iommu_release_mapping(struct 
> > dma_iommu_mapping *mapping);
> >   int arm_iommu_attach_device(struct device *dev,
> >                       struct dma_iommu_mapping *mapping);
> >   void arm_iommu_detach_device(struct device *dev);
> > +void arm_iommu_release_device(struct device *dev);
> >   #endif /* __KERNEL__ */
> >   #endif
> > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> > index 8bc01071474a..96fa27f4a164 100644
> > --- a/arch/arm/mm/dma-mapping.c
> > +++ b/arch/arm/mm/dma-mapping.c
> > @@ -1682,6 +1682,31 @@ int arm_iommu_attach_device(struct device *dev,
> >   }
> >   EXPORT_SYMBOL_GPL(arm_iommu_attach_device);
> > +/**
> > + * arm_iommu_release_device
> > + * @dev: valid struct device pointer
> > + *
> > + * This is like arm_iommu_detach_device() except it handles the special
> > + * case of being called under an iommu driver's release operation. In 
> > this
> > + * case the driver must have already detached the device from any 
> > attached
> > + * domain before calling this function.
> > + */
> > +void arm_iommu_release_device(struct device *dev)
> > +{
> > +    struct dma_iommu_mapping *mapping;
> > +
> > +    mapping = to_dma_iommu_mapping(dev);
> > +    if (!mapping) {
> > +        dev_warn(dev, "Not attached\n");
> > +        return;
> > +    }
> > +
> > +    kref_put(&mapping->kref, release_iommu_mapping);
> > +    to_dma_iommu_mapping(dev) = NULL;
> > +    set_dma_ops(dev, NULL);
> > +}
> > +EXPORT_SYMBOL_GPL(arm_iommu_release_device);
> > +
> >   /**
> >    * arm_iommu_detach_device
> >    * @dev: valid struct device pointer
> > diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> > index bdf1a4e5eae0..de9c74cf61a4 100644
> > --- a/drivers/iommu/ipmmu-vmsa.c
> > +++ b/drivers/iommu/ipmmu-vmsa.c
> > @@ -30,7 +30,7 @@
> >   #define arm_iommu_create_mapping(...)    NULL
> >   #define arm_iommu_attach_device(...)    -ENODEV
> >   #define arm_iommu_release_mapping(...)    do {} while (0)
> > -#define arm_iommu_detach_device(...)    do {} while (0)
> > +#define arm_iommu_release_device(...)    do {} while (0)
> >   #endif
> >   #define IPMMU_CTX_MAX        16U
> > @@ -820,7 +820,18 @@ static void ipmmu_probe_finalize(struct device *dev)
> >   static void ipmmu_release_device(struct device *dev)
> >   {
> > -    arm_iommu_detach_device(dev);
> > +    struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> > +    struct ipmmu_vmsa_device *mmu = to_ipmmu(dev);
> > +    unsigned int i;
> > +
> > +    for (i = 0; i < fwspec->num_ids; ++i) {
> > +        unsigned int utlb = fwspec->ids[i];
> > +
> > +        ipmmu_imuctr_write(mmu, utlb, 0);
> > +        mmu->utlb_ctx[utlb] = IPMMU_CTX_INVALID;
> > +    }
> > +
> > +    arm_iommu_release_device(dev);
>
> Just call the existing arm_iommu_release_mapping(). Look at where the 
> BUS_NOTIFY_REMOVED_DEVICE point is in device_del(); this device is not 
> coming back. Zeroing out pointers and testing for a condition which 
> cannot be true by construction is simply a waste of time and code.

Fair enough. But the driver seems to have done things wrong.

It creates arm mappings in iommu probe path, but release it in the
driver's .remove path (see ipmmu_remove()). So perhaps we should move
calling arm_iommu_release_mapping() from ipmmu_remove() to
ipmmu_release_device()?

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index de9c74cf61a4..057050c28360 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -831,7 +831,7 @@ static void ipmmu_release_device(struct device *dev)
                mmu->utlb_ctx[utlb] = IPMMU_CTX_INVALID;
        }

-       arm_iommu_release_device(dev);
+       arm_iommu_release_mapping(mmu->mapping);
 }

 static struct iommu_group *ipmmu_find_group(struct device *dev)
@@ -1091,8 +1091,6 @@ static int ipmmu_remove(struct platform_device *pdev)
        iommu_device_sysfs_remove(&mmu->iommu);
        iommu_device_unregister(&mmu->iommu);

-       arm_iommu_release_mapping(mmu->mapping);
-
        ipmmu_device_reset(mmu);

        return 0;

Best regards,
baolu