On Mon, Aug 14, 2023 at 11:06:12AM +0800, Baolu Lu wrote:
> On 2023/8/3 8:07, Jason Gunthorpe wrote:
> > What mtk does during mtk_iommu_v1_set_platform_dma() is actually putting
> > the iommu into identity mode. Make this available as a proper IDENTITY
> > domain.
> >
> > The mtk_iommu_v1_def_domain_type() from
> > commit 8bbe13f52cb7 ("iommu/mediatek-v1: Add def_domain_type") explains
> > this was needed to allow probe_finalize() to be called, but now the
> > IDENTITY domain will do the same job so change the returned
> > def_domain_type.
> >
> > mkt_v1 is the only driver that returns IOMMU_DOMAIN_UNMANAGED from
> > def_domain_type(). This allows the next patch to enforce an IDENTITY
> > domain policy for this driver.
>
> This code seems to be not working properly.
>
> * @def_domain_type: device default domain type, return value:
> * - IOMMU_DOMAIN_IDENTITY: must use an identity domain
> * - IOMMU_DOMAIN_DMA: must use a dma domain
> * - 0: use the default setting
>
> The core code is not ready to accept any other return value.
Right, it is not following the spec. The design does do what it is
supposed to though..
> > diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
> > index 8a0a5e5d049f4a..cc3e7d53d33ad9 100644
> > --- a/drivers/iommu/mtk_iommu_v1.c
> > +++ b/drivers/iommu/mtk_iommu_v1.c
> > @@ -319,11 +319,27 @@ static int mtk_iommu_v1_attach_device(struct iommu_domain *domain, struct device
> > return 0;
> > }
> > -static void mtk_iommu_v1_set_platform_dma(struct device *dev)
> > +static int mtk_iommu_v1_identity_attach(struct iommu_domain *identity_domain,
> > + struct device *dev)
> > {
> > struct mtk_iommu_v1_data *data = dev_iommu_priv_get(dev);
> > mtk_iommu_v1_config(data, dev, false);
> > + return 0;
> > +}
> > +
> > +static struct iommu_domain_ops mtk_iommu_v1_identity_ops = {
> > + .attach_dev = mtk_iommu_v1_identity_attach,
> > +};
> > +
> > +static struct iommu_domain mtk_iommu_v1_identity_domain = {
> > + .type = IOMMU_DOMAIN_IDENTITY,
> > + .ops = &mtk_iommu_v1_identity_ops,
> > +};
> > +
> > +static void mtk_iommu_v1_set_platform_dma(struct device *dev)
> > +{
> > + mtk_iommu_v1_identity_attach(&mtk_iommu_v1_identity_domain, dev);
>
> This callback seems to be a dead code now.
Not yet in the series, it is still used. All this patch does is
introduce the identity domain.
> > @@ -443,7 +459,7 @@ static int mtk_iommu_v1_create_mapping(struct device *dev, struct of_phandle_arg
> > static int mtk_iommu_v1_def_domain_type(struct device *dev)
> > {
> > - return IOMMU_DOMAIN_UNMANAGED;
> > + return IOMMU_DOMAIN_IDENTITY;
>
> def_domain_type can't be used for this purpose. But this seems to be a
> temporary code, as it will be removed in patch 09/25.
It looked OK when I checked it, mkt_v1 is really confusing what it
tries to do, but it should call probe_finalize and basically do the
same hacky thing as what UNMANAGED was trying to accomplish.
Did you see something else?
Jason
