Re: [PATCH v3 2/2] iommu/vt-d: don's issue devTLB flush request when device is disconnected

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

On 2023/12/19 15:27, Ethan Zhao wrote:
> Baolu,
> 
> On 12/19/2023 3:16 PM, Ethan Zhao wrote:
>>
>> On 12/19/2023 2:57 PM, Baolu Lu wrote:
>>> On 2023/12/19 14:49, Ethan Zhao wrote:
>>>>>> diff --git a/drivers/iommu/intel/pasid.c 
>>>>>> b/drivers/iommu/intel/pasid.c
>>>>>> index 74e8e4c17e81..182eb5df244d 100644
>>>>>> --- a/drivers/iommu/intel/pasid.c
>>>>>> +++ b/drivers/iommu/intel/pasid.c
>>>>>> @@ -476,6 +476,23 @@ devtlb_invalidation_with_pasid(struct 
>>>>>> intel_iommu *iommu,
>>>>>>   {
>>>>>>       struct device_domain_info *info;
>>>>>>       u16 sid, qdep, pfsid;
>>>>>> +    struct pci_dev *pdev;
>>>>>> +
>>>>>> +    pdev = to_pci_dev(dev);
>>>>>> +    if (!pdev)
>>>>>> +        return;
>>>>>> +
>>>>>> +    /*
>>>>>> +     * If endpoint device's link was brough down by user's pci 
>>>>>> configuration
>>>>>> +     * access to it's hotplug capable slot link control register, 
>>>>>> as sequence
>>>>>> +     * response for DLLSC, pciehp_ist() will set the device 
>>>>>> error_state to
>>>>>> +     * pci_channel_io_perm_failure. Checking device's state here 
>>>>>> to avoid
>>>>>> +     * issuing meaningless devTLB flush request to it, that might 
>>>>>> cause lockup
>>>>>> +     * warning or deadlock because too long time waiting in 
>>>>>> interrupt context.
>>>>>> +     */
>>>>>> +
>>>>>> +    if (pci_dev_is_disconnected(pdev))
>>>>>> +        return;
>>>>>>         info = dev_iommu_priv_get(dev);
>>>>>>       if (!info || !info->ats_enabled)
>>>>>
>>>>> It's likely better to check the device status after verifying
>>>>> ats_enabled. How about below change?
>>>>>
>>>>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>>>>> index 74e8e4c17e81..fa19c6cdfd8b 100644
>>>>> --- a/drivers/iommu/intel/pasid.c
>>>>> +++ b/drivers/iommu/intel/pasid.c
>>>>> @@ -481,6 +481,9 @@ devtlb_invalidation_with_pasid(struct 
>>>>> intel_iommu *iommu,
>>>>>         if (!info || !info->ats_enabled)
>>>>>                 return;
>>>>>
>>>>> +       if (pci_dev_is_disconnected(to_pci_dev(dev)))
>>>>
>>>> I like this kind of simplicity, but rationalist always brings me 
>>>> back to the no-error(ugly)
>>>>
>>>> style.  🙂
>>>
>>> The rational is that Intel IOMMU driver only supports PCI ATS. So if
>>> device is marked as ATS supported, then it must be a PCI device.
>>> Therefore, it's safe to convert it to pci_device with to_pci_dev().
>>
>> Fair engough !
>>
>>
> May I use  your reviewed-by sign in next version after that ?

I am not sure about the changes in the PCI subsystem. Since the code
here calls the new interface from that subsystem, I need acked-by on
that change before proceeding.

Best regards,
baolu