Received-SPF: pass (google.com: domain of linux-kernel+bounces-4812-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <fa317b2b-b1b8-4776-a122-ae7993bd3c41@linux.intel.com>
Date: Tue, 19 Dec 2023 15:35:57 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Cc: baolu.lu@linux.intel.com, linux-pci@vger.kernel.org,
 iommu@lists.linux.dev, linux-kernel@vger.kernel.org, bhelgaas@google.com,
 dwmw2@infradead.org, will@kernel.org, robin.murphy@arm.com, lukas@wunner.de
Subject: Re: [PATCH v3 2/2] iommu/vt-d: don's issue devTLB flush request when
 device is disconnected
Content-Language: en-US
To: Ethan Zhao <haifeng.zhao@linux.intel.com>
References: <20231217114902.3836260-1-haifeng.zhao@linux.intel.com>
 <20231217114902.3836260-3-haifeng.zhao@linux.intel.com>
 <ac0fd341-7def-485e-9f32-530cf8a83273@linux.intel.com>
 <a275032f-cb1d-4ccc-9418-9567aba6b343@linux.intel.com>
 <f7dfdec4-ec52-44b0-9e04-bdcc3af2ba25@linux.intel.com>
 <2fa95471-e22e-46b7-8f23-f4e6abbbd69b@linux.intel.com>
 <53591775-b93c-4963-b57b-8027e39a6ecc@linux.intel.com>
From: Baolu Lu <baolu.lu@linux.intel.com>
In-Reply-To: <53591775-b93c-4963-b57b-8027e39a6ecc@linux.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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