[v13,1/3] PCI: make pci_dev_is_disconnected() helper public for other drivers

  Make pci_dev_is_disconnected() public so that it can be called from
Intel VT-d driver to quickly fix/workaround the surprise removal
unplug hang issue for those ATS capable devices on PCIe switch downstream
hotplug capable ports.

Beside pci_device_is_present() function, this one has no config space
space access, so is light enough to optimize the normal pure surprise
removal and safe removal flow.

Tested-by: Haorong Ye <yehaorong@bytedance.com>
Signed-off-by: Ethan Zhao <haifeng.zhao@linux.intel.com>
---
 drivers/pci/pci.h   | 5 -----
 include/linux/pci.h | 5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)
  

On 2024/2/22 17:02, Ethan Zhao wrote:
> Make pci_dev_is_disconnected() public so that it can be called from
> Intel VT-d driver to quickly fix/workaround the surprise removal
> unplug hang issue for those ATS capable devices on PCIe switch downstream
> hotplug capable ports.
> 
> Beside pci_device_is_present() function, this one has no config space
> space access, so is light enough to optimize the normal pure surprise
> removal and safe removal flow.
> 
> Tested-by: Haorong Ye<yehaorong@bytedance.com>
> Signed-off-by: Ethan Zhao<haifeng.zhao@linux.intel.com>
> ---
>   drivers/pci/pci.h   | 5 -----
>   include/linux/pci.h | 5 +++++
>   2 files changed, 5 insertions(+), 5 deletions(-)

Hi PCI subsystem maintainers,

The iommu drivers (including, but not limited to, the Intel VT-d driver)
require a helper to check the physical presence of a PCI device in two
scenarios:

- During the iommu_release_device() path: This ensures the device is
   physically present before sending device TLB invalidation to device.

- During the device driver lifecycle when a device TLB invalidation
   timeout event is generated by the IOMMU hardware: This helps handle
   situations where the device might have been hot-removed.

While there may be some adjustments needed in patch 3/3, I'd like to
confirm with you whether it's feasible to expose this helper for general
use within the iommu subsystem.

If you agree with this change, I can route this patch to Linus through
the iommu tree with an "acked-by" or "reviewed-by" tag from you.

Best regards,
baolu
  

I'm not a PCI maintainer, but first two patches seem good to me.  For
the third patch, my complaints were really minor.  Let's just add a
Fixes tag for sure, the rest is okay.

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter
  

On 2/23/2024 2:47 PM, Dan Carpenter wrote:
> I'm not a PCI maintainer, but first two patches seem good to me.  For
> the third patch, my complaints were really minor.  Let's just add a
> Fixes tag for sure, the rest is okay.
>
> Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

Thanks Dan.

>
> regards,
> dan carpenter
>
  

On Thu, Feb 22, 2024 at 08:54:54PM +0800, Baolu Lu wrote:
> On 2024/2/22 17:02, Ethan Zhao wrote:
> > Make pci_dev_is_disconnected() public so that it can be called from
> > Intel VT-d driver to quickly fix/workaround the surprise removal
> > unplug hang issue for those ATS capable devices on PCIe switch downstream
> > hotplug capable ports.
> > 
> > Beside pci_device_is_present() function, this one has no config space
> > space access, so is light enough to optimize the normal pure surprise
> > removal and safe removal flow.
> > 
> > Tested-by: Haorong Ye<yehaorong@bytedance.com>
> > Signed-off-by: Ethan Zhao<haifeng.zhao@linux.intel.com>
> > ---
> >   drivers/pci/pci.h   | 5 -----
> >   include/linux/pci.h | 5 +++++
> >   2 files changed, 5 insertions(+), 5 deletions(-)
> 
> Hi PCI subsystem maintainers,
> 
> The iommu drivers (including, but not limited to, the Intel VT-d driver)
> require a helper to check the physical presence of a PCI device in two
> scenarios:
> 
> - During the iommu_release_device() path: This ensures the device is
>   physically present before sending device TLB invalidation to device.

This wording is fundamentally wrong.  Testing
pci_dev_is_disconnected() can never ensure the device will still be
present by the time a TLB invalidation is sent.

The device may be removed after the pci_dev_is_disconnected() test and
before a TLB invalidate is sent.

This is why I hesitate to expose pci_dev_is_disconnected() (and
pci_device_is_present(), which we already export) outside
drivers/pci/.  They both lead to terrible mistakes like relying on the
false assumption that the result will remain valid after the functions
return, without any recognition that we MUST be able to deal with the
cases where that assumption is broken.

This series claims to avoid "continuous hard lockup warnings and
system hangs".  It may reduce the likelihood, but I don't think it can
completely avoid them.  

