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

[Ethan Zhao <haifeng.zhao@linux.intel.com>]

On 12/19/2023 3:35 PM, Baolu Lu wrote:
> 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

IC


Thanks,

Ethan

> here calls the new interface from that subsystem, I need acked-by on
> that change before proceeding.
>
> Best regards,
> baolu