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

[Jason Gunthorpe <jgg@ziepe.ca>]

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.

> @Jason,
>   I'm fine with your version of patch as well. Are you going to send proper
> patch -OR- do you want me to respin?

I was hoping you'd test it and can you share the oops message?

Thanks,
Jason