I don't see any acknowledgement of that in the commit logs of this
series.  E.g., it doesn't say "we can recover from ATS Invalidate
Completion Timeouts, but the timeouts are on the order of minutes, so
we want to avoid them when possible."

And given the "system hangs" language, I am not convinced that we
actually *can* recover from those timeouts.

Using pci_dev_is_disconnected() may make those timeouts less frequent
and give the illusion that the problem is "solved", but it just means
the problem is still there and harder to reproduce.

> - During the device driver lifecycle when a device TLB invalidation
>   timeout event is generated by the IOMMU hardware: This helps handle
>   situations where the device might have been hot-removed.
> 
> While there may be some adjustments needed in patch 3/3, I'd like to
> confirm with you whether it's feasible to expose this helper for general
> use within the iommu subsystem.
> 
> If you agree with this change, I can route this patch to Linus through
> the iommu tree with an "acked-by" or "reviewed-by" tag from you.
> 
> Best regards,
> baolu
  

On 2/27/2024 7:05 AM, Bjorn Helgaas wrote:
> On Thu, Feb 22, 2024 at 08:54:54PM +0800, Baolu Lu wrote:
>> On 2024/2/22 17:02, Ethan Zhao wrote:
>>> Make pci_dev_is_disconnected() public so that it can be called from
>>> Intel VT-d driver to quickly fix/workaround the surprise removal
>>> unplug hang issue for those ATS capable devices on PCIe switch downstream
>>> hotplug capable ports.
>>>
>>> Beside pci_device_is_present() function, this one has no config space
>>> space access, so is light enough to optimize the normal pure surprise
>>> removal and safe removal flow.
>>>
>>> Tested-by: Haorong Ye<yehaorong@bytedance.com>
>>> Signed-off-by: Ethan Zhao<haifeng.zhao@linux.intel.com>
>>> ---
>>>    drivers/pci/pci.h   | 5 -----
>>>    include/linux/pci.h | 5 +++++
>>>    2 files changed, 5 insertions(+), 5 deletions(-)
>> Hi PCI subsystem maintainers,
>>
>> The iommu drivers (including, but not limited to, the Intel VT-d driver)
>> require a helper to check the physical presence of a PCI device in two
>> scenarios:
>>
>> - During the iommu_release_device() path: This ensures the device is
>>    physically present before sending device TLB invalidation to device.
> This wording is fundamentally wrong.  Testing
> pci_dev_is_disconnected() can never ensure the device will still be
> present by the time a TLB invalidation is sent.

The logic of testing pci_dev_is_disconnected() in patch [2/3] works
in the opposite:

1. if pci_dev_is_disconnected() return true, means the device is in
    the process of surprise removal handling, adapter already been
    removed from the slot.

2. for removed device, no need to send ATS invalidation request to it.
    removed device lost power, its devTLB wouldn't be valid anymore.

3. if pci_dev_is_disconnected() return false, the device is *likely*
    to be removed at any momoment after this function called.
    such case will be treated in the iommu ITE fault handling, not to
    retry the timeout request if device isn't present (patch [3/3]).

>
> The device may be removed after the pci_dev_is_disconnected() test and
> before a TLB invalidate is sent.

even in the process while TLB is invalidating.

>
> This is why I hesitate to expose pci_dev_is_disconnected() (and
> pci_device_is_present(), which we already export) outside
> drivers/pci/.  They both lead to terrible mistakes like relying on the
> false assumption that the result will remain valid after the functions
> return, without any recognition that we MUST be able to deal with the
> cases where that assumption is broken.

Yup, your concern is worthy ,but isn't happening within this patchset.

>
> This series claims to avoid "continuous hard lockup warnings and
> system hangs".  It may reduce the likelihood, but I don't think it can
> completely avoid them.

It doesn't try to close the race window, actually we are doing post-fault
handling in patch [3/3].

>
> I don't see any acknowledgement of that in the commit logs of this
> series.  E.g., it doesn't say "we can recover from ATS Invalidate
> Completion Timeouts, but the timeouts are on the order of minutes, so
> we want to avoid them when possible."
> And given the "system hangs" language, I am not convinced that we
> actually *can* recover from those timeouts.

It is testing in customer's environment. separatly, patch[3/3] vs

patch [2/3].

>
> Using pci_dev_is_disconnected() may make those timeouts less frequent

will test patch[3/3] alone, if couldn't recover from the 100% ITE fault
case, we will look for other method.

Thanks,
Ethan

