Re: [PATCH v2 iommu/next] iommu: Fix default domain setup

[Baolu Lu <baolu.lu@linux.intel.com>]

On 6/22/23 9:55 PM, Jason Gunthorpe wrote:
> On Thu, Jun 22, 2023 at 10:29:58AM +0530, Vasant Hegde wrote:
>> Hi Baolu, Jason,
>>
>>
>> On 6/20/2023 5:12 PM, Jason Gunthorpe wrote:
>>> On Tue, Jun 20, 2023 at 02:31:43PM +0800, Baolu Lu wrote:
>>>>>    err_restore:
>>>>>    	if (old_dom) {
>>>>>    		__iommu_group_set_domain_internal(
>>>>>    			group, old_dom, IOMMU_SET_DOMAIN_MUST_SUCCEED);
>>>>> +		group->default_domain = old_dom;
>>>>>    		iommu_domain_free(dom);
>>>>>    		old_dom = NULL;
>>>>>    	}
>>>> The err_restore branch doesn't work if old_dom is NULL. We have no means
>>>> to restore a group from a successful first-time attaching to NULL
>>>> attaching.
>>> Yes, this is what I fixed in my alternative version
>> Not really. Even your version has
> I though what Baolu means is that it crashes in some of the cases.
> 
>> +err_restore_domain:
>> +	if (old_dom)
>>
>>
>> Only first time we will have old_dom is NULL and this functions returns error
>> code. iommu_probe_device()/bus_iommu_probe() handler error path properly and
>> frees resources. So I think this is fine.
> Yes, the design is to leave it as-is and know that the error unwind
> will fail to attach the device and free the domain after
> release. There is a comment explaining this.
> 

I thought that perhaps we could throw a warning to the driver when
old_dom is NULL. Something like this:

	if (!old_dom)
		pr_warn("can't restore to old domain\n");
	else
		goto err_restore_domain;

But as Vasant said the only caller can handle this error case
gracefully. Then it's fine to keep the code as it is.

Best regards,
baolu