Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 22 Mar 2023 12:16:35 -0300
From:   Jason Gunthorpe <jgg@ziepe.ca>
To:     Steven Price <steven.price@arm.com>
Cc:     Heiko Stuebner <heiko@sntech.de>, Joerg Roedel <joro@8bytes.org>,
        Will Deacon <will@kernel.org>,
        Robin Murphy <robin.murphy@arm.com>, iommu@lists.linux.dev,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-rockchip@lists.infradead.org,
        Lu Baolu <baolu.lu@linux.intel.com>
Subject: Re: [PATCH] iommu/rockchip: Add missing set_platform_dma_ops callback
Message-ID: <ZBsb01emBJJMZIt0@ziepe.ca>
References: <20230315164152.333251-1-steven.price@arm.com>
 <ZBnBU9OU4iV6CV0W@ziepe.ca>
 <85607806-b888-2d5e-67a4-e9d63ebd1976@arm.com>
 <ZBr5e6tn1i7EE/16@ziepe.ca>
 <a5b946f0-5be8-a656-a8d5-1cd75399f0c4@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a5b946f0-5be8-a656-a8d5-1cd75399f0c4@arm.com>
Precedence: bulk

On Wed, Mar 22, 2023 at 03:08:41PM +0000, Steven Price wrote:
> @@ -1035,8 +1055,9 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
>  	if (iommu->domain == domain)
>  		return 0;
>  
> -	if (iommu->domain)
> -		rk_iommu_detach_device(iommu->domain, dev);
> +	ret = rk_iommu_identity_attach(&rk_identity_domain, dev);
> +	if (ret)
> +		return ret;

>  
>  	iommu->domain = domain;
>  
> @@ -1049,8 +1070,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
>  		return 0;
>  
>  	ret = rk_iommu_enable(iommu);
> -	if (ret)
> -		rk_iommu_detach_device(iommu->domain, dev);

I think this still needs error handling, it should put it back to the
identity domain and return an error code if it fails to attach to the
requested domain.

It should also initlaize iommu->domain to the identity domain when the
iommu struct is allocated. The iommu->domain should never be
NULL. identity domain means the IOMMU is turned off which was
previously called "detached".

Otherwise it looks like I would expect, thanks

Jason