> and give the illusion that the problem is "solved", but it just means
> the problem is still there and harder to reproduce.
>
>> - During the device driver lifecycle when a device TLB invalidation
>>    timeout event is generated by the IOMMU hardware: This helps handle
>>    situations where the device might have been hot-removed.
>>
>> While there may be some adjustments needed in patch 3/3, I'd like to
>> confirm with you whether it's feasible to expose this helper for general
>> use within the iommu subsystem.
>>
>> If you agree with this change, I can route this patch to Linus through
>> the iommu tree with an "acked-by" or "reviewed-by" tag from you.
>>
>> Best regards,
>> baolu
  

On Thu, Feb 22, 2024 at 04:02:49AM -0500, Ethan Zhao wrote:
> Make pci_dev_is_disconnected() public so that it can be called from
> Intel VT-d driver to quickly fix/workaround the surprise removal
> unplug hang issue for those ATS capable devices on PCIe switch downstream
> hotplug capable ports.
> 
> Beside pci_device_is_present() function, this one has no config space
> space access, so is light enough to optimize the normal pure surprise
> removal and safe removal flow.
> 
> Tested-by: Haorong Ye <yehaorong@bytedance.com>
> Signed-off-by: Ethan Zhao <haifeng.zhao@linux.intel.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  drivers/pci/pci.h   | 5 -----
>  include/linux/pci.h | 5 +++++
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index e9750b1b19ba..bfc56f7bee1c 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -368,11 +368,6 @@ static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
>  	return 0;
>  }
>  
> -static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
> -{
> -	return dev->error_state == pci_channel_io_perm_failure;
> -}
> -
>  /* pci_dev priv_flags */
>  #define PCI_DEV_ADDED 0
>  #define PCI_DPC_RECOVERED 1
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 7ab0d13672da..213109d3c601 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -2517,6 +2517,11 @@ static inline struct pci_dev *pcie_find_root_port(struct pci_dev *dev)
>  	return NULL;
>  }
>  
> +static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
> +{
> +	return dev->error_state == pci_channel_io_perm_failure;
> +}
> +
>  void pci_request_acs(void);
>  bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
>  bool pci_acs_path_enabled(struct pci_dev *start,
> -- 
> 2.31.1
>
  

On Thu, Feb 29, 2024 at 09:58:43AM +0800, Ethan Zhao wrote:
> On 2/27/2024 7:05 AM, Bjorn Helgaas wrote:
> > On Thu, Feb 22, 2024 at 08:54:54PM +0800, Baolu Lu wrote:
> > > On 2024/2/22 17:02, Ethan Zhao wrote:
> > > > Make pci_dev_is_disconnected() public so that it can be called from
> > > > Intel VT-d driver to quickly fix/workaround the surprise removal
> > > > unplug hang issue for those ATS capable devices on PCIe switch downstream
> > > > hotplug capable ports.
> > > > 
> > > > Beside pci_device_is_present() function, this one has no config space
> > > > space access, so is light enough to optimize the normal pure surprise
> > > > removal and safe removal flow.
> > > > 
> > > > Tested-by: Haorong Ye<yehaorong@bytedance.com>
> > > > Signed-off-by: Ethan Zhao<haifeng.zhao@linux.intel.com>
> > > > ---
> > > >    drivers/pci/pci.h   | 5 -----
> > > >    include/linux/pci.h | 5 +++++
> > > >    2 files changed, 5 insertions(+), 5 deletions(-)
> > > Hi PCI subsystem maintainers,
> > > 
> > > The iommu drivers (including, but not limited to, the Intel VT-d driver)
> > > require a helper to check the physical presence of a PCI device in two
> > > scenarios:
> > > 
> > > - During the iommu_release_device() path: This ensures the device is
> > >    physically present before sending device TLB invalidation to device.
> > This wording is fundamentally wrong.  Testing
> > pci_dev_is_disconnected() can never ensure the device will still be
> > present by the time a TLB invalidation is sent.
> 
> The logic of testing pci_dev_is_disconnected() in patch [2/3] works
> in the opposite:
> 
> 1. if pci_dev_is_disconnected() return true, means the device is in
>    the process of surprise removal handling, adapter already been
>    removed from the slot.
> 
> 2. for removed device, no need to send ATS invalidation request to it.
>    removed device lost power, its devTLB wouldn't be valid anymore.
> 
> 3. if pci_dev_is_disconnected() return false, the device is *likely*
>    to be removed at any momoment after this function called.
>    such case will be treated in the iommu ITE fault handling, not to
>    retry the timeout request if device isn't present (patch [3/3]).
> 
> > The device may be removed after the pci_dev_is_disconnected() test and
> > before a TLB invalidate is sent.
> 
> even in the process while TLB is invalidating.
> 
> > This is why I hesitate to expose pci_dev_is_disconnected() (and
> > pci_device_is_present(), which we already export) outside
> > drivers/pci/.  They both lead to terrible mistakes like relying on the
> > false assumption that the result will remain valid after the functions
> > return, without any recognition that we MUST be able to deal with the
> > cases where that assumption is broken.
> 
> Yup, your concern is worthy ,but isn't happening within this patchset.

OK, I acked the patch.

I guess my complaint is really with pci_device_is_present() because
that's even harder to use correctly.

pci_device_is_present():
  slow (may do config access to device)
  true  => device *was* present in the recent past, may not be now
  false => device is not accessible

pci_dev_is_disconnected():
  fast (doesn't touch device)
  true  => device is not accessible
  false => basically means nothing

I guess they're both hard ;)

Bjorn
  

On 3/1/2024 6:33 AM, Bjorn Helgaas wrote:
> On Thu, Feb 29, 2024 at 09:58:43AM +0800, Ethan Zhao wrote:
>> On 2/27/2024 7:05 AM, Bjorn Helgaas wrote:
>>> On Thu, Feb 22, 2024 at 08:54:54PM +0800, Baolu Lu wrote:
>>>> On 2024/2/22 17:02, Ethan Zhao wrote:
>>>>> Make pci_dev_is_disconnected() public so that it can be called from
>>>>> Intel VT-d driver to quickly fix/workaround the surprise removal
>>>>> unplug hang issue for those ATS capable devices on PCIe switch downstream
>>>>> hotplug capable ports.
>>>>>
>>>>> Beside pci_device_is_present() function, this one has no config space
>>>>> space access, so is light enough to optimize the normal pure surprise
>>>>> removal and safe removal flow.
>>>>>
>>>>> Tested-by: Haorong Ye<yehaorong@bytedance.com>
>>>>> Signed-off-by: Ethan Zhao<haifeng.zhao@linux.intel.com>
>>>>> ---
>>>>>     drivers/pci/pci.h   | 5 -----
>>>>>     include/linux/pci.h | 5 +++++
>>>>>     2 files changed, 5 insertions(+), 5 deletions(-)
>>>> Hi PCI subsystem maintainers,
>>>>
>>>> The iommu drivers (including, but not limited to, the Intel VT-d driver)
>>>> require a helper to check the physical presence of a PCI device in two
>>>> scenarios:
>>>>
>>>> - During the iommu_release_device() path: This ensures the device is
>>>>     physically present before sending device TLB invalidation to device.
>>> This wording is fundamentally wrong.  Testing
>>> pci_dev_is_disconnected() can never ensure the device will still be
>>> present by the time a TLB invalidation is sent.
>> The logic of testing pci_dev_is_disconnected() in patch [2/3] works
>> in the opposite:
>>
>> 1. if pci_dev_is_disconnected() return true, means the device is in
>>     the process of surprise removal handling, adapter already been
>>     removed from the slot.
>>
>> 2. for removed device, no need to send ATS invalidation request to it.
>>     removed device lost power, its devTLB wouldn't be valid anymore.
>>
>> 3. if pci_dev_is_disconnected() return false, the device is *likely*
>>     to be removed at any momoment after this function called.
>>     such case will be treated in the iommu ITE fault handling, not to
>>     retry the timeout request if device isn't present (patch [3/3]).
>>
>>> The device may be removed after the pci_dev_is_disconnected() test and
>>> before a TLB invalidate is sent.
>> even in the process while TLB is invalidating.
>>
>>> This is why I hesitate to expose pci_dev_is_disconnected() (and
>>> pci_device_is_present(), which we already export) outside
>>> drivers/pci/.  They both lead to terrible mistakes like relying on the
>>> false assumption that the result will remain valid after the functions
>>> return, without any recognition that we MUST be able to deal with the
>>> cases where that assumption is broken.
>> Yup, your concern is worthy ,but isn't happening within this patchset.
> OK, I acked the patch.

Great !

>
> I guess my complaint is really with pci_device_is_present() because
> that's even harder to use correctly.
>
> pci_device_is_present():
>    slow (may do config access to device)
>    true  => device *was* present in the recent past, may not be now
>    false => device is not accessible

so the 'false' result is reliable for post-calling code, then give up
more attempt of the same request. the usage in patch[3/3]

>
> pci_dev_is_disconnected():
>    fast (doesn't touch device)
>    true  => device is not accessible

also we are relying on the 'true' result returned, used in  patch[2/3].

>    false => basically means nothing
>
> I guess they're both hard ;)

seems I didn't mess them up. :)

Thanks,
Ethan

>
> Bjorn