Re: [PATCH v2] iommu/rockchip: Add missing set_platform_dma_ops callback

[Baolu Lu]

On 2023/3/24 21:24, Steven Price wrote:
> On 24/03/2023 13:16, Baolu Lu wrote:
> > On 2023/3/24 19:11, Steven Price wrote:
> > > Similar to exynos, we need a set_platform_dma_ops() callback for proper
> > > operation on ARM 32 bit after recent changes in the IOMMU framework
> > > (detach ops removal). But also the use of a NULL domain is confusing.
> > >
> > > Rework the code to have a singleton rk_identity_domain which is assigned
> > > to domain when using an identity mapping rather than "detaching". This
> > > makes the code easier to reason about.
> > >
> > > Signed-off-by: Steven Price<steven.price@xxxxxxx>
> > > ---
> > > Changes since v1[1]:
> > >
> > >    * Reworked the code to avoid a NULL domain, instead a singleton
> > >      rk_identity_domain is used instead. The 'detach' language is no
> > >      longer used.
> > >
> > > [1]
> > > https://lore.kernel.org/r/20230315164152.333251-1-steven.price%40arm.com
> > >
> > >    drivers/iommu/rockchip-iommu.c | 50 ++++++++++++++++++++++++++--------
> > >    1 file changed, 39 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/iommu/rockchip-iommu.c
> > > b/drivers/iommu/rockchip-iommu.c
> > > index f30db22ea5d7..437541004994 100644
> > > --- a/drivers/iommu/rockchip-iommu.c
> > > +++ b/drivers/iommu/rockchip-iommu.c
> > > @@ -124,6 +124,7 @@ struct rk_iommudata {
> > >      static struct device *dma_dev;
> > >    static const struct rk_iommu_ops *rk_ops;
> > > +static struct iommu_domain rk_identity_domain;
> > >      static inline void rk_table_flush(struct rk_iommu_domain *dom,
> > > dma_addr_t dma,
> > >                      unsigned int count)
> > > @@ -980,26 +981,27 @@ static int rk_iommu_enable(struct rk_iommu *iommu)
> > >        return ret;
> > >    }
> > >    -static void rk_iommu_detach_device(struct iommu_domain *domain,
> > > -                   struct device *dev)
> > > +static int rk_iommu_identity_attach(struct iommu_domain
> > > *identity_domain,
> > > +                    struct device *dev)
> > >    {
> > >        struct rk_iommu *iommu;
> > > -    struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
> > > +    struct rk_iommu_domain *rk_domain;
> > >        unsigned long flags;
> > >        int ret;
> > >          /* Allow 'virtual devices' (eg drm) to detach from domain */
> > >        iommu = rk_iommu_from_dev(dev);
> > >        if (!iommu)
> > > -        return;
> > > +        return -ENODEV;
> > > +
> > > +    rk_domain = to_rk_domain(iommu->domain);
> > >          dev_dbg(dev, "Detaching from iommu domain\n");
> > >    -    /* iommu already detached */
> > > -    if (iommu->domain != domain)
> > > -        return;
> > > +    if (iommu->domain == identity_domain)
> > > +        return 0;
> > >    -    iommu->domain = NULL;
> > > +    iommu->domain = identity_domain;
> > Where did identity_domain come from? Is it rk_identity_domain?
> It's a parameter of the function. In the case of the call in
> rk_iommu_attach_device() then, yes, it's rk_identity_domain. But this
> function is also the "attach_dev" callback of "rk_identity_ops".
>
> I'll admit this is cargo-culted from Jason's example:
>
> https://lore.kernel.org/linux-iommu/ZBnef7g7GCxogPNz@xxxxxxxx/

Oh! I overlooked that. Thank you for the explanation.

Best regards,
baolu