Re: [PATCH v3 10/10] iommu/amd: Improve amd_iommu_release_device()

[Vasant Hegde <vasant.hegde@amd.com>]

Jason,

On 10/17/2024 7:09 PM, Jason Gunthorpe wrote:
> On Wed, Oct 16, 2024 at 05:35:01AM +0000, Vasant Hegde wrote:
> 
>> @@ -2251,7 +2234,18 @@ static void amd_iommu_release_device(struct device *dev)
>>  	if (!iommu)
>>  		return;
>>  
>> -	amd_iommu_uninit_device(dev);
>> +	dev_data = dev_iommu_priv_get(dev);
>> +	if (!dev_data)
>> +		return;
> 
> This can never happen, the core code never calls an op unless
> probe_device succeeded.

Correct. Let me remove this check.

> 
>> +	if (dev_data->domain)
>> +		detach_device(dev);
> 
> This is also can't happen. The driver sets release_domain then the
> core code never calls this with anything other than release_domain
> attached.

I wanted to implement ops->release_domain  after this series. May be I can put
it as part of this series itself. We already have all the support. Should be
easy to add.


> 
> Suggest just 
> 
>  struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> 
>  WARN_ON(dev->data->domain);
> 
>> +	/*
>> +	 * We keep dev_data around for unplugged devices and reuse it when the
>> +	 * device is re-plugged - not doing so would introduce a ton of races.
>> +	 */
>> +
> 
> Yikes.. Sounds sketchy. Why doesn't any other driver have these races?

I don't know. Suravee/Joerg may know have historical details. I just moved this
from amd_iommu_uninit_device() to here.

-Vasant