Received-SPF: pass (google.com: domain of linux-kernel+bounces-4791-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: <2fa95471-e22e-46b7-8f23-f4e6abbbd69b@linux.intel.com>
Date: Tue, 19 Dec 2023 15:16:50 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 2/2] iommu/vt-d: don's issue devTLB flush request when
 device is disconnected
To: Baolu Lu <baolu.lu@linux.intel.com>, bhelgaas@google.com,
 dwmw2@infradead.org, will@kernel.org, robin.murphy@arm.com, lukas@wunner.de
Cc: linux-pci@vger.kernel.org, iommu@lists.linux.dev,
 linux-kernel@vger.kernel.org
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>
From: Ethan Zhao <haifeng.zhao@linux.intel.com>
In-Reply-To: <f7dfdec4-ec52-44b0-9e04-bdcc3af2ba25@linux.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


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 !


Thanks,

Ethan

>
> If you move this up before ATS checking, then you need to check
> dev_is_pci().
>
> Best regards,
> baolu
>