[PATCH 0/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[PATCH 0/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Thomas Tai]

Hi Bjorn, Keith and Oza,

I want to submit a patch to fix the
commit bd91b56cb3b27492963caeb5fccefe20a986ca8d
PCI/AER: Work around use-after-free in pcie_do_fatal_recovery()

Would you kindly have a look to see if it make sense to you
or not?

Thank you very much,
Thomas

[PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Thomas Tai]

In order to prevent the pcie_do_fatal_recovery() from
using the device after it is removed, the device's
domain:bus:devfn is stored at the entry of
pcie_do_fatal_recovery(). After rescanning the bus, the
stored domain:bus:devfn is used to find the device and
uses to report pci_info. The original issue only happens
on an non-bridge device, a local variable is used instead
of checking the device's header type.

Signed-off-by: Thomas Tai <thomas.tai@oracle.com>
---
 drivers/pci/pcie/err.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index f02e334..3414445 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -287,15 +287,20 @@ void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
 	struct pci_bus *parent;
 	struct pci_dev *pdev, *temp;
 	pci_ers_result_t result;
+	bool is_bridge_device = false;
+	u16 domain = pci_domain_nr(dev->bus);
+	u8 bus = dev->bus->number;
+	u8 devfn = dev->devfn;
 
-	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
+	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
+		is_bridge_device = true;
 		udev = dev;
-	else
+	} else {
 		udev = dev->bus->self;
+	}
 
 	parent = udev->subordinate;
 	pci_lock_rescan_remove();
-	pci_dev_get(dev);
 	list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
 					 bus_list) {
 		pci_dev_get(pdev);
@@ -309,27 +314,35 @@ void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
 
 	result = reset_link(udev, service);
 
-	if ((service == PCIE_PORT_SERVICE_AER) &&
-	    (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)) {
+	if (service == PCIE_PORT_SERVICE_AER && is_bridge_device) {
 		/*
 		 * If the error is reported by a bridge, we think this error
 		 * is related to the downstream link of the bridge, so we
 		 * do error recovery on all subordinates of the bridge instead
 		 * of the bridge and clear the error status of the bridge.
 		 */
-		pci_cleanup_aer_uncorrect_error_status(dev);
+		pci_cleanup_aer_uncorrect_error_status(udev);
 	}
 
 	if (result == PCI_ERS_RESULT_RECOVERED) {
 		if (pcie_wait_for_link(udev, true))
 			pci_rescan_bus(udev->bus);
-		pci_info(dev, "Device recovery from fatal error successful\n");
+		/* find the pci_dev after rescanning the bus */
+		dev = pci_get_domain_bus_and_slot(domain, bus, devfn);
+		if (dev)
+			pci_info(dev, "Device recovery from fatal error successful\n");
+		else
+			pr_err("AER: Can not find pci_dev for %04x:%02x:%02x.%x\n",
+			       domain, bus,
+			       PCI_SLOT(devfn), PCI_FUNC(devfn));
+		pci_dev_put(dev);
 	} else {
-		pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
-		pci_info(dev, "Device recovery from fatal error failed\n");
+		if (is_bridge_device)
+			pci_uevent_ers(udev, PCI_ERS_RESULT_DISCONNECT);
+		pr_err("AER: Device %04x:%02x:%02x.%x recovery from fatal error failed\n",
+		       domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
 	}
 
-	pci_dev_put(dev);
 	pci_unlock_rescan_remove();
 }
 
-- 
1.8.3.1

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-13 22:21, Thomas Tai wrote:
> In order to prevent the pcie_do_fatal_recovery() from
> using the device after it is removed, the device's
> domain:bus:devfn is stored at the entry of
> pcie_do_fatal_recovery(). After rescanning the bus, the
> stored domain:bus:devfn is used to find the device and
> uses to report pci_info. The original issue only happens
> on an non-bridge device, a local variable is used instead
> of checking the device's header type.
> 
> Signed-off-by: Thomas Tai <thomas.tai@oracle.com>
> ---
>  drivers/pci/pcie/err.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index f02e334..3414445 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -287,15 +287,20 @@ void pcie_do_fatal_recovery(struct pci_dev *dev,
> u32 service)
>  	struct pci_bus *parent;
>  	struct pci_dev *pdev, *temp;
>  	pci_ers_result_t result;
> +	bool is_bridge_device = false;
> +	u16 domain = pci_domain_nr(dev->bus);
> +	u8 bus = dev->bus->number;
> +	u8 devfn = dev->devfn;
> 
> -	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
> +	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> +		is_bridge_device = true;
>  		udev = dev;
> -	else
> +	} else {
>  		udev = dev->bus->self;
> +	}
> 
>  	parent = udev->subordinate;
>  	pci_lock_rescan_remove();
> -	pci_dev_get(dev);
>  	list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
>  					 bus_list) {
>  		pci_dev_get(pdev);
> @@ -309,27 +314,35 @@ void pcie_do_fatal_recovery(struct pci_dev *dev,
> u32 service)
> 
>  	result = reset_link(udev, service);
> 
> -	if ((service == PCIE_PORT_SERVICE_AER) &&
> -	    (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)) {
> +	if (service == PCIE_PORT_SERVICE_AER && is_bridge_device) {
>  		/*
>  		 * If the error is reported by a bridge, we think this error
>  		 * is related to the downstream link of the bridge, so we
>  		 * do error recovery on all subordinates of the bridge instead
>  		 * of the bridge and clear the error status of the bridge.
>  		 */
> -		pci_cleanup_aer_uncorrect_error_status(dev);
> +		pci_cleanup_aer_uncorrect_error_status(udev);
>  	}
> 
>  	if (result == PCI_ERS_RESULT_RECOVERED) {
>  		if (pcie_wait_for_link(udev, true))
>  			pci_rescan_bus(udev->bus);
> -		pci_info(dev, "Device recovery from fatal error successful\n");
> +		/* find the pci_dev after rescanning the bus */
> +		dev = pci_get_domain_bus_and_slot(domain, bus, devfn);
one of the motivations was to remove and re-enumerate rather then going 
thorugh driver's recovery sequence
was; it might be possible that hotplug capable bridge, the device might 
have changed.
hence this check will fail
> +		if (dev)
> +			pci_info(dev, "Device recovery from fatal error successful\n");
> +		else
> +			pr_err("AER: Can not find pci_dev for %04x:%02x:%02x.%x\n",
> +			       domain, bus,
> +			       PCI_SLOT(devfn), PCI_FUNC(devfn));
> +		pci_dev_put(dev);
>  	} else {
> -		pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
> -		pci_info(dev, "Device recovery from fatal error failed\n");
> +		if (is_bridge_device)
previously there was no condition.. why is this required now ?
and it was doing on "dev" while you are doing it on "udev"
> +			pci_uevent_ers(udev, PCI_ERS_RESULT_DISCONNECT);
> +		pr_err("AER: Device %04x:%02x:%02x.%x recovery from fatal error 
> failed\n",
> +		       domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>  	}
> 
> -	pci_dev_put(dev);
>  	pci_unlock_rescan_remove();
>  }

Regards,
Oza.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-14 14:46, poza@codeaurora.org wrote:
> On 2018-08-13 22:21, Thomas Tai wrote:
>> In order to prevent the pcie_do_fatal_recovery() from
>> using the device after it is removed, the device's
>> domain:bus:devfn is stored at the entry of
>> pcie_do_fatal_recovery(). After rescanning the bus, the
>> stored domain:bus:devfn is used to find the device and
>> uses to report pci_info. The original issue only happens
>> on an non-bridge device, a local variable is used instead
>> of checking the device's header type.
>> 
>> Signed-off-by: Thomas Tai <thomas.tai@oracle.com>
>> ---
>>  drivers/pci/pcie/err.c | 33 +++++++++++++++++++++++----------
>>  1 file changed, 23 insertions(+), 10 deletions(-)
>> 
>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>> index f02e334..3414445 100644
>> --- a/drivers/pci/pcie/err.c
>> +++ b/drivers/pci/pcie/err.c
>> @@ -287,15 +287,20 @@ void pcie_do_fatal_recovery(struct pci_dev *dev,
>> u32 service)
>>  	struct pci_bus *parent;
>>  	struct pci_dev *pdev, *temp;
>>  	pci_ers_result_t result;
>> +	bool is_bridge_device = false;
>> +	u16 domain = pci_domain_nr(dev->bus);
>> +	u8 bus = dev->bus->number;
>> +	u8 devfn = dev->devfn;
>> 
>> -	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
>> +	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>> +		is_bridge_device = true;
>>  		udev = dev;
>> -	else
>> +	} else {
>>  		udev = dev->bus->self;
>> +	}
>> 
>>  	parent = udev->subordinate;
>>  	pci_lock_rescan_remove();
>> -	pci_dev_get(dev);
>>  	list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
>>  					 bus_list) {
>>  		pci_dev_get(pdev);
>> @@ -309,27 +314,35 @@ void pcie_do_fatal_recovery(struct pci_dev *dev,
>> u32 service)
>> 
>>  	result = reset_link(udev, service);
>> 
>> -	if ((service == PCIE_PORT_SERVICE_AER) &&
>> -	    (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)) {
>> +	if (service == PCIE_PORT_SERVICE_AER && is_bridge_device) {
>>  		/*
>>  		 * If the error is reported by a bridge, we think this error
>>  		 * is related to the downstream link of the bridge, so we
>>  		 * do error recovery on all subordinates of the bridge instead
>>  		 * of the bridge and clear the error status of the bridge.
>>  		 */
>> -		pci_cleanup_aer_uncorrect_error_status(dev);
>> +		pci_cleanup_aer_uncorrect_error_status(udev);
>>  	}
>> 
>>  	if (result == PCI_ERS_RESULT_RECOVERED) {
>>  		if (pcie_wait_for_link(udev, true))
>>  			pci_rescan_bus(udev->bus);
>> -		pci_info(dev, "Device recovery from fatal error successful\n");
>> +		/* find the pci_dev after rescanning the bus */
>> +		dev = pci_get_domain_bus_and_slot(domain, bus, devfn);
> one of the motivations was to remove and re-enumerate rather then
> going thorugh driver's recovery sequence
> was; it might be possible that hotplug capable bridge, the device
> might have changed.
> hence this check will fail
>> +		if (dev)
>> +			pci_info(dev, "Device recovery from fatal error successful\n");
>> +		else
>> +			pr_err("AER: Can not find pci_dev for %04x:%02x:%02x.%x\n",
>> +			       domain, bus,
>> +			       PCI_SLOT(devfn), PCI_FUNC(devfn));
>> +		pci_dev_put(dev);
>>  	} else {
>> -		pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
>> -		pci_info(dev, "Device recovery from fatal error failed\n");
>> +		if (is_bridge_device)
> previously there was no condition.. why is this required now ?
> and it was doing on "dev" while you are doing it on "udev"
is that the reason we dont watnt o use dev after it is removed ? instead 
do pci_uevent_ers on bridge ?
>> +			pci_uevent_ers(udev, PCI_ERS_RESULT_DISCONNECT);
>> +		pr_err("AER: Device %04x:%02x:%02x.%x recovery from fatal error 
>> failed\n",
>> +		       domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>>  	}
>> 
>> -	pci_dev_put(dev);
>>  	pci_unlock_rescan_remove();
>>  }
> 
> Regards,
> Oza.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Thomas Tai]

On 08/14/2018 05:22 AM, poza@codeaurora.org wrote:
> On 2018-08-14 14:46, poza@codeaurora.org wrote:
>> On 2018-08-13 22:21, Thomas Tai wrote:
>>> In order to prevent the pcie_do_fatal_recovery() from
>>> using the device after it is removed, the device's
>>> domain:bus:devfn is stored at the entry of
>>> pcie_do_fatal_recovery(). After rescanning the bus, the
>>> stored domain:bus:devfn is used to find the device and
>>> uses to report pci_info. The original issue only happens
>>> on an non-bridge device, a local variable is used instead
>>> of checking the device's header type.
>>>
>>> Signed-off-by: Thomas Tai <thomas.tai@oracle.com>
>>> ---
>>>  drivers/pci/pcie/err.c | 33 +++++++++++++++++++++++----------
>>>  1 file changed, 23 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>>> index f02e334..3414445 100644
>>> --- a/drivers/pci/pcie/err.c
>>> +++ b/drivers/pci/pcie/err.c
>>> @@ -287,15 +287,20 @@ void pcie_do_fatal_recovery(struct pci_dev *dev,
>>> u32 service)
>>>      struct pci_bus *parent;
>>>      struct pci_dev *pdev, *temp;
>>>      pci_ers_result_t result;
>>> +    bool is_bridge_device = false;
>>> +    u16 domain = pci_domain_nr(dev->bus);
>>> +    u8 bus = dev->bus->number;
>>> +    u8 devfn = dev->devfn;
>>>
>>> -    if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
>>> +    if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>>> +        is_bridge_device = true;
>>>          udev = dev;
>>> -    else
>>> +    } else {
>>>          udev = dev->bus->self;
>>> +    }
>>>
>>>      parent = udev->subordinate;
>>>      pci_lock_rescan_remove();
>>> -    pci_dev_get(dev);
>>>      list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
>>>                       bus_list) {
>>>          pci_dev_get(pdev);
>>> @@ -309,27 +314,35 @@ void pcie_do_fatal_recovery(struct pci_dev *dev,
>>> u32 service)
>>>
>>>      result = reset_link(udev, service);
>>>
>>> -    if ((service == PCIE_PORT_SERVICE_AER) &&
>>> -        (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)) {
>>> +    if (service == PCIE_PORT_SERVICE_AER && is_bridge_device) {
>>>          /*
>>>           * If the error is reported by a bridge, we think this error
>>>           * is related to the downstream link of the bridge, so we
>>>           * do error recovery on all subordinates of the bridge instead
>>>           * of the bridge and clear the error status of the bridge.
>>>           */
>>> -        pci_cleanup_aer_uncorrect_error_status(dev);
>>> +        pci_cleanup_aer_uncorrect_error_status(udev);
>>>      }
>>>
>>>      if (result == PCI_ERS_RESULT_RECOVERED) {
>>>          if (pcie_wait_for_link(udev, true))
>>>              pci_rescan_bus(udev->bus);
>>> -        pci_info(dev, "Device recovery from fatal error successful\n");
>>> +        /* find the pci_dev after rescanning the bus */
>>> +        dev = pci_get_domain_bus_and_slot(domain, bus, devfn);
>> one of the motivations was to remove and re-enumerate rather then
>> going thorugh driver's recovery sequence
>> was; it might be possible that hotplug capable bridge, the device
>> might have changed.
>> hence this check will fail

Thanks Oza for your information. Instead of searching for the dev_pci, 
should I just use the stored domain:bus:devfn to printout the info?

>>> +        if (dev)
>>> +            pci_info(dev, "Device recovery from fatal error 
>>> successful\n");
>>> +        else
>>> +            pr_err("AER: Can not find pci_dev for %04x:%02x:%02x.%x\n",
>>> +                   domain, bus,
>>> +                   PCI_SLOT(devfn), PCI_FUNC(devfn));
>>> +        pci_dev_put(dev);

That is, replace above with:
	
	pr_info("AER: Device %04x:%02x:%02x.%x recover from fatal error 
successful\n", domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));


>>>      } else {
>>> -        pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
>>> -        pci_info(dev, "Device recovery from fatal error failed\n");
>>> +        if (is_bridge_device)
>> previously there was no condition.. why is this required now ?
>> and it was doing on "dev" while you are doing it on "udev"
> is that the reason we dont watnt o use dev after it is removed ? instead 
> do pci_uevent_ers on bridge ?


Yes, that's exactly the reason. I should have written down the reason in 
the commit log. Here is the details explanation for the benefit of 
others. If the failing device is a bridge device, udev is equal to dev. 
So we can use udev in the pci_uevent_ers. But for non bridge device the 
device is already removed from the bus and thus can't send pci_uevent_ers.

Thank you,
Thomas

>>> +            pci_uevent_ers(udev, PCI_ERS_RESULT_DISCONNECT);
>>> +        pr_err("AER: Device %04x:%02x:%02x.%x recovery from fatal 
>>> error failed\n",
>>> +               domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>>>      }
>>>
>>> -    pci_dev_put(dev);
>>>      pci_unlock_rescan_remove();
>>>  }
>>
>> Regards,
>> Oza.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-14 19:21, Thomas Tai wrote:
> On 08/14/2018 05:22 AM, poza@codeaurora.org wrote:
>> On 2018-08-14 14:46, poza@codeaurora.org wrote:
>>> On 2018-08-13 22:21, Thomas Tai wrote:
>>>> In order to prevent the pcie_do_fatal_recovery() from
>>>> using the device after it is removed, the device's
>>>> domain:bus:devfn is stored at the entry of
>>>> pcie_do_fatal_recovery(). After rescanning the bus, the
>>>> stored domain:bus:devfn is used to find the device and
>>>> uses to report pci_info. The original issue only happens
>>>> on an non-bridge device, a local variable is used instead
>>>> of checking the device's header type.
>>>> 
>>>> Signed-off-by: Thomas Tai <thomas.tai@oracle.com>
>>>> ---
>>>>  drivers/pci/pcie/err.c | 33 +++++++++++++++++++++++----------
>>>>  1 file changed, 23 insertions(+), 10 deletions(-)
>>>> 
>>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>>>> index f02e334..3414445 100644
>>>> --- a/drivers/pci/pcie/err.c
>>>> +++ b/drivers/pci/pcie/err.c
>>>> @@ -287,15 +287,20 @@ void pcie_do_fatal_recovery(struct pci_dev 
>>>> *dev,
>>>> u32 service)
>>>>      struct pci_bus *parent;
>>>>      struct pci_dev *pdev, *temp;
>>>>      pci_ers_result_t result;
>>>> +    bool is_bridge_device = false;
>>>> +    u16 domain = pci_domain_nr(dev->bus);
>>>> +    u8 bus = dev->bus->number;
>>>> +    u8 devfn = dev->devfn;
>>>> 
>>>> -    if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
>>>> +    if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>>>> +        is_bridge_device = true;
>>>>          udev = dev;
>>>> -    else
>>>> +    } else {
>>>>          udev = dev->bus->self;
>>>> +    }
>>>> 
>>>>      parent = udev->subordinate;
>>>>      pci_lock_rescan_remove();
>>>> -    pci_dev_get(dev);
>>>>      list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
>>>>                       bus_list) {
>>>>          pci_dev_get(pdev);
>>>> @@ -309,27 +314,35 @@ void pcie_do_fatal_recovery(struct pci_dev 
>>>> *dev,
>>>> u32 service)
>>>> 
>>>>      result = reset_link(udev, service);
>>>> 
>>>> -    if ((service == PCIE_PORT_SERVICE_AER) &&
>>>> -        (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)) {
>>>> +    if (service == PCIE_PORT_SERVICE_AER && is_bridge_device) {
>>>>          /*
>>>>           * If the error is reported by a bridge, we think this 
>>>> error
>>>>           * is related to the downstream link of the bridge, so we
>>>>           * do error recovery on all subordinates of the bridge 
>>>> instead
>>>>           * of the bridge and clear the error status of the bridge.
>>>>           */
>>>> -        pci_cleanup_aer_uncorrect_error_status(dev);
>>>> +        pci_cleanup_aer_uncorrect_error_status(udev);
>>>>      }
>>>> 
>>>>      if (result == PCI_ERS_RESULT_RECOVERED) {
>>>>          if (pcie_wait_for_link(udev, true))
>>>>              pci_rescan_bus(udev->bus);
>>>> -        pci_info(dev, "Device recovery from fatal error 
>>>> successful\n");
>>>> +        /* find the pci_dev after rescanning the bus */
>>>> +        dev = pci_get_domain_bus_and_slot(domain, bus, devfn);
>>> one of the motivations was to remove and re-enumerate rather then
>>> going thorugh driver's recovery sequence
>>> was; it might be possible that hotplug capable bridge, the device
>>> might have changed.
>>> hence this check will fail
> 
> Thanks Oza for your information. Instead of searching for the dev_pci,
> should I just use the stored domain:bus:devfn to printout the info?
> 
>>>> +        if (dev)
>>>> +            pci_info(dev, "Device recovery from fatal error 
>>>> successful\n");
>>>> +        else
>>>> +            pr_err("AER: Can not find pci_dev for 
>>>> %04x:%02x:%02x.%x\n",
>>>> +                   domain, bus,
>>>> +                   PCI_SLOT(devfn), PCI_FUNC(devfn));
>>>> +        pci_dev_put(dev);
> 
> That is, replace above with:
> 
> 	pr_info("AER: Device %04x:%02x:%02x.%x recover from fatal error
> successful\n", domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
> 
is this not sufficient to print ?  which is there already.
pci_info(dev, "Device recovery from fatal error successful\n");

> 
>>>>      } else {
>>>> -        pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
>>>> -        pci_info(dev, "Device recovery from fatal error failed\n");
>>>> +        if (is_bridge_device)
>>> previously there was no condition.. why is this required now ?
>>> and it was doing on "dev" while you are doing it on "udev"
>> is that the reason we dont watnt o use dev after it is removed ? 
>> instead do pci_uevent_ers on bridge ?
> 
> 
> Yes, that's exactly the reason. I should have written down the reason
> in the commit log. Here is the details explanation for the benefit of
> others. If the failing device is a bridge device, udev is equal to
> dev. So we can use udev in the pci_uevent_ers. But for non bridge
> device the device is already removed from the bus and thus can't send
> pci_uevent_ers.
> 
> Thank you,
> Thomas
> 
>>>> +            pci_uevent_ers(udev, PCI_ERS_RESULT_DISCONNECT);
>>>> +        pr_err("AER: Device %04x:%02x:%02x.%x recovery from fatal 
>>>> error failed\n",
>>>> +               domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>>>>      }
>>>> 
>>>> -    pci_dev_put(dev);
>>>>      pci_unlock_rescan_remove();
>>>>  }
>>> 
>>> Regards,
>>> Oza.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Thomas Tai]

On 08/15/2018 10:57 AM, poza@codeaurora.org wrote:
> On 2018-08-14 19:21, Thomas Tai wrote:
>> On 08/14/2018 05:22 AM, poza@codeaurora.org wrote:
>>> On 2018-08-14 14:46, poza@codeaurora.org wrote:
>>>> On 2018-08-13 22:21, Thomas Tai wrote:
>>>>> In order to prevent the pcie_do_fatal_recovery() from
>>>>> using the device after it is removed, the device's
>>>>> domain:bus:devfn is stored at the entry of
>>>>> pcie_do_fatal_recovery(). After rescanning the bus, the
>>>>> stored domain:bus:devfn is used to find the device and
>>>>> uses to report pci_info. The original issue only happens
>>>>> on an non-bridge device, a local variable is used instead
>>>>> of checking the device's header type.
>>>>>
>>>>> Signed-off-by: Thomas Tai <thomas.tai@oracle.com>
>>>>> ---
>>>>>  drivers/pci/pcie/err.c | 33 +++++++++++++++++++++++----------
>>>>>  1 file changed, 23 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>>>>> index f02e334..3414445 100644
>>>>> --- a/drivers/pci/pcie/err.c
>>>>> +++ b/drivers/pci/pcie/err.c
>>>>> @@ -287,15 +287,20 @@ void pcie_do_fatal_recovery(struct pci_dev *dev,
>>>>> u32 service)
>>>>>      struct pci_bus *parent;
>>>>>      struct pci_dev *pdev, *temp;
>>>>>      pci_ers_result_t result;
>>>>> +    bool is_bridge_device = false;
>>>>> +    u16 domain = pci_domain_nr(dev->bus);
>>>>> +    u8 bus = dev->bus->number;
>>>>> +    u8 devfn = dev->devfn;
>>>>>
>>>>> -    if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
>>>>> +    if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>>>>> +        is_bridge_device = true;
>>>>>          udev = dev;
>>>>> -    else
>>>>> +    } else {
>>>>>          udev = dev->bus->self;
>>>>> +    }
>>>>>
>>>>>      parent = udev->subordinate;
>>>>>      pci_lock_rescan_remove();
>>>>> -    pci_dev_get(dev);
>>>>>      list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
>>>>>                       bus_list) {
>>>>>          pci_dev_get(pdev);
>>>>> @@ -309,27 +314,35 @@ void pcie_do_fatal_recovery(struct pci_dev *dev,
>>>>> u32 service)
>>>>>
>>>>>      result = reset_link(udev, service);
>>>>>
>>>>> -    if ((service == PCIE_PORT_SERVICE_AER) &&
>>>>> -        (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)) {
>>>>> +    if (service == PCIE_PORT_SERVICE_AER && is_bridge_device) {
>>>>>          /*
>>>>>           * If the error is reported by a bridge, we think this error
>>>>>           * is related to the downstream link of the bridge, so we
>>>>>           * do error recovery on all subordinates of the bridge 
>>>>> instead
>>>>>           * of the bridge and clear the error status of the bridge.
>>>>>           */
>>>>> -        pci_cleanup_aer_uncorrect_error_status(dev);
>>>>> +        pci_cleanup_aer_uncorrect_error_status(udev);
>>>>>      }
>>>>>
>>>>>      if (result == PCI_ERS_RESULT_RECOVERED) {
>>>>>          if (pcie_wait_for_link(udev, true))
>>>>>              pci_rescan_bus(udev->bus);
>>>>> -        pci_info(dev, "Device recovery from fatal error 
>>>>> successful\n");
>>>>> +        /* find the pci_dev after rescanning the bus */
>>>>> +        dev = pci_get_domain_bus_and_slot(domain, bus, devfn);
>>>> one of the motivations was to remove and re-enumerate rather then
>>>> going thorugh driver's recovery sequence
>>>> was; it might be possible that hotplug capable bridge, the device
>>>> might have changed.
>>>> hence this check will fail
>>
>> Thanks Oza for your information. Instead of searching for the dev_pci,
>> should I just use the stored domain:bus:devfn to printout the info?
>>
>>>>> +        if (dev)
>>>>> +            pci_info(dev, "Device recovery from fatal error 
>>>>> successful\n");
>>>>> +        else
>>>>> +            pr_err("AER: Can not find pci_dev for 
>>>>> %04x:%02x:%02x.%x\n",
>>>>> +                   domain, bus,
>>>>> +                   PCI_SLOT(devfn), PCI_FUNC(devfn));
>>>>> +        pci_dev_put(dev);
>>
>> That is, replace above with:
>>
>>     pr_info("AER: Device %04x:%02x:%02x.%x recover from fatal error
>> successful\n", domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>>
> is this not sufficient to print ?  which is there already.
> pci_info(dev, "Device recovery from fatal error successful\n");

Unfortunately, I can't use "dev" because as you said I can't 
pci_get_domain_slot to search for it after rescanning.

-T

> 
>>
>>>>>      } else {
>>>>> -        pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
>>>>> -        pci_info(dev, "Device recovery from fatal error failed\n");
>>>>> +        if (is_bridge_device)
>>>> previously there was no condition.. why is this required now ?
>>>> and it was doing on "dev" while you are doing it on "udev"
>>> is that the reason we dont watnt o use dev after it is removed ? 
>>> instead do pci_uevent_ers on bridge ?
>>
>>
>> Yes, that's exactly the reason. I should have written down the reason
>> in the commit log. Here is the details explanation for the benefit of
>> others. If the failing device is a bridge device, udev is equal to
>> dev. So we can use udev in the pci_uevent_ers. But for non bridge
>> device the device is already removed from the bus and thus can't send
>> pci_uevent_ers.
>>
>> Thank you,
>> Thomas
>>
>>>>> +            pci_uevent_ers(udev, PCI_ERS_RESULT_DISCONNECT);
>>>>> +        pr_err("AER: Device %04x:%02x:%02x.%x recovery from fatal 
>>>>> error failed\n",
>>>>> +               domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>>>>>      }
>>>>>
>>>>> -    pci_dev_put(dev);
>>>>>      pci_unlock_rescan_remove();
>>>>>  }
>>>>
>>>> Regards,
>>>> Oza.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-15 20:32, Thomas Tai wrote:
> On 08/15/2018 10:57 AM, poza@codeaurora.org wrote:
>> On 2018-08-14 19:21, Thomas Tai wrote:
>>> On 08/14/2018 05:22 AM, poza@codeaurora.org wrote:
>>>> On 2018-08-14 14:46, poza@codeaurora.org wrote:
>>>>> On 2018-08-13 22:21, Thomas Tai wrote:
>>>>>> In order to prevent the pcie_do_fatal_recovery() from
>>>>>> using the device after it is removed, the device's
>>>>>> domain:bus:devfn is stored at the entry of
>>>>>> pcie_do_fatal_recovery(). After rescanning the bus, the
>>>>>> stored domain:bus:devfn is used to find the device and
>>>>>> uses to report pci_info. The original issue only happens
>>>>>> on an non-bridge device, a local variable is used instead
>>>>>> of checking the device's header type.
>>>>>> 
>>>>>> Signed-off-by: Thomas Tai <thomas.tai@oracle.com>
>>>>>> ---
>>>>>>  drivers/pci/pcie/err.c | 33 +++++++++++++++++++++++----------
>>>>>>  1 file changed, 23 insertions(+), 10 deletions(-)
>>>>>> 
>>>>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>>>>>> index f02e334..3414445 100644
>>>>>> --- a/drivers/pci/pcie/err.c
>>>>>> +++ b/drivers/pci/pcie/err.c
>>>>>> @@ -287,15 +287,20 @@ void pcie_do_fatal_recovery(struct pci_dev 
>>>>>> *dev,
>>>>>> u32 service)
>>>>>>      struct pci_bus *parent;
>>>>>>      struct pci_dev *pdev, *temp;
>>>>>>      pci_ers_result_t result;
>>>>>> +    bool is_bridge_device = false;
>>>>>> +    u16 domain = pci_domain_nr(dev->bus);
>>>>>> +    u8 bus = dev->bus->number;
>>>>>> +    u8 devfn = dev->devfn;
>>>>>> 
>>>>>> -    if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
>>>>>> +    if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>>>>>> +        is_bridge_device = true;
>>>>>>          udev = dev;
>>>>>> -    else
>>>>>> +    } else {
>>>>>>          udev = dev->bus->self;
>>>>>> +    }
>>>>>> 
>>>>>>      parent = udev->subordinate;
>>>>>>      pci_lock_rescan_remove();
>>>>>> -    pci_dev_get(dev);
>>>>>>      list_for_each_entry_safe_reverse(pdev, temp, 
>>>>>> &parent->devices,
>>>>>>                       bus_list) {
>>>>>>          pci_dev_get(pdev);
>>>>>> @@ -309,27 +314,35 @@ void pcie_do_fatal_recovery(struct pci_dev 
>>>>>> *dev,
>>>>>> u32 service)
>>>>>> 
>>>>>>      result = reset_link(udev, service);
>>>>>> 
>>>>>> -    if ((service == PCIE_PORT_SERVICE_AER) &&
>>>>>> -        (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)) {
>>>>>> +    if (service == PCIE_PORT_SERVICE_AER && is_bridge_device) {
>>>>>>          /*
>>>>>>           * If the error is reported by a bridge, we think this 
>>>>>> error
>>>>>>           * is related to the downstream link of the bridge, so we
>>>>>>           * do error recovery on all subordinates of the bridge 
>>>>>> instead
>>>>>>           * of the bridge and clear the error status of the 
>>>>>> bridge.
>>>>>>           */
>>>>>> -        pci_cleanup_aer_uncorrect_error_status(dev);
>>>>>> +        pci_cleanup_aer_uncorrect_error_status(udev);
>>>>>>      }
>>>>>> 
>>>>>>      if (result == PCI_ERS_RESULT_RECOVERED) {
>>>>>>          if (pcie_wait_for_link(udev, true))
>>>>>>              pci_rescan_bus(udev->bus);
>>>>>> -        pci_info(dev, "Device recovery from fatal error 
>>>>>> successful\n");
>>>>>> +        /* find the pci_dev after rescanning the bus */
>>>>>> +        dev = pci_get_domain_bus_and_slot(domain, bus, devfn);
>>>>> one of the motivations was to remove and re-enumerate rather then
>>>>> going thorugh driver's recovery sequence
>>>>> was; it might be possible that hotplug capable bridge, the device
>>>>> might have changed.
>>>>> hence this check will fail
>>> 
>>> Thanks Oza for your information. Instead of searching for the 
>>> dev_pci,
>>> should I just use the stored domain:bus:devfn to printout the info?
>>> 
>>>>>> +        if (dev)
>>>>>> +            pci_info(dev, "Device recovery from fatal error 
>>>>>> successful\n");
>>>>>> +        else
>>>>>> +            pr_err("AER: Can not find pci_dev for 
>>>>>> %04x:%02x:%02x.%x\n",
>>>>>> +                   domain, bus,
>>>>>> +                   PCI_SLOT(devfn), PCI_FUNC(devfn));
>>>>>> +        pci_dev_put(dev);
>>> 
>>> That is, replace above with:
>>> 
>>>     pr_info("AER: Device %04x:%02x:%02x.%x recover from fatal error
>>> successful\n", domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>>> 
>> is this not sufficient to print ?  which is there already.
>> pci_info(dev, "Device recovery from fatal error successful\n");
> 
> Unfortunately, I can't use "dev" because as you said I can't
> pci_get_domain_slot to search for it after rescanning.
> 
> -T

If I understood the patch correctly,
1) it is restructuring of pci_dev_put(dev);
2) now we are sending uevent only if it is bridge.
3) the other logic where you store and compare bdf is not required, 
although we have to fix following.
pci_info(dev, "Device recovery from fatal error successful\n");


probably 1) and 2) could be a separate patch.

by the way,
In my testing even after removing device the call pci_uevent_ers(dev, 
PCI_ERS_RESULT_DISCONNECT); did not create any problem.
> 

> 
>> 
>>> 
>>>>>>      } else {
>>>>>> -        pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
>>>>>> -        pci_info(dev, "Device recovery from fatal error 
>>>>>> failed\n");
>>>>>> +        if (is_bridge_device)
>>>>> previously there was no condition.. why is this required now ?
>>>>> and it was doing on "dev" while you are doing it on "udev"
>>>> is that the reason we dont watnt o use dev after it is removed ? 
>>>> instead do pci_uevent_ers on bridge ?
>>> 
>>> 
>>> Yes, that's exactly the reason. I should have written down the reason
>>> in the commit log. Here is the details explanation for the benefit of
>>> others. If the failing device is a bridge device, udev is equal to
>>> dev. So we can use udev in the pci_uevent_ers. But for non bridge
>>> device the device is already removed from the bus and thus can't send
>>> pci_uevent_ers.
>>> 
>>> Thank you,
>>> Thomas
>>> 
>>>>>> +            pci_uevent_ers(udev, PCI_ERS_RESULT_DISCONNECT);
>>>>>> +        pr_err("AER: Device %04x:%02x:%02x.%x recovery from fatal 
>>>>>> error failed\n",
>>>>>> +               domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>>>>>>      }
>>>>>> 
>>>>>> -    pci_dev_put(dev);
>>>>>>      pci_unlock_rescan_remove();
>>>>>>  }
>>>>> 
>>>>> Regards,
>>>>> Oza.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Thomas Tai]

[ ... ]>>>>>>> +        if (dev)
>>>>>>> +            pci_info(dev, "Device recovery from fatal error 
>>>>>>> successful\n");
>>>>>>> +        else
>>>>>>> +            pr_err("AER: Can not find pci_dev for 
>>>>>>> %04x:%02x:%02x.%x\n",
>>>>>>> +                   domain, bus,
>>>>>>> +                   PCI_SLOT(devfn), PCI_FUNC(devfn));
>>>>>>> +        pci_dev_put(dev);
>>>>
>>>> That is, replace above with:
>>>>
>>>>     pr_info("AER: Device %04x:%02x:%02x.%x recover from fatal error
>>>> successful\n", domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>>>>
>>> is this not sufficient to print ?  which is there already.
>>> pci_info(dev, "Device recovery from fatal error successful\n");
>>
>> Unfortunately, I can't use "dev" because as you said I can't
>> pci_get_domain_slot to search for it after rescanning.
>>
>> -T
> 
> If I understood the patch correctly,
> 1) it is restructuring of pci_dev_put(dev);
> 2) now we are sending uevent only if it is bridge.
> 3) the other logic where you store and compare bdf is not required, 
> although we have to fix following.
> pci_info(dev, "Device recovery from fatal error successful\n");
> 
> 
> probably 1) and 2) could be a separate patch.

Yes Oza. I will separate 1) and 2) and fix the pci_info(dev, "...") to 
use domain:bus:devfn

> 
> by the way,
> In my testing even after removing device the call pci_uevent_ers(dev, 
> PCI_ERS_RESULT_DISCONNECT); did not create any problem.

I think that is dangerous, if we restructure pci_dev_put(dev), ie revert 
commit bd91b56cb3b27492963caeb5fccefe20a986ca8d. The dev structure is 
freed and we shouldn't really use it to send pci_uevent_ers(). Although 
pci_uevent_ers() may be smart enough to figure out to avoid crash, we 
should only use it when the dev is not removed.

Thanks,
Thomas

>>
> 
>>
>>>
>>>>
>>>>>>>      } else {
>>>>>>> -        pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
>>>>>>> -        pci_info(dev, "Device recovery from fatal error failed\n");
>>>>>>> +        if (is_bridge_device)
>>>>>> previously there was no condition.. why is this required now ?
>>>>>> and it was doing on "dev" while you are doing it on "udev"
>>>>> is that the reason we dont watnt o use dev after it is removed ? 
>>>>> instead do pci_uevent_ers on bridge ?
>>>>
>>>>
>>>> Yes, that's exactly the reason. I should have written down the reason
>>>> in the commit log. Here is the details explanation for the benefit of
>>>> others. If the failing device is a bridge device, udev is equal to
>>>> dev. So we can use udev in the pci_uevent_ers. But for non bridge
>>>> device the device is already removed from the bus and thus can't send
>>>> pci_uevent_ers.
>>>>
>>>> Thank you,
>>>> Thomas
>>>>
>>>>>>> +            pci_uevent_ers(udev, PCI_ERS_RESULT_DISCONNECT);
>>>>>>> +        pr_err("AER: Device %04x:%02x:%02x.%x recovery from 
>>>>>>> fatal error failed\n",
>>>>>>> +               domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>>>>>>>      }
>>>>>>>
>>>>>>> -    pci_dev_put(dev);
>>>>>>>      pci_unlock_rescan_remove();
>>>>>>>  }
>>>>>>
>>>>>> Regards,
>>>>>> Oza.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-15 21:13, Thomas Tai wrote:
> [ ... ]>>>>>>> +        if (dev)
>>>>>>>> +            pci_info(dev, "Device recovery from fatal error 
>>>>>>>> successful\n");
>>>>>>>> +        else
>>>>>>>> +            pr_err("AER: Can not find pci_dev for 
>>>>>>>> %04x:%02x:%02x.%x\n",
>>>>>>>> +                   domain, bus,
>>>>>>>> +                   PCI_SLOT(devfn), PCI_FUNC(devfn));
>>>>>>>> +        pci_dev_put(dev);
>>>>> 
>>>>> That is, replace above with:
>>>>> 
>>>>>     pr_info("AER: Device %04x:%02x:%02x.%x recover from fatal error
>>>>> successful\n", domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>>>>> 
>>>> is this not sufficient to print ?  which is there already.
>>>> pci_info(dev, "Device recovery from fatal error successful\n");
>>> 
>>> Unfortunately, I can't use "dev" because as you said I can't
>>> pci_get_domain_slot to search for it after rescanning.
>>> 
>>> -T
>> 
>> If I understood the patch correctly,
>> 1) it is restructuring of pci_dev_put(dev);
>> 2) now we are sending uevent only if it is bridge.
>> 3) the other logic where you store and compare bdf is not required, 
>> although we have to fix following.
>> pci_info(dev, "Device recovery from fatal error successful\n");
>> 
>> 
>> probably 1) and 2) could be a separate patch.
> 
> Yes Oza. I will separate 1) and 2) and fix the pci_info(dev, "...") to
> use domain:bus:devfn

Thanks Thomas,
although I am not sure on 3)  by doing following.

  dev = pci_get_domain_bus_and_slot(domain, bus, devfn);
      if (dev)
         pci_info(dev, "Device recovery from fatal error successful\n");

but also it may be the case that device is removed and there is no new 
device which would match BDF
so in any case recovery has happened as far as PCIe link is concerned.

surprisingly in my case followin also went fine
pci_info(dev, "Device recovery from fatal error successful\n");
I guess probably after re-enumeration, I got the sane dev back !!  
(since it enumerated the same device)

how about something like this

dev = pci_get_domain_bus_and_slot(domain, bus, devfn);
      if (dev)
         pci_info(dev, "Device recovery from fatal error successful\n");
       else
          pr_ifo ("AER: Device recovery from fatal error successful")

Regards,
Oza.

> 
>> 
>> by the way,
>> In my testing even after removing device the call pci_uevent_ers(dev, 
>> PCI_ERS_RESULT_DISCONNECT); did not create any problem.
> 
> I think that is dangerous, if we restructure pci_dev_put(dev), ie
> revert commit bd91b56cb3b27492963caeb5fccefe20a986ca8d. The dev
> structure is freed and we shouldn't really use it to send
> pci_uevent_ers(). Although pci_uevent_ers() may be smart enough to
> figure out to avoid crash, we should only use it when the dev is not
> removed.
> 

I Agree.

> Thanks,
> Thomas
> 
>>> 
>> 
>>> 
>>>> 
>>>>> 
>>>>>>>>      } else {
>>>>>>>> -        pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
>>>>>>>> -        pci_info(dev, "Device recovery from fatal error 
>>>>>>>> failed\n");
>>>>>>>> +        if (is_bridge_device)
>>>>>>> previously there was no condition.. why is this required now ?
>>>>>>> and it was doing on "dev" while you are doing it on "udev"
>>>>>> is that the reason we dont watnt o use dev after it is removed ? 
>>>>>> instead do pci_uevent_ers on bridge ?
>>>>> 
>>>>> 
>>>>> Yes, that's exactly the reason. I should have written down the 
>>>>> reason
>>>>> in the commit log. Here is the details explanation for the benefit 
>>>>> of
>>>>> others. If the failing device is a bridge device, udev is equal to
>>>>> dev. So we can use udev in the pci_uevent_ers. But for non bridge
>>>>> device the device is already removed from the bus and thus can't 
>>>>> send
>>>>> pci_uevent_ers.
>>>>> 
>>>>> Thank you,
>>>>> Thomas
>>>>> 
>>>>>>>> +            pci_uevent_ers(udev, PCI_ERS_RESULT_DISCONNECT);
>>>>>>>> +        pr_err("AER: Device %04x:%02x:%02x.%x recovery from 
>>>>>>>> fatal error failed\n",
>>>>>>>> +               domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>>>>>>>>      }
>>>>>>>> 
>>>>>>>> -    pci_dev_put(dev);
>>>>>>>>      pci_unlock_rescan_remove();
>>>>>>>>  }
>>>>>>> 
>>>>>>> Regards,
>>>>>>> Oza.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Thomas Tai]

On 08/15/2018 11:59 AM, poza@codeaurora.org wrote:
> On 2018-08-15 21:13, Thomas Tai wrote:
>> [ ... ]>>>>>>> +        if (dev)
>>>>>>>>> +            pci_info(dev, "Device recovery from fatal error 
>>>>>>>>> successful\n");
>>>>>>>>> +        else
>>>>>>>>> +            pr_err("AER: Can not find pci_dev for 
>>>>>>>>> %04x:%02x:%02x.%x\n",
>>>>>>>>> +                   domain, bus,
>>>>>>>>> +                   PCI_SLOT(devfn), PCI_FUNC(devfn));
>>>>>>>>> +        pci_dev_put(dev);
>>>>>>
>>>>>> That is, replace above with:
>>>>>>
>>>>>>     pr_info("AER: Device %04x:%02x:%02x.%x recover from fatal error
>>>>>> successful\n", domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>>>>>>
>>>>> is this not sufficient to print ?  which is there already.
>>>>> pci_info(dev, "Device recovery from fatal error successful\n");
>>>>
>>>> Unfortunately, I can't use "dev" because as you said I can't
>>>> pci_get_domain_slot to search for it after rescanning.
>>>>
>>>> -T
>>>
>>> If I understood the patch correctly,
>>> 1) it is restructuring of pci_dev_put(dev);
>>> 2) now we are sending uevent only if it is bridge.
>>> 3) the other logic where you store and compare bdf is not required, 
>>> although we have to fix following.
>>> pci_info(dev, "Device recovery from fatal error successful\n");
>>>
>>>
>>> probably 1) and 2) could be a separate patch.
>>
>> Yes Oza. I will separate 1) and 2) and fix the pci_info(dev, "...") to
>> use domain:bus:devfn
> 
> Thanks Thomas,
> although I am not sure on 3)  by doing following.
> 
>   dev = pci_get_domain_bus_and_slot(domain, bus, devfn);
>       if (dev)
>          pci_info(dev, "Device recovery from fatal error successful\n");
> 
> but also it may be the case that device is removed and there is no new 
> device which would match BDF
> so in any case recovery has happened as far as PCIe link is concerned.
> 
> surprisingly in my case followin also went fine
> pci_info(dev, "Device recovery from fatal error successful\n");
> I guess probably after re-enumeration, I got the sane dev back !! (since 
> it enumerated the same device)
> 
> how about something like this
> 
> dev = pci_get_domain_bus_and_slot(domain, bus, devfn);
>       if (dev)
>          pci_info(dev, "Device recovery from fatal error successful\n");
>        else
>           pr_ifo ("AER: Device recovery from fatal error successful")

That is cool! Thank you for your suggestion, I will do so in V2.

Thomas

> 
> Regards,
> Oza.
> 
>>
>>>
>>> by the way,
>>> In my testing even after removing device the call pci_uevent_ers(dev, 
>>> PCI_ERS_RESULT_DISCONNECT); did not create any problem.
>>
>> I think that is dangerous, if we restructure pci_dev_put(dev), ie
>> revert commit bd91b56cb3b27492963caeb5fccefe20a986ca8d. The dev
>> structure is freed and we shouldn't really use it to send
>> pci_uevent_ers(). Although pci_uevent_ers() may be smart enough to
>> figure out to avoid crash, we should only use it when the dev is not
>> removed.
>>
> 
> I Agree.
> 
>> Thanks,
>> Thomas
>>
>>>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>>>      } else {
>>>>>>>>> -        pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
>>>>>>>>> -        pci_info(dev, "Device recovery from fatal error 
>>>>>>>>> failed\n");
>>>>>>>>> +        if (is_bridge_device)
>>>>>>>> previously there was no condition.. why is this required now ?
>>>>>>>> and it was doing on "dev" while you are doing it on "udev"
>>>>>>> is that the reason we dont watnt o use dev after it is removed ? 
>>>>>>> instead do pci_uevent_ers on bridge ?
>>>>>>
>>>>>>
>>>>>> Yes, that's exactly the reason. I should have written down the reason
>>>>>> in the commit log. Here is the details explanation for the benefit of
>>>>>> others. If the failing device is a bridge device, udev is equal to
>>>>>> dev. So we can use udev in the pci_uevent_ers. But for non bridge
>>>>>> device the device is already removed from the bus and thus can't send
>>>>>> pci_uevent_ers.
>>>>>>
>>>>>> Thank you,
>>>>>> Thomas
>>>>>>
>>>>>>>>> +            pci_uevent_ers(udev, PCI_ERS_RESULT_DISCONNECT);
>>>>>>>>> +        pr_err("AER: Device %04x:%02x:%02x.%x recovery from 
>>>>>>>>> fatal error failed\n",
>>>>>>>>> +               domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>>>>>>>>>      }
>>>>>>>>>
>>>>>>>>> -    pci_dev_put(dev);
>>>>>>>>>      pci_unlock_rescan_remove();
>>>>>>>>>  }
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Oza.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Tue, 2018-08-14 at 14:52 +0530, poza@codeaurora.org wrote:
> > >       if (result == PCI_ERS_RESULT_RECOVERED) {
> > >               if (pcie_wait_for_link(udev, true))
> > >                       pci_rescan_bus(udev->bus);
> > > -            pci_info(dev, "Device recovery from fatal error successful\n");
> > > +            /* find the pci_dev after rescanning the bus */
> > > +            dev = pci_get_domain_bus_and_slot(domain, bus, devfn);
> > 
> > one of the motivations was to remove and re-enumerate rather then
> > going thorugh driver's recovery sequence
> > was; it might be possible that hotplug capable bridge, the device
> > might have changed.
> > hence this check will fail

Under what circumstances do you actually "unplug" the device ? We are
trying to cleanup/fix some of the PowerPC EEH code which is in a way
similar to AER, and we found that this unplug/replug, which we do if
the driver doesn't have recovery callbacks only, is causing more
problems than it solves.

We are moving toward instead unbinding the driver, resetting the
device, then re-binding the driver instead of unplug/replug.

Also why would you ever bypass the driver callbacks if the driver has
some ? The whole point is to keep the driver bound while resetting the
device (provided it has the right callbacks) so we don't lose the
linkage between stroage devices and mounted filesystems.

Cheers,
Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

(resent using my proper email address)

On Tue, 2018-08-14 at 14:52 +0530, poza@codeaurora.org wrote:
> > >       if (result == PCI_ERS_RESULT_RECOVERED) {
> > >               if (pcie_wait_for_link(udev, true))
> > >                       pci_rescan_bus(udev->bus);
> > > -            pci_info(dev, "Device recovery from fatal error successful\n");
> > > +            /* find the pci_dev after rescanning the bus */
> > > +            dev = pci_get_domain_bus_and_slot(domain, bus, devfn);
> > 
> > one of the motivations was to remove and re-enumerate rather then
> > going thorugh driver's recovery sequence
> > was; it might be possible that hotplug capable bridge, the device
> > might have changed.
> > hence this check will fail

Under what circumstances do you actually "unplug" the device ? We are
trying to cleanup/fix some of the PowerPC EEH code which is in a way
similar to AER, and we found that this unplug/replug, which we do if
the driver doesn't have recovery callbacks only, is causing more
problems than it solves.

We are moving toward instead unbinding the driver, resetting the
device, then re-binding the driver instead of unplug/replug.

Also why would you ever bypass the driver callbacks if the driver has
some ? The whole point is to keep the driver bound while resetting the
device (provided it has the right callbacks) so we don't lose the
linkage between stroage devices and mounted filesystems.

Cheers,
Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-16 03:26, Benjamin Herrenschmidt wrote:
> (resent using my proper email address)
> 
> On Tue, 2018-08-14 at 14:52 +0530, poza@codeaurora.org wrote:
>> > >       if (result == PCI_ERS_RESULT_RECOVERED) {
>> > >               if (pcie_wait_for_link(udev, true))
>> > >                       pci_rescan_bus(udev->bus);
>> > > -            pci_info(dev, "Device recovery from fatal error successful\n");
>> > > +            /* find the pci_dev after rescanning the bus */
>> > > +            dev = pci_get_domain_bus_and_slot(domain, bus, devfn);
>> >
>> > one of the motivations was to remove and re-enumerate rather then
>> > going thorugh driver's recovery sequence
>> > was; it might be possible that hotplug capable bridge, the device
>> > might have changed.
>> > hence this check will fail
> 
> Under what circumstances do you actually "unplug" the device ? We are
> trying to cleanup/fix some of the PowerPC EEH code which is in a way
> similar to AER, and we found that this unplug/replug, which we do if
> the driver doesn't have recovery callbacks only, is causing more
> problems than it solves.
> 
> We are moving toward instead unbinding the driver, resetting the
> device, then re-binding the driver instead of unplug/replug.
> 
> Also why would you ever bypass the driver callbacks if the driver has
> some ? The whole point is to keep the driver bound while resetting the
> device (provided it has the right callbacks) so we don't lose the
> linkage between stroage devices and mounted filesystems.
> 

there are two different paths we are taking, one is for ERR_FATAL and 
the other is for ERR_NONFATAL.
while AER and DPC converge into ERR_FATAL path; ERR_NONFATAL path is the 
one where AER and driver callbacks would come into picture.

since DPC does hw recovery of the link, and handles only ERR_FATAL, we 
do remove devices and re-enumerate them.
but if the error is no fatal, we will fall back to driver callbacks to 
initiate link error handling and recovery process.

under normal circumstances I do not see the some downstream devices 
would have been replaced in case of FATAL errors, but code might not 
want to assume that
same device is there, since we are removing downstream devices 
completely. so taking reference of old BDF and keeping it for later 
reference is not good idea.
although I think there are some parts of pcie_do_fatal_recovery need a 
fix, which Thomas is fixing is anyway.

so yes, I agree with you and we are talking about same thing e.g.
"
  We are moving toward instead unbinding the driver, resetting the
  device, then re-binding the driver instead of unplug/replug.
"

driver callbacks are very much there, please have a look at 
pcie_do_nonfatal_recovery().

refering Sepc: 6.2.2.2.1 Fatal Errors, where link is unreliable and it 
might need AER style reset of link or DPC style HW recovery
In both cases, the shutdown callbacks are expected to be called,
e.g.  some driver handle errors ERR_NONFATAL or FATAL in similar ways
e.g.
ioat_pcie_error_detected();  calls  ioat_shutdown(); in case of 
ERR_NONFATAL
otherwise ioat_shutdown() in case of ERR_FATAL.


> Cheers,
> Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Thu, 2018-08-16 at 12:06 +0530, poza@codeaurora.org wrote:
> 
> > Under what circumstances do you actually "unplug" the device ? We are
> > trying to cleanup/fix some of the PowerPC EEH code which is in a way
> > similar to AER, and we found that this unplug/replug, which we do if
> > the driver doesn't have recovery callbacks only, is causing more
> > problems than it solves.
> > 
> > We are moving toward instead unbinding the driver, resetting the
> > device, then re-binding the driver instead of unplug/replug.
> > 
> > Also why would you ever bypass the driver callbacks if the driver has
> > some ? The whole point is to keep the driver bound while resetting the
> > device (provided it has the right callbacks) so we don't lose the
> > linkage between stroage devices and mounted filesystems.
> > 
> 
> there are two different paths we are taking, one is for ERR_FATAL and 
> the other is for ERR_NONFATAL.

Ok. With EEH we have a much finer granularity but fundamentally we
don't care as long as we reset the device.

> while AER and DPC converge into ERR_FATAL path; ERR_NONFATAL path is the 
> one where AER and driver callbacks would come into picture.

I disagree. See below.

> since DPC does hw recovery of the link, and handles only ERR_FATAL, we 
> do remove devices and re-enumerate them.

What is "DPC" ?

> but if the error is no fatal, we will fall back to driver callbacks to 
> initiate link error handling and recovery process.

Hrm ... the callbacks were never about link errors, they were about
device errors of any kind and predate PCIe concept of links in fact.

They include iommu errors, HW errors, SW bugs etc...

> under normal circumstances I do not see the some downstream devices 
> would have been replaced in case of FATAL errors, but code might not 
> want to assume that
> same device is there, since we are removing downstream devices 
> completely. so taking reference of old BDF and keeping it for later 
> reference is not good idea.
> although I think there are some parts of pcie_do_fatal_recovery need a 
> fix, which Thomas is fixing is anyway.

I think you shouldn't be unplugging/replugging.

> so yes, I agree with you and we are talking about same thing e.g.
> "
>   We are moving toward instead unbinding the driver, resetting the
>   device, then re-binding the driver instead of unplug/replug.
> "
> 
> driver callbacks are very much there, please have a look at 
> pcie_do_nonfatal_recovery().

Even for fatal. Why on earth would you skip the driver callbacks ? Most
errors are fatal anyway.

For us, we don't really differenciate because in order to avoid any
form of propagation of bad data, our HW will freeze/isolate a device on
any error, whether it's fatal or non fatal.

So we treat them both as needing a full recovery which usually implies
a reset of the device.

However we do that through the use of the driver callbacks because
otherwise you will lose the linkage between a storage driver and the
moutned file systems which cannot be recovered, so you are killing your
system.

> refering Sepc: 6.2.2.2.1 Fatal Errors, where link is unreliable and it 
> might need AER style reset of link or DPC style HW recovery
> In both cases, the shutdown callbacks are expected to be called,

No, this is wrong and not the intent of the error handling.

You seem to be applying PCIe specific concepts brain-farted at Intel
that are way way away from what we care about in practice and in Linux.

> e.g.  some driver handle errors ERR_NONFATAL or FATAL in similar ways
> e.g.
> ioat_pcie_error_detected();  calls  ioat_shutdown(); in case of 
> ERR_NONFATAL
> otherwise ioat_shutdown() in case of ERR_FATAL.

Since when the error handling callbacks even have the concept of FATAL
vs. non-fatal ? This doesn't appear anyhwhere in the prototype of the
struct pci_error_handlers and shouldn't.

Benm=.

> 
> > Cheers,
> > Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Thu, 2018-08-16 at 16:51 +1000, Benjamin Herrenschmidt wrote:
> > refering Sepc: 6.2.2.2.1 Fatal Errors, where link is unreliable and it 
> > might need AER style reset of link or DPC style HW recovery
> > In both cases, the shutdown callbacks are expected to be called,
> 
> No, this is wrong and not the intent of the error handling.
> 
> You seem to be applying PCIe specific concepts brain-farted at Intel
> that are way way away from what we care about in practice and in Linux.
> 
> > e.g.  some driver handle errors ERR_NONFATAL or FATAL in similar ways
> > e.g.
> > ioat_pcie_error_detected();  calls  ioat_shutdown(); in case of 
> > ERR_NONFATAL
> > otherwise ioat_shutdown() in case of ERR_FATAL.
> 
> Since when the error handling callbacks even have the concept of FATAL
> vs. non-fatal ? This doesn't appear anyhwhere in the prototype of the
> struct pci_error_handlers and shouldn't.

In fact looking at pcie_do_nonfatal_recovery() it's indeed completely
broken. It tells the driver that the slot was reset without actually
resetting anything... Ugh.

Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-16 12:29, Benjamin Herrenschmidt wrote:
> On Thu, 2018-08-16 at 16:51 +1000, Benjamin Herrenschmidt wrote:
>> > refering Sepc: 6.2.2.2.1 Fatal Errors, where link is unreliable and it
>> > might need AER style reset of link or DPC style HW recovery
>> > In both cases, the shutdown callbacks are expected to be called,
>> 
>> No, this is wrong and not the intent of the error handling.
>> 
>> You seem to be applying PCIe specific concepts brain-farted at Intel
>> that are way way away from what we care about in practice and in 
>> Linux.
>> 
>> > e.g.  some driver handle errors ERR_NONFATAL or FATAL in similar ways
>> > e.g.
>> > ioat_pcie_error_detected();  calls  ioat_shutdown(); in case of
>> > ERR_NONFATAL
>> > otherwise ioat_shutdown() in case of ERR_FATAL.
>> 
>> Since when the error handling callbacks even have the concept of FATAL
>> vs. non-fatal ? This doesn't appear anyhwhere in the prototype of the
>> struct pci_error_handlers and shouldn't.
> 
> In fact looking at pcie_do_nonfatal_recovery() it's indeed completely
> broken. It tells the driver that the slot was reset without actually
> resetting anything... Ugh.
> 
> Ben.

pcie_do_nonfatal_recovery() exhibit the same behavior with or without 
the patch-series.
in short, there was no functional change brought in to 
pcie_do_nonfatal_recovery()

Regards,
Oza.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Thu, 2018-08-16 at 13:37 +0530, poza@codeaurora.org wrote:
> > 
> > In fact looking at pcie_do_nonfatal_recovery() it's indeed completely
> > broken. It tells the driver that the slot was reset without actually
> > resetting anything... Ugh.
> > 
> > Ben.
> 
> pcie_do_nonfatal_recovery() exhibit the same behavior with or without 
> the patch-series.
> in short, there was no functional change brought in to 
> pcie_do_nonfatal_recovery()

Yes, I know, I'm just saying what it does is broken :-)

Keep in mind that those callbacks were designed originally for EEH
(which predates AER), and so was the spec written.

We don't actually use the AER code on POWER today, so we didn't notice
how broken the implementation was :-)

We should fix that.

Either we can sort all that out by email, or we should plan some kind
of catch-up, either at Plumbers (provided I can go there) or maybe a
webex call.

Cheers,
Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-16 13:42, Benjamin Herrenschmidt wrote:
> On Thu, 2018-08-16 at 13:37 +0530, poza@codeaurora.org wrote:
>> >
>> > In fact looking at pcie_do_nonfatal_recovery() it's indeed completely
>> > broken. It tells the driver that the slot was reset without actually
>> > resetting anything... Ugh.
>> >
>> > Ben.
>> 
>> pcie_do_nonfatal_recovery() exhibit the same behavior with or without
>> the patch-series.
>> in short, there was no functional change brought in to
>> pcie_do_nonfatal_recovery()
> 
> Yes, I know, I'm just saying what it does is broken :-)


when I meant spec, i meant PCIe Spec.
At least spec distinguish fatal and non-fatal

"
Non-fatal errors are uncorrectable errors which cause a particular 
transaction to be unreliable but the Link is otherwise fully functional.
Isolating Non-fatal from Fatal errors provides Requester/Receiver logic 
in a device or system management software the opportunity to recover
from the error without resetting the components on the Link and 
disturbing other transactions in progress.
"

Here the basic assumption is link is fully functional, hence we do not 
initiate link reset. (while in case FATAL we do initiate Secondary Bus 
Reset)


okay, so here is what current pcie_do_nonfatal_recovery() doe.

pcie_do_nonfatal_recovery
     report_error_detected()    >> calls driver callbacks
     report_mmio_enabled()
     report_slot_reset()  >> if PCI_ERS_RESULT_NEED_RESET
     report_resume()

If you suggest how it is broken, it will help me to understand.
probably you might want to point out what are the calls need to be added 
or removed or differently handled, specially storage point of view.

Regards,
Oza.

> 
> Keep in mind that those callbacks were designed originally for EEH
> (which predates AER), and so was the spec written.
> 
> We don't actually use the AER code on POWER today, so we didn't notice
> how broken the implementation was :-)
> 
> We should fix that.
> 
> Either we can sort all that out by email, or we should plan some kind
> of catch-up, either at Plumbers (provided I can go there) or maybe a
> webex call.
> 
> Cheers,
> Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Thu, 2018-08-16 at 14:33 +0530, poza@codeaurora.org wrote:
> On 2018-08-16 13:42, Benjamin Herrenschmidt wrote:
> > 
> when I meant spec, i meant PCIe Spec.
> At least spec distinguish fatal and non-fatal

Yes, I'm well aware of that, however the policy implemented by EEH is
stricter in that *any* uncorrectable error will trigger an immediate
freeze of the endpoint in order to prevent bad data propagation.

However, you don't have to implement it that way for AER. You can
implement a policy where you don't enforce a reset of the device and
link unless the driver requests it.

However if/when the driver does, then you should honor the driver wish
and do it which isn't currently the case in the AER code.

Note that the current error callbacks have no way to convey the fatal
vs. non-fatal information to the device that I can see, we might want
to change the prototype here with a tree-wide patch if you think that
drivers might care.

> Non-fatal errors are uncorrectable errors which cause a particular 
> transaction to be unreliable but the Link is otherwise fully functional.
> Isolating Non-fatal from Fatal errors provides Requester/Receiver logic 
> in a device or system management software the opportunity to recover
> from the error without resetting the components on the Link and 
> disturbing other transactions in progress.
> "
> 
> Here the basic assumption is link is fully functional, hence we do not 
> initiate link reset. (while in case FATAL we do initiate Secondary Bus 
> Reset)

See above.
> 
> okay, so here is what current pcie_do_nonfatal_recovery() doe.
> 
> pcie_do_nonfatal_recovery
>      report_error_detected()    >> calls driver callbacks
>      report_mmio_enabled()
>      report_slot_reset()  >> if PCI_ERS_RESULT_NEED_RESET

Above if the driver returned "NEED RESET" we should not just "report" a
slot reset, we should *perform* one :-) Unless the AER code does it in
a place I missed...

Also we should do a hot reset at least, not just a link reset.

>      report_resume()
> 
> If you suggest how it is broken, it will help me to understand.
> probably you might want to point out what are the calls need to be added 
> or removed or differently handled, specially storage point of view.


> Regards,
> Oza.
> 
> > 
> > Keep in mind that those callbacks were designed originally for EEH
> > (which predates AER), and so was the spec written.
> > 
> > We don't actually use the AER code on POWER today, so we didn't notice
> > how broken the implementation was :-)
> > 
> > We should fix that.
> > 
> > Either we can sort all that out by email, or we should plan some kind
> > of catch-up, either at Plumbers (provided I can go there) or maybe a
> > webex call.
> > 
> > Cheers,
> > Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-16 15:37, Benjamin Herrenschmidt wrote:
> On Thu, 2018-08-16 at 14:33 +0530, poza@codeaurora.org wrote:
>> On 2018-08-16 13:42, Benjamin Herrenschmidt wrote:
>> >
>> when I meant spec, i meant PCIe Spec.
>> At least spec distinguish fatal and non-fatal
> 
> Yes, I'm well aware of that, however the policy implemented by EEH is
> stricter in that *any* uncorrectable error will trigger an immediate
> freeze of the endpoint in order to prevent bad data propagation.
> 
> However, you don't have to implement it that way for AER. You can
> implement a policy where you don't enforce a reset of the device and
> link unless the driver requests it.
> 
> However if/when the driver does, then you should honor the driver wish
> and do it which isn't currently the case in the AER code.
> 
> Note that the current error callbacks have no way to convey the fatal
> vs. non-fatal information to the device that I can see, we might want
> to change the prototype here with a tree-wide patch if you think that
> drivers might care.
> 
>> Non-fatal errors are uncorrectable errors which cause a particular
>> transaction to be unreliable but the Link is otherwise fully 
>> functional.
>> Isolating Non-fatal from Fatal errors provides Requester/Receiver 
>> logic
>> in a device or system management software the opportunity to recover
>> from the error without resetting the components on the Link and
>> disturbing other transactions in progress.
>> "
>> 
>> Here the basic assumption is link is fully functional, hence we do not
>> initiate link reset. (while in case FATAL we do initiate Secondary Bus
>> Reset)
> 
> See above.
>> 
>> okay, so here is what current pcie_do_nonfatal_recovery() doe.
>> 
>> pcie_do_nonfatal_recovery
>>      report_error_detected()    >> calls driver callbacks
>>      report_mmio_enabled()
>>      report_slot_reset()  >> if PCI_ERS_RESULT_NEED_RESET
> 
> Above if the driver returned "NEED RESET" we should not just "report" a
> slot reset, we should *perform* one :-) Unless the AER code does it in
> a place I missed...

I am willing work on this if Bjorn agrees.
but I am still trying to figure out missing piece.

so Ben,
you are suggesting

ERR_NONFATAL handling
pcie_do_nonfatal_recovery
      report_error_detected()    >> calls driver callbacks
       report_mmio_enabled()
       report_slot_reset()  >> if PCI_ERS_RESULT_NEED_RESET
Here along with calling slot_reset, you are suggesting to do Secondary 
Bus Reset ?

but this is ERR_NONFATAL and according to definition the link is still 
good, so that the transcriptions on PCIe link can happen.
so my question is why do we want to reset the link ?

although
I see following note in the code as well.
                 /*
		 * TODO: Should call platform-specific
		 * functions to reset slot before calling
		 * drivers' slot_reset callbacks?
		 */

Regards,
Oza.

> 
> Also we should do a hot reset at least, not just a link reset.
> 
>>      report_resume()
>> 
>> If you suggest how it is broken, it will help me to understand.
>> probably you might want to point out what are the calls need to be 
>> added
>> or removed or differently handled, specially storage point of view.
> 
> 
>> Regards,
>> Oza.
>> 
>> >
>> > Keep in mind that those callbacks were designed originally for EEH
>> > (which predates AER), and so was the spec written.
>> >
>> > We don't actually use the AER code on POWER today, so we didn't notice
>> > how broken the implementation was :-)
>> >
>> > We should fix that.
>> >
>> > Either we can sort all that out by email, or we should plan some kind
>> > of catch-up, either at Plumbers (provided I can go there) or maybe a
>> > webex call.
>> >
>> > Cheers,
>> > Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Thu, 2018-08-16 at 19:41 +0530, poza@codeaurora.org wrote:
> 
> > See above.
> > > 
> > > okay, so here is what current pcie_do_nonfatal_recovery() doe.
> > > 
> > > pcie_do_nonfatal_recovery
> > >      report_error_detected()    >> calls driver callbacks
> > >      report_mmio_enabled()
> > >      report_slot_reset()  >> if PCI_ERS_RESULT_NEED_RESET
> > 
> > Above if the driver returned "NEED RESET" we should not just "report" a
> > slot reset, we should *perform* one :-) Unless the AER code does it in
> > a place I missed...
> 
> I am willing work on this if Bjorn agrees.
> but I am still trying to figure out missing piece.
> 
> so Ben,
> you are suggesting
> 
> ERR_NONFATAL handling
> pcie_do_nonfatal_recovery
>       report_error_detected()    >> calls driver callbacks
>        report_mmio_enabled()
>        report_slot_reset()  >> if PCI_ERS_RESULT_NEED_RESET
> Here along with calling slot_reset, you are suggesting to do Secondary 
> Bus Reset ?
> 
> but this is ERR_NONFATAL and according to definition the link is still 
> good, so that the transcriptions on PCIe link can happen.
> so my question is why do we want to reset the link ?

The driver responded to you from error_detected() or mmio_enabled()
that it wants the slot to be reset. So you should do so. This comes
from the driver deciding that whatever happens, it doesn't trust the
device state anymore.

report_slot_reset() iirc is about telling the driver that the reset has
been performed and it can reconfigure the device.

To be frank I haven't looked at this in a while, so we should refer to
the document before ou patched it :-) But the basic design we created
back then was precisely that the driver would have the ultimate say in
the matter.

Also because multiple devices, at least on power, can share a domain, 
we can get into a situation where one device requires a reset, so all
will get reset, and their respective drivers need to be notified.

Note: it's not so much about resetting the *link* than about resetting
the *device*.

> although
> I see following note in the code as well.
>                  /*
> 		 * TODO: Should call platform-specific
> 		 * functions to reset slot before calling
> 		 * drivers' slot_reset callbacks?
> 		 */

Yup :-)

Cheers,
Ben.

> Regards,
> Oza.
> 
> > 
> > Also we should do a hot reset at least, not just a link reset.
> > 
> > >      report_resume()
> > > 
> > > If you suggest how it is broken, it will help me to understand.
> > > probably you might want to point out what are the calls need to be 
> > > added
> > > or removed or differently handled, specially storage point of view.
> > 
> > 
> > > Regards,
> > > Oza.
> > > 
> > > > 
> > > > Keep in mind that those callbacks were designed originally for EEH
> > > > (which predates AER), and so was the spec written.
> > > > 
> > > > We don't actually use the AER code on POWER today, so we didn't notice
> > > > how broken the implementation was :-)
> > > > 
> > > > We should fix that.
> > > > 
> > > > Either we can sort all that out by email, or we should plan some kind
> > > > of catch-up, either at Plumbers (provided I can go there) or maybe a
> > > > webex call.
> > > > 
> > > > Cheers,
> > > > Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-17 05:00, Benjamin Herrenschmidt wrote:
> On Thu, 2018-08-16 at 19:41 +0530, poza@codeaurora.org wrote:
>> 
>> > See above.
>> > >
>> > > okay, so here is what current pcie_do_nonfatal_recovery() doe.
>> > >
>> > > pcie_do_nonfatal_recovery
>> > >      report_error_detected()    >> calls driver callbacks
>> > >      report_mmio_enabled()
>> > >      report_slot_reset()  >> if PCI_ERS_RESULT_NEED_RESET
>> >
>> > Above if the driver returned "NEED RESET" we should not just "report" a
>> > slot reset, we should *perform* one :-) Unless the AER code does it in
>> > a place I missed...
>> 
>> I am willing work on this if Bjorn agrees.
>> but I am still trying to figure out missing piece.
>> 
>> so Ben,
>> you are suggesting
>> 
>> ERR_NONFATAL handling
>> pcie_do_nonfatal_recovery
>>       report_error_detected()    >> calls driver callbacks
>>        report_mmio_enabled()
>>        report_slot_reset()  >> if PCI_ERS_RESULT_NEED_RESET
>> Here along with calling slot_reset, you are suggesting to do Secondary
>> Bus Reset ?
>> 
>> but this is ERR_NONFATAL and according to definition the link is still
>> good, so that the transcriptions on PCIe link can happen.
>> so my question is why do we want to reset the link ?
> 
> The driver responded to you from error_detected() or mmio_enabled()
> that it wants the slot to be reset. So you should do so. This comes
> from the driver deciding that whatever happens, it doesn't trust the
> device state anymore.
> 
> report_slot_reset() iirc is about telling the driver that the reset has
> been performed and it can reconfigure the device.
> 
> To be frank I haven't looked at this in a while, so we should refer to
> the document before ou patched it :-) But the basic design we created
> back then was precisely that the driver would have the ultimate say in
> the matter.

The existing design is; framework dictating drivers what it should do 
rather than driver deciding.
let me explain.

aer_isr
     aer_isr_one_error
          get_device_error_info   >> this checks
               {
                 /* Link is still healthy for IO reads */    >> Bjorn 
this looks like a very strange thing to me, if link is not healthy we 
are not even going for pcie_do_fatal_recovery()
		pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS,
			&info->status);
		pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_MASK,
			&info->mask);
		if (!(info->status & ~info->mask))
			return 0;

               {
         handle_error_source
         pcie_do_nonfatal_recovery or pcie_do_fatal_recovery

now it does not seem to me that driver has some way of knowing if it has 
to return PCI_ERS_RESULT_CAN_RECOVER ? or PCI_ERS_RESULT_NEED_RESET?

let us take an example of storage driver here e.g. NVME
nvme_error_detected()  relies heavily on pci_channel_state_t which is 
passed by error framework  (err.c)

I understand your point of view and original intention of design that 
let driver dictate whether it wants slot_reset ?
but driver relies on framework to pass that information in order to take 
decision.

In case of pci_channel_io_frozen  (which is ERR_FATAL), most of the 
drivers demand link reset.

but in case of ERR_NONFATAL, the link is supposed to be functional and 
there is no need for link reset.
and exactly all the PCI based drivers returns 
PCI_ERS_RESULT_CAN_RECOVER, which is valid.


so I think to conclude, you are referring more of your views towards
pcie_do_fatal_recovery()


which does

> remove_devices from the downstream link
> reset_link()   (Secondary bus reset)
> re-enumerate.

and this is what DPC defined, and AER is just following that.

Regards,
Oza.

> 
> Also because multiple devices, at least on power, can share a domain,
> we can get into a situation where one device requires a reset, so all
> will get reset, and their respective drivers need to be notified.
> 
> Note: it's not so much about resetting the *link* than about resetting
> the *device*.
> 
>> although
>> I see following note in the code as well.
>>                  /*
>> 		 * TODO: Should call platform-specific
>> 		 * functions to reset slot before calling
>> 		 * drivers' slot_reset callbacks?
>> 		 */
> 
> Yup :-)
> 
> Cheers,
> Ben.
> 
>> Regards,
>> Oza.
>> 
>> >
>> > Also we should do a hot reset at least, not just a link reset.
>> >
>> > >      report_resume()
>> > >
>> > > If you suggest how it is broken, it will help me to understand.
>> > > probably you might want to point out what are the calls need to be
>> > > added
>> > > or removed or differently handled, specially storage point of view.
>> >
>> >
>> > > Regards,
>> > > Oza.
>> > >
>> > > >
>> > > > Keep in mind that those callbacks were designed originally for EEH
>> > > > (which predates AER), and so was the spec written.
>> > > >
>> > > > We don't actually use the AER code on POWER today, so we didn't notice
>> > > > how broken the implementation was :-)
>> > > >
>> > > > We should fix that.
>> > > >
>> > > > Either we can sort all that out by email, or we should plan some kind
>> > > > of catch-up, either at Plumbers (provided I can go there) or maybe a
>> > > > webex call.
>> > > >
>> > > > Cheers,
>> > > > Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-17 15:59, poza@codeaurora.org wrote:
> On 2018-08-17 05:00, Benjamin Herrenschmidt wrote:
>> On Thu, 2018-08-16 at 19:41 +0530, poza@codeaurora.org wrote:
>>> 
>>> > See above.
>>> > >
>>> > > okay, so here is what current pcie_do_nonfatal_recovery() doe.
>>> > >
>>> > > pcie_do_nonfatal_recovery
>>> > >      report_error_detected()    >> calls driver callbacks
>>> > >      report_mmio_enabled()
>>> > >      report_slot_reset()  >> if PCI_ERS_RESULT_NEED_RESET
>>> >
>>> > Above if the driver returned "NEED RESET" we should not just "report" a
>>> > slot reset, we should *perform* one :-) Unless the AER code does it in
>>> > a place I missed...
>>> 
>>> I am willing work on this if Bjorn agrees.
>>> but I am still trying to figure out missing piece.
>>> 
>>> so Ben,
>>> you are suggesting
>>> 
>>> ERR_NONFATAL handling
>>> pcie_do_nonfatal_recovery
>>>       report_error_detected()    >> calls driver callbacks
>>>        report_mmio_enabled()
>>>        report_slot_reset()  >> if PCI_ERS_RESULT_NEED_RESET
>>> Here along with calling slot_reset, you are suggesting to do 
>>> Secondary
>>> Bus Reset ?
>>> 
>>> but this is ERR_NONFATAL and according to definition the link is 
>>> still
>>> good, so that the transcriptions on PCIe link can happen.
>>> so my question is why do we want to reset the link ?
>> 
>> The driver responded to you from error_detected() or mmio_enabled()
>> that it wants the slot to be reset. So you should do so. This comes
>> from the driver deciding that whatever happens, it doesn't trust the
>> device state anymore.
>> 
>> report_slot_reset() iirc is about telling the driver that the reset 
>> has
>> been performed and it can reconfigure the device.
>> 
>> To be frank I haven't looked at this in a while, so we should refer to
>> the document before ou patched it :-) But the basic design we created
>> back then was precisely that the driver would have the ultimate say in
>> the matter.
> 
> The existing design is; framework dictating drivers what it should do
> rather than driver deciding.
> let me explain.
> 
> aer_isr
>     aer_isr_one_error
>          get_device_error_info   >> this checks
>               {
>                 /* Link is still healthy for IO reads */    >> Bjorn
> this looks like a very strange thing to me, if link is not healthy we
> are not even going for pcie_do_fatal_recovery()

I misread mask as severity, this code is ok, it just does not process 
masked errors.

> 		pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS,
> 			&info->status);
> 		pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_MASK,
> 			&info->mask);
> 		if (!(info->status & ~info->mask))
> 			return 0;
> 
>               {
>         handle_error_source
>         pcie_do_nonfatal_recovery or pcie_do_fatal_recovery
> 
> now it does not seem to me that driver has some way of knowing if it
> has to return PCI_ERS_RESULT_CAN_RECOVER ? or
> PCI_ERS_RESULT_NEED_RESET?
> 
> let us take an example of storage driver here e.g. NVME
> nvme_error_detected()  relies heavily on pci_channel_state_t which is
> passed by error framework  (err.c)
> 
> I understand your point of view and original intention of design that
> let driver dictate whether it wants slot_reset ?
> but driver relies on framework to pass that information in order to
> take decision.
> 
> In case of pci_channel_io_frozen  (which is ERR_FATAL), most of the
> drivers demand link reset.
> 
> but in case of ERR_NONFATAL, the link is supposed to be functional and
> there is no need for link reset.
> and exactly all the PCI based drivers returns
> PCI_ERS_RESULT_CAN_RECOVER, which is valid.
> 
> 
> so I think to conclude, you are referring more of your views towards
> pcie_do_fatal_recovery()
> 
> 
> which does
> 
>> remove_devices from the downstream link
>> reset_link()   (Secondary bus reset)
>> re-enumerate.
> 
> and this is what DPC defined, and AER is just following that.
> 
> Regards,
> Oza.
> 
>> 
>> Also because multiple devices, at least on power, can share a domain,
>> we can get into a situation where one device requires a reset, so all
>> will get reset, and their respective drivers need to be notified.
>> 
>> Note: it's not so much about resetting the *link* than about resetting
>> the *device*.
>> 
>>> although
>>> I see following note in the code as well.
>>>                  /*
>>> 		 * TODO: Should call platform-specific
>>> 		 * functions to reset slot before calling
>>> 		 * drivers' slot_reset callbacks?
>>> 		 */
>> 
>> Yup :-)
>> 
>> Cheers,
>> Ben.
>> 
>>> Regards,
>>> Oza.
>>> 
>>> >
>>> > Also we should do a hot reset at least, not just a link reset.
>>> >
>>> > >      report_resume()
>>> > >
>>> > > If you suggest how it is broken, it will help me to understand.
>>> > > probably you might want to point out what are the calls need to be
>>> > > added
>>> > > or removed or differently handled, specially storage point of view.
>>> >
>>> >
>>> > > Regards,
>>> > > Oza.
>>> > >
>>> > > >
>>> > > > Keep in mind that those callbacks were designed originally for EEH
>>> > > > (which predates AER), and so was the spec written.
>>> > > >
>>> > > > We don't actually use the AER code on POWER today, so we didn't notice
>>> > > > how broken the implementation was :-)
>>> > > >
>>> > > > We should fix that.
>>> > > >
>>> > > > Either we can sort all that out by email, or we should plan some kind
>>> > > > of catch-up, either at Plumbers (provided I can go there) or maybe a
>>> > > > webex call.
>>> > > >
>>> > > > Cheers,
>>> > > > Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Fri, 2018-08-17 at 15:59 +0530, poza@codeaurora.org wrote:
> 
> The existing design is; framework dictating drivers what it should do 
> rather than driver deciding.
> let me explain.

The AER code design might be but it's not what was documented in
Documentation/PCI/pci-error-recovery.txt before you changed it !

And it's not what about 20 years of doing PCI error recovery before
there even was such a thing as PCIe or AER taught us on powerpc.

The basic idea is that the "action" to be taken to recover is the
"deepest" of all the ones requested by the framework and all the
involved drivers (more than one device could be part of an error
handling "domain" under some circumstances, for example when the
failure originates from a bridge).

What the PCIe spec says is rather uninteresting and can be trusted as
far as policy is concern about as much as you can trust HW to be
compliant with it, that is about zilch.

The only interesting thing to take from the spec is that in the case of
a FATAL error, you *must* at least reset the link. That's it. It
doesn't mean that you shouldn't reset more (PERST for example) and it
doesn't mean that you shouldn't reset the device or link for a
NON_FATAL error either. And even if it did we should ignore it.

There are many reasons why a driver might request a reset. For example,
the error, while non-fatal to the link itself, might still have
triggered a non-recoverable event in the device, either in HW or SW,
and the only safe way out might well be a reset.

On POWER systems, with EEH HW support, we have additional rules that
say that on *any* non-corrected error (and under some circumstances if
corrected errors cross a certain threshold) will isolate the device
completely and the typical recovery from that is a reset (it doesn't
*have* to be but that's what most drivers chose).

This is rules design to prevent propagation of potentially corrupted
data which is considering way more important than QoS of the device.

We should see if we can make some of the AER and EEH code more common,
the first thing would be to take things out of pcie/ since EEH isn't
PCIe specific, and abstract the recovery process from the underlying
architecture hooks to perform things like reset etc...
 
> aer_isr
>      aer_isr_one_error
>           get_device_error_info   >> this checks
>                {
>                  /* Link is still healthy for IO reads */    >> Bjorn 
> this looks like a very strange thing to me, if link is not healthy we 
> are not even going for pcie_do_fatal_recovery()
> 		pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS,
> 			&info->status);
> 		pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_MASK,
> 			&info->mask);
> 		if (!(info->status & ~info->mask))
> 			return 0;
> 
>                {
>          handle_error_source
>          pcie_do_nonfatal_recovery or pcie_do_fatal_recovery
> 
> now it does not seem to me that driver has some way of knowing if it has 
> to return PCI_ERS_RESULT_CAN_RECOVER ? or PCI_ERS_RESULT_NEED_RESET?

You don't know that. A driver can look at the device state at the
enable_mmio phase and decide that the device is sufficiently sane that
it can continue... or not. A driver could have a policy of always
resetting. Etc...

> let us take an example of storage driver here e.g. NVME
> nvme_error_detected()  relies heavily on pci_channel_state_t which is 
> passed by error framework  (err.c)

So what ? This is one example... the IPR SCSI driver is much more
complex in its error recovery afaik.

> I understand your point of view and original intention of design that 
> let driver dictate whether it wants slot_reset ?

It's both the driver *and* the framework, which ever wants the
"harshest" solution wins. If either wants a reset we do a reset.

> but driver relies on framework to pass that information in order to take 
> decision.

No. Some drivers might. This is by no means the absolute rule. There
are also all other type of errors that aren't covered by the AER
categories that fit in that framework such as iommu errors (at least
for us on powerpc).

> In case of pci_channel_io_frozen  (which is ERR_FATAL), most of the 
> drivers demand link reset.

No, they demand a *device* reset. ie, hot reset or PERST. It's not
about the link. It's about the device.

> but in case of ERR_NONFATAL, the link is supposed to be functional and 
> there is no need for link reset.

It depends whether the error was corrected or not. If it was not
corrected, then something bad did happen and the framework has no way
to know what is the appropriate remediation for a given device.

> and exactly all the PCI based drivers returns 
> PCI_ERS_RESULT_CAN_RECOVER, which is valid.

You are mixing the AER terminology of FATAL/NON_FATAL which is AER
specific and the framework defined channel states, which are
"functional", "frozen" and "permanently dead". These don't necessarily
match 1:1.

> so I think to conclude, you are referring more of your views towards
> pcie_do_fatal_recovery()

Both. But yes, pcie_do_fatal_recovery() should definitely NOT do an
unplug/replug blindly. The whole point of the error handlers was to
allow driver to recover from these things without losing the links to
their respective consumers (such as filesystems for block devices).

> which does
> 
> > remove_devices from the downstream link
> > reset_link()   (Secondary bus reset)
> > re-enumerate.
> 
> and this is what DPC defined, and AER is just following that.

And is completely and utterly broken and useless as a policy.

Again, somebody cared too much about the spec rather than what makes
actual sense in practice.

You should *only* remove devices if the driver doesn't have the
necessary callbacks.

In fact, remove and re-plug device is probably the worst thing you can
do, I don't remember all the details but it's been causing us endless
issues with EEH, which is why as I mentioned, we are looking more
toward an unbind and rebind of the driver, but that's a details.

If the driver has the error callbacks it should participate in the
recovery and NOT be unbound or the device unplugged. That's completely
useless for Linux to do that since it means you cannot recover from an
error on a storage driver.

Ben.

> Regards,
> Oza.
> 
> > 
> > Also because multiple devices, at least on power, can share a domain,
> > we can get into a situation where one device requires a reset, so all
> > will get reset, and their respective drivers need to be notified.
> > 
> > Note: it's not so much about resetting the *link* than about resetting
> > the *device*.
> > 
> > > although
> > > I see following note in the code as well.
> > >                  /*
> > > 		 * TODO: Should call platform-specific
> > > 		 * functions to reset slot before calling
> > > 		 * drivers' slot_reset callbacks?
> > > 		 */
> > 
> > Yup :-)
> > 
> > Cheers,
> > Ben.
> > 
> > > Regards,
> > > Oza.
> > > 
> > > > 
> > > > Also we should do a hot reset at least, not just a link reset.
> > > > 
> > > > >      report_resume()
> > > > > 
> > > > > If you suggest how it is broken, it will help me to understand.
> > > > > probably you might want to point out what are the calls need to be
> > > > > added
> > > > > or removed or differently handled, specially storage point of view.
> > > > 
> > > > 
> > > > > Regards,
> > > > > Oza.
> > > > > 
> > > > > > 
> > > > > > Keep in mind that those callbacks were designed originally for EEH
> > > > > > (which predates AER), and so was the spec written.
> > > > > > 
> > > > > > We don't actually use the AER code on POWER today, so we didn't notice
> > > > > > how broken the implementation was :-)
> > > > > > 
> > > > > > We should fix that.
> > > > > > 
> > > > > > Either we can sort all that out by email, or we should plan some kind
> > > > > > of catch-up, either at Plumbers (provided I can go there) or maybe a
> > > > > > webex call.
> > > > > > 
> > > > > > Cheers,
> > > > > > Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Thu, 2018-08-16 at 16:51 +1000, Benjamin Herrenschmidt wrote:
> No, this is wrong and not the intent of the error handling.
> 
> You seem to be applying PCIe specific concepts brain-farted at Intel
> that are way way away from what we care about in practice and in Linux.
> 
> > e.g.  some driver handle errors ERR_NONFATAL or FATAL in similar ways
> > e.g.
> > ioat_pcie_error_detected();  calls  ioat_shutdown(); in case of 
> > ERR_NONFATAL
> > otherwise ioat_shutdown() in case of ERR_FATAL.
> 
> Since when the error handling callbacks even have the concept of FATAL
> vs. non-fatal ? This doesn't appear anyhwhere in the prototype of the
> struct pci_error_handlers and shouldn't.

Ugh... I just saw the changes you did to Documentation/PCI/pci-error-
recovery.txt and I would very much like to revert those !

Bjorn, you shouldn't let changes to the PCI error handling through
without acks from us, it looks like we didn't notice (we can't possibly
monitor all lists).

We wrote that in the firsat place and our EEH infrastructure rely on it
heavily on it.

Poza, you seem to have not understood the intent of the code and are
now changing the rules in ways that are broken in our opinion. This is
bad.

Bjorn, please revert all of those changes.

There was NEVER an intent to separate fatal from non-fatal at that
level. We could pass the information to the driver if we wished but the
recovery sequence is NOT intended to be different.

Especially we specifically do NOT want to unplug and replug the device
for fatal errors at all. This is not going to work with drivers that
cannot re-link with their various kernel services, such as storage
devices re-connecting with mounted file systems etc...

Those changes are utterly broken.

The basic premise of the design that we woudl do that unplug/replug
trick if and ONLY IF the driver doesn't have the appropriate callbacks.

We are also now looking at replacing this with an ubind/re-bind because
in practice, the unplugging is causing us all sort of problems. Sam
(CC) can elaborate.

Bjorn, we are the main authors of that spec (Linas wrote it under my
supervision) and created those callbacks for EEH. AER picked them up
only later. Those changes must be at the very least acked by us before
going upstream.

Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Thu, 2018-08-16 at 17:05 +1000, Benjamin Herrenschmidt wrote:
> Those changes are utterly broken.
> 
> The basic premise of the design that we woudl do that unplug/replug
> trick if and ONLY IF the driver doesn't have the appropriate callbacks.
> 
> We are also now looking at replacing this with an ubind/re-bind because
> in practice, the unplugging is causing us all sort of problems. Sam
> (CC) can elaborate.
> 
> Bjorn, we are the main authors of that spec (Linas wrote it under my
> supervision) and created those callbacks for EEH. AER picked them up
> only later. Those changes must be at the very least acked by us before
> going upstream.

Also I had a quick look at the DPC spec, it looks like a subset of our
EEH capability.

Again, the code in Linux was written without concertation with us,
misunderstands/misuses the driver callbacks, does unplugs when it
shouldn't etc... 

It's all very broken.

Please, at the very least revert the spec changes. They are utterly
wrong.

The driver MUST remain active during the recovery process *including*
fatal errors.

Only if the recovery fails and the driver gives us may you chose to
unplug the device (though there is little point).

What you have designed will work fine for network drivers but will not
work for storage.

Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-16 12:45, Benjamin Herrenschmidt wrote:
> On Thu, 2018-08-16 at 17:05 +1000, Benjamin Herrenschmidt wrote:
>> Those changes are utterly broken.
>> 
>> The basic premise of the design that we woudl do that unplug/replug
>> trick if and ONLY IF the driver doesn't have the appropriate 
>> callbacks.
>> 
>> We are also now looking at replacing this with an ubind/re-bind 
>> because
>> in practice, the unplugging is causing us all sort of problems. Sam
>> (CC) can elaborate.
>> 
>> Bjorn, we are the main authors of that spec (Linas wrote it under my
>> supervision) and created those callbacks for EEH. AER picked them up
>> only later. Those changes must be at the very least acked by us before
>> going upstream.
> 
> Also I had a quick look at the DPC spec, it looks like a subset of our
> EEH capability.
> 
> Again, the code in Linux was written without concertation with us,
> misunderstands/misuses the driver callbacks, does unplugs when it
> shouldn't etc...
> 
> It's all very broken.
> 
> Please, at the very least revert the spec changes. They are utterly
> wrong.
> 
> The driver MUST remain active during the recovery process *including*
> fatal errors.
> 
> Only if the recovery fails and the driver gives us may you chose to
> unplug the device (though there is little point).
> 
> What you have designed will work fine for network drivers but will not
> work for storage.
> 
> Ben.

Hi Ben,

If you look at original DPC driver it was doing exactly the same as it 
is doing now.
so with or without new changes DPC driver had the same behavior from 
beginning.

commit 26e515713342b6f7c553aa3c66b21c6ab7cf82af

Regards,
Oza.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Thu, 2018-08-16 at 13:26 +0530, poza@codeaurora.org wrote:
> > Please, at the very least revert the spec changes. They are utterly
> > wrong.
> > 
> > The driver MUST remain active during the recovery process *including*
> > fatal errors.
> > 
> > Only if the recovery fails and the driver gives us may you chose to
> > unplug the device (though there is little point).
> > 
> > What you have designed will work fine for network drivers but will not
> > work for storage.
> > 
> > Ben.
> 
> Hi Ben,
> 
> If you look at original DPC driver it was doing exactly the same as it 
> is doing now.
> so with or without new changes DPC driver had the same behavior from 
> beginning.

Yes and that behaviour is utterly wrong.

> commit 26e515713342b6f7c553aa3c66b21c6ab7cf82af

Yup, I noticed. We don't use DPC, all of that is subsumed into our EEH
infrastructure, so we didn't notice.

But now that the wrongness is spreading it's starting to show,
especially the changes to the spec.

We shouldn't be too focused on ERR_FATAL vs NON_FATAL. HW has a knack
for misreporting these things anyway.

All we really care about is whether the device was isolated and data
was lost or whether it's just an informational corrected error. The
latter should just be reported without impact. The former should lead
to a recovery procedure.

The design of the error callback was specifially so that you *could*
reset the link (or even PERST the whole device & retrain) *without*
having to unbind the driver or worse, soft-unplug the device.
Specifically beacause once you do the latter, you lose the links to the
mounted filesystems and those cannot be re-established.

So the spec needs to be reverted, DPC fixed and AER as well to do the
right thing here.

In fact it would be nice if we could make some of that logic common
with EEH. Sam (CC) has been cleaning up some of our EEH code which is,
to be honest, a bloody mess, so we might be able to comprehend and
refactor it more easily.

The basic idea is that regardless of the type of error, you first
notify the driver that an error occured. At that point, the io channel
might have been frozen already, so the driver can make no expectation
that the device is still reachable, which matches what DPC does I
believe.

The driver can ask for re-enabling the channel if it thinks it can
attempt a recovery but the core doesn't have to honor it and can chose
to just reset the device.

Similarly, in case of re-enabling, the driver can decide that this
didn't work out and request an escalation to a reset.

So typically, a non-fatal could go through the re-enable phase (in the
case where DPC didn't actually block the channel at all), while non-
fatal would always go to reset.

The only case where we would "unplug" on the linux side is when the
driver doesn't have error handling callbacks.

Cheers,
Ben.
 

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-16 12:35, Benjamin Herrenschmidt wrote:
> On Thu, 2018-08-16 at 16:51 +1000, Benjamin Herrenschmidt wrote:
>> No, this is wrong and not the intent of the error handling.
>> 
>> You seem to be applying PCIe specific concepts brain-farted at Intel
>> that are way way away from what we care about in practice and in 
>> Linux.
>> 
>> > e.g.  some driver handle errors ERR_NONFATAL or FATAL in similar ways
>> > e.g.
>> > ioat_pcie_error_detected();  calls  ioat_shutdown(); in case of
>> > ERR_NONFATAL
>> > otherwise ioat_shutdown() in case of ERR_FATAL.
>> 
>> Since when the error handling callbacks even have the concept of FATAL
>> vs. non-fatal ? This doesn't appear anyhwhere in the prototype of the
>> struct pci_error_handlers and shouldn't.
> 
> Ugh... I just saw the changes you did to Documentation/PCI/pci-error-
> recovery.txt and I would very much like to revert those !
> 
> Bjorn, you shouldn't let changes to the PCI error handling through
> without acks from us, it looks like we didn't notice (we can't possibly
> monitor all lists).
> 
> We wrote that in the firsat place and our EEH infrastructure rely on it
> heavily on it.
> 
> Poza, you seem to have not understood the intent of the code and are
> now changing the rules in ways that are broken in our opinion. This is
> bad.
> 
> Bjorn, please revert all of those changes.
> 
> There was NEVER an intent to separate fatal from non-fatal at that
> level. We could pass the information to the driver if we wished but the
> recovery sequence is NOT intended to be different.
> 
> Especially we specifically do NOT want to unplug and replug the device
> for fatal errors at all. This is not going to work with drivers that
> cannot re-link with their various kernel services, such as storage
> devices re-connecting with mounted file systems etc...
> 
> Those changes are utterly broken.
> 
> The basic premise of the design that we woudl do that unplug/replug
> trick if and ONLY IF the driver doesn't have the appropriate callbacks.
> 
> We are also now looking at replacing this with an ubind/re-bind because
> in practice, the unplugging is causing us all sort of problems. Sam
> (CC) can elaborate.
> 
> Bjorn, we are the main authors of that spec (Linas wrote it under my
> supervision) and created those callbacks for EEH. AER picked them up
> only later. Those changes must be at the very least acked by us before
> going upstream.
> 
> Ben.


+ Sinan

This patch set was there in mailing list for nearly 17 to 18 revisions 
for 7 months.
besides the intent was to bring DPC and AER into the same well defined 
way of error handling.
The way DPC used to behave in 2016, is still the same; which involved 
removing and re-enumerating the devices.

Regards,
Oza.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Thu, 2018-08-16 at 13:35 +0530, poza@codeaurora.org wrote:
> > 
> > Bjorn, we are the main authors of that spec (Linas wrote it under my
> > supervision) and created those callbacks for EEH. AER picked them up
> > only later. Those changes must be at the very least acked by us before
> > going upstream.
> > 
> > Ben.
> 
> 
> + Sinan
> 
> This patch set was there in mailing list for nearly 17 to 18 revisions 
> for 7 months.

Right and sadly the guy doing EEH on our side left and I didn't notice
what was going on in the list.

But Bjorn should know better :-)

> besides the intent was to bring DPC and AER into the same well defined 
> way of error handling.

That's a good idea, but we need to fix DPC and AER understanding of the
intent of those callbacks, not change the spec to match the broken
implementation.

> The way DPC used to behave in 2016, is still the same; which involved 
> removing and re-enumerating the devices.

Which is mostly useless for anything that isn't a network device.

We've been doing EEH for something like 15 to 20 years, so we have a
long experience with what it takes to get PCI(e) devices to recover on
enterprise systems.

Removing and re-enumerating is one of the very worst thing you can do
in that area.

Cheers,
Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-16 13:45, Benjamin Herrenschmidt wrote:
> On Thu, 2018-08-16 at 13:35 +0530, poza@codeaurora.org wrote:
>> >
>> > Bjorn, we are the main authors of that spec (Linas wrote it under my
>> > supervision) and created those callbacks for EEH. AER picked them up
>> > only later. Those changes must be at the very least acked by us before
>> > going upstream.
>> >
>> > Ben.
>> 
>> 
>> + Sinan
>> 
>> This patch set was there in mailing list for nearly 17 to 18 revisions
>> for 7 months.
> 
> Right and sadly the guy doing EEH on our side left and I didn't notice
> what was going on in the list.
> 
> But Bjorn should know better :-)
> 
>> besides the intent was to bring DPC and AER into the same well defined
>> way of error handling.
> 
> That's a good idea, but we need to fix DPC and AER understanding of the
> intent of those callbacks, not change the spec to match the broken
> implementation.
> 

ok lets start with what we have rather than going back, because 
reverting the changes is not going to solve anything
as I mentioned the behavior of some of the functions and DPC (was the 
same before and now)
but the good thing happened because of the patches is; there is a common 
framework defined in err.c
and DPC and AER both act on similar rules (the rule is what we define 
understanding of SPEC)

and all we have to do is discuss and evolve it or change it
we can catch up on webex, (Sinan is going to be there in Plumber's 
conference, I might not be able to join there, as we have bring-up 
coming)


>> The way DPC used to behave in 2016, is still the same; which involved
>> removing and re-enumerating the devices.
> 
> Which is mostly useless for anything that isn't a network device.
> 
> We've been doing EEH for something like 15 to 20 years, so we have a
> long experience with what it takes to get PCI(e) devices to recover on
> enterprise systems.
> 
> Removing and re-enumerating is one of the very worst thing you can do
> in that area.
> 
> Cheers,
> Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Thu, 2018-08-16 at 13:52 +0530, poza@codeaurora.org wrote:
> 
> ok lets start with what we have rather than going back, because 
> reverting the changes is not going to solve anything

I'm not saying you should revert the DPC/AER changes, but I want to
revert the *spec* changes, they are wrong and they make EEH now not
match the spec.

IE. Documentation/PCI/pci-error-recovery.txt

> as I mentioned the behavior of some of the functions and DPC (was the 
> same before and now)
> but the good thing happened because of the patches is; there is a common 
> framework defined in err.c
> and DPC and AER both act on similar rules (the rule is what we define 
> understanding of SPEC)

Depends what you call spec. I'm talking about the Linux error recovery
specification.

Let's not muddle it with the PCIe spec itself, or I'll start quoting
Linus about the general usefulness of specs :-)

What we need to do is how we want Linux and drivers to behave for error
recovery, more / less *regardless* of what the PCIe spec says because
HW specs are invariably wrong and HW implementations ignore them more
often than not anyway.

> and all we have to do is discuss and evolve it or change it
> we can catch up on webex, (Sinan is going to be there in Plumber's 
> conference, I might not be able to join there, as we have bring-up 
> coming)

Ok, I'll try to get there. Let's plan at least a BOF or two if not a
microconf.

To setup a webex let's first list who needs to attend and respective
timezones so we can figure out a time. I'm in Australia east coast.

> > > The way DPC used to behave in 2016, is still the same; which involved
> > > removing and re-enumerating the devices.
> > 
> > Which is mostly useless for anything that isn't a network device.
> > 
> > We've been doing EEH for something like 15 to 20 years, so we have a
> > long experience with what it takes to get PCI(e) devices to recover on
> > enterprise systems.
> > 
> > Removing and re-enumerating is one of the very worst thing you can do
> > in that area.
> > 
> > Cheers,
> > Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Thomas Tai]

[ ... ]>
>> and all we have to do is discuss and evolve it or change it
>> we can catch up on webex, (Sinan is going to be there in Plumber's
>> conference, I might not be able to join there, as we have bring-up
>> coming)
> 
> Ok, I'll try to get there. Let's plan at least a BOF or two if not a
> microconf.
> 
> To setup a webex let's first list who needs to attend and respective
> timezones so we can figure out a time. I'm in Australia east coast.

Hi guys,
I see that there are a lot discussion regarding the error handling. May 
I join the webex and hopefully I can be helpful? I'm in Canada east 
coast (Ottawa,Ontario,Canada)

Thank you,
Thomas

> 
>>>> The way DPC used to behave in 2016, is still the same; which involved
>>>> removing and re-enumerating the devices.
>>>
>>> Which is mostly useless for anything that isn't a network device.
>>>
>>> We've been doing EEH for something like 15 to 20 years, so we have a
>>> long experience with what it takes to get PCI(e) devices to recover on
>>> enterprise systems.
>>>
>>> Removing and re-enumerating is one of the very worst thing you can do
>>> in that area.
>>>
>>> Cheers,
>>> Ben.
> 

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Sinan Kaya]

On 8/16/2018 4:22 AM, poza@codeaurora.org wrote:
> we can catch up on webex, (Sinan is going to be there in Plumber's 
> conference, I might not be able to join there, as we have bring-up coming)

My plumbers conference attendance is not confirmed yet. I'm trying to
combine it with a business trip. I did not get an ACK yet from my boss.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Thu, 2018-08-16 at 09:46 -0400, Sinan Kaya wrote:
> On 8/16/2018 4:22 AM, poza@codeaurora.org wrote:
> > we can catch up on webex, (Sinan is going to be there in Plumber's 
> > conference, I might not be able to join there, as we have bring-up coming)
> 
> My plumbers conference attendance is not confirmed yet. I'm trying to
> combine it with a business trip. I did not get an ACK yet from my boss.

Ok, I'll collect the names and will propose a time/date early next
week, I'm a bit caught up until monday.

Bjorn, are you available as well ?

Cheers,
Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-17 04:57, Benjamin Herrenschmidt wrote:
> On Thu, 2018-08-16 at 09:46 -0400, Sinan Kaya wrote:
>> On 8/16/2018 4:22 AM, poza@codeaurora.org wrote:
>> > we can catch up on webex, (Sinan is going to be there in Plumber's
>> > conference, I might not be able to join there, as we have bring-up coming)
>> 
>> My plumbers conference attendance is not confirmed yet. I'm trying to
>> combine it with a business trip. I did not get an ACK yet from my 
>> boss.
> 
> Ok, I'll collect the names and will propose a time/date early next
> week, I'm a bit caught up until monday.
> 
> Bjorn, are you available as well ?

I might be able to make it to Plumbers conference.

Regards,
Oza.

> 
> Cheers,
> Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Bjorn Helgaas]

On Fri, Aug 17, 2018 at 09:27:03AM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2018-08-16 at 09:46 -0400, Sinan Kaya wrote:
> > On 8/16/2018 4:22 AM, poza@codeaurora.org wrote:
> > > we can catch up on webex, (Sinan is going to be there in Plumber's 
> > > conference, I might not be able to join there, as we have bring-up coming)
> > 
> > My plumbers conference attendance is not confirmed yet. I'm trying to
> > combine it with a business trip. I did not get an ACK yet from my boss.
> 
> Ok, I'll collect the names and will propose a time/date early next
> week, I'm a bit caught up until monday.
> 
> Bjorn, are you available as well ?

I'm in CDT (UTC-5).  Possible times for me are 6am-10pm CDT.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-19 07:54, Bjorn Helgaas wrote:
> On Fri, Aug 17, 2018 at 09:27:03AM +1000, Benjamin Herrenschmidt wrote:
>> On Thu, 2018-08-16 at 09:46 -0400, Sinan Kaya wrote:
>> > On 8/16/2018 4:22 AM, poza@codeaurora.org wrote:
>> > > we can catch up on webex, (Sinan is going to be there in Plumber's
>> > > conference, I might not be able to join there, as we have bring-up coming)
>> >
>> > My plumbers conference attendance is not confirmed yet. I'm trying to
>> > combine it with a business trip. I did not get an ACK yet from my boss.
>> 
>> Ok, I'll collect the names and will propose a time/date early next
>> week, I'm a bit caught up until monday.
>> 
>> Bjorn, are you available as well ?
> 
> I'm in CDT (UTC-5).  Possible times for me are 6am-10pm CDT.

I am in India timezone.

Regards,
Oza.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Mon, 2018-08-20 at 10:39 +0530, poza@codeaurora.org wrote:
> On 2018-08-19 07:54, Bjorn Helgaas wrote:
> > On Fri, Aug 17, 2018 at 09:27:03AM +1000, Benjamin Herrenschmidt wrote:
> > > On Thu, 2018-08-16 at 09:46 -0400, Sinan Kaya wrote:
> > > > On 8/16/2018 4:22 AM, poza@codeaurora.org wrote:
> > > > > we can catch up on webex, (Sinan is going to be there in Plumber's
> > > > > conference, I might not be able to join there, as we have bring-up coming)
> > > > 
> > > > My plumbers conference attendance is not confirmed yet. I'm trying to
> > > > combine it with a business trip. I did not get an ACK yet from my boss.
> > > 
> > > Ok, I'll collect the names and will propose a time/date early next
> > > week, I'm a bit caught up until monday.
> > > 
> > > Bjorn, are you available as well ?
> > 
> > I'm in CDT (UTC-5).  Possible times for me are 6am-10pm CDT.
> 
> I am in India timezone.

Hrm ... this is going to be difficult to reconcile. Let's first see
what we can do by email this week shall we ?

Cheers,
Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Thomas Tai]

On 08/20/2018 01:15 AM, Benjamin Herrenschmidt wrote:
> On Mon, 2018-08-20 at 10:39 +0530, poza@codeaurora.org wrote:
>> On 2018-08-19 07:54, Bjorn Helgaas wrote:
>>> On Fri, Aug 17, 2018 at 09:27:03AM +1000, Benjamin Herrenschmidt wrote:
>>>> On Thu, 2018-08-16 at 09:46 -0400, Sinan Kaya wrote:
>>>>> On 8/16/2018 4:22 AM, poza@codeaurora.org wrote:
>>>>>> we can catch up on webex, (Sinan is going to be there in Plumber's
>>>>>> conference, I might not be able to join there, as we have bring-up coming)
>>>>>
>>>>> My plumbers conference attendance is not confirmed yet. I'm trying to
>>>>> combine it with a business trip. I did not get an ACK yet from my boss.
>>>>
>>>> Ok, I'll collect the names and will propose a time/date early next
>>>> week, I'm a bit caught up until monday.
>>>>
>>>> Bjorn, are you available as well ?
>>>
>>> I'm in CDT (UTC-5).  Possible times for me are 6am-10pm CDT.
>>
>> I am in India timezone.
> 
> Hrm ... this is going to be difficult to reconcile. Let's first see
> what we can do by email this week shall we ?

OK, I will catch up on email to see the details. I suppose I should hold 
off the original patch to fix the "prevent pcie_do_fatal_recovery from 
using device after it is removed" patch, right?

Thank you,
Thomas

> 
> Cheers,
> Ben.
> 
> 

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Mon, 2018-08-20 at 09:02 -0400, Thomas Tai wrote:
> 
> On 08/20/2018 01:15 AM, Benjamin Herrenschmidt wrote:
> > On Mon, 2018-08-20 at 10:39 +0530, poza@codeaurora.org wrote:
> > > On 2018-08-19 07:54, Bjorn Helgaas wrote:
> > > > On Fri, Aug 17, 2018 at 09:27:03AM +1000, Benjamin Herrenschmidt wrote:
> > > > > On Thu, 2018-08-16 at 09:46 -0400, Sinan Kaya wrote:
> > > > > > On 8/16/2018 4:22 AM, poza@codeaurora.org wrote:
> > > > > > > we can catch up on webex, (Sinan is going to be there in Plumber's
> > > > > > > conference, I might not be able to join there, as we have bring-up coming)
> > > > > > 
> > > > > > My plumbers conference attendance is not confirmed yet. I'm trying to
> > > > > > combine it with a business trip. I did not get an ACK yet from my boss.
> > > > > 
> > > > > Ok, I'll collect the names and will propose a time/date early next
> > > > > week, I'm a bit caught up until monday.
> > > > > 
> > > > > Bjorn, are you available as well ?
> > > > 
> > > > I'm in CDT (UTC-5).  Possible times for me are 6am-10pm CDT.
> > > 
> > > I am in India timezone.
> > 
> > Hrm ... this is going to be difficult to reconcile. Let's first see
> > what we can do by email this week shall we ?
> 
> OK, I will catch up on email to see the details. I suppose I should hold 
> off the original patch to fix the "prevent pcie_do_fatal_recovery from 
> using device after it is removed" patch, right?

I wouldn't hold on bug fixes for now. We might refactor all of this but
it might take a kernel version or two...

Cheers,
Ben.

> 
> Thank you,
> Thomas
> 
> > 
> > Cheers,
> > Ben.
> > 
> > 

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Bjorn Helgaas]

On Thu, Aug 16, 2018 at 05:05:30PM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2018-08-16 at 16:51 +1000, Benjamin Herrenschmidt wrote:
> > No, this is wrong and not the intent of the error handling.
> > 
> > You seem to be applying PCIe specific concepts brain-farted at Intel
> > that are way way away from what we care about in practice and in Linux.
> > 
> > > e.g.  some driver handle errors ERR_NONFATAL or FATAL in similar ways
> > > e.g.
> > > ioat_pcie_error_detected();  calls  ioat_shutdown(); in case of 
> > > ERR_NONFATAL
> > > otherwise ioat_shutdown() in case of ERR_FATAL.
> > 
> > Since when the error handling callbacks even have the concept of FATAL
> > vs. non-fatal ? This doesn't appear anyhwhere in the prototype of the
> > struct pci_error_handlers and shouldn't.
> 
> Ugh... I just saw the changes you did to Documentation/PCI/pci-error-
> recovery.txt and I would very much like to revert those !
> 
> Bjorn, you shouldn't let changes to the PCI error handling through
> without acks from us, it looks like we didn't notice (we can't possibly
> monitor all lists).

Sorry, they were certainly very visible on linux-pci, but I should
have added you to cc if you weren't there already.  Please update
MAINTAINERS if it's incomplete or out of date (I can't possibly know
who might be interested in every change).

> Bjorn, please revert all of those changes.

Please send the appropriate patches and we'll go from there.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Sinan Kaya]

On 8/18/2018 10:19 PM, Bjorn Helgaas wrote:
>> Bjorn, please revert all of those changes.
> Please send the appropriate patches and we'll go from there.
> 

I'm also catching up on this thread.

I don't think revert is the way to go. There is certainly value in Oza's
code to make error handling common.

We started by following the existing error handling scheme and then
moved onto the stop/remove behavior based on Bjorn's feedback.

The right thing is for Oza to rework the code to go back to original
error handling callback mechanism. That should be a trivial change.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Sun, 2018-08-19 at 17:41 -0400, Sinan Kaya wrote:
> On 8/18/2018 10:19 PM, Bjorn Helgaas wrote:
> > > Bjorn, please revert all of those changes.
> > 
> > Please send the appropriate patches and we'll go from there.
> > 
> 
> I'm also catching up on this thread.
> 
> I don't think revert is the way to go. There is certainly value in Oza's
> code to make error handling common.

The revert of the Documentation change must happen though. It's
completely wrong. The documentation documents what EEH implements so by
making it match what I argue is a broken implementation in AER, you are
in fact breaking us.

> We started by following the existing error handling scheme and then
> moved onto the stop/remove behavior based on Bjorn's feedback.

Whish is utterly wrong.

> The right thing is for Oza to rework the code to go back to original
> error handling callback mechanism. That should be a trivial change.

At this stage I'm only asking to revert the documentation updatgae.
I'll send a patch to that effect.

As for figuring out where to go from there, I agree we should discuss
this further, I would love to be able to make more of the code common
with EEH as well.

Cheers,
Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-20 07:33, Benjamin Herrenschmidt wrote:
> On Sun, 2018-08-19 at 17:41 -0400, Sinan Kaya wrote:
>> On 8/18/2018 10:19 PM, Bjorn Helgaas wrote:
>> > > Bjorn, please revert all of those changes.
>> >
>> > Please send the appropriate patches and we'll go from there.
>> >
>> 
>> I'm also catching up on this thread.
>> 
>> I don't think revert is the way to go. There is certainly value in 
>> Oza's
>> code to make error handling common.
> 
> The revert of the Documentation change must happen though. It's
> completely wrong. The documentation documents what EEH implements so by
> making it match what I argue is a broken implementation in AER, you are
> in fact breaking us.
> 
>> We started by following the existing error handling scheme and then
>> moved onto the stop/remove behavior based on Bjorn's feedback.
> 
> Whish is utterly wrong.
> 
>> The right thing is for Oza to rework the code to go back to original
>> error handling callback mechanism. That should be a trivial change.
> 
> At this stage I'm only asking to revert the documentation updatgae.
> I'll send a patch to that effect.
> 
> As for figuring out where to go from there, I agree we should discuss
> this further, I would love to be able to make more of the code common
> with EEH as well.
> 
> Cheers,
> Ben.


Reverting spec/Documentation which is fine by me.

But the good thing has happened now is; we can have very clear 
definition for the framework to go forward.
e.g. how the errors have to be handled.

Because of those patches, the whole error framework is under common code 
base and now has become independent of service e.g. AER, DPC etc..
That enables us to define or extend policies in more clearly defined way 
irrespective of what services are running.

Now it is just that we have to change in err.c and walk away with the 
policies what we want to enforce.

let me know how this sounds Ben.

Regards,
Oza.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Mon, 2018-08-20 at 10:49 +0530, poza@codeaurora.org wrote:
> 
> Reverting spec/Documentation which is fine by me.
> 
> But the good thing has happened now is; we can have very clear 
> definition for the framework to go forward.
> e.g. how the errors have to be handled.
> 
> Because of those patches, the whole error framework is under common code 
> base and now has become independent of service e.g. AER, DPC etc..

Well, EEH isn't yet :-) But then the EEH code is a real mess buried in
arch/powerpc. Sam (CC) is trying to improve that situation and I might
step in as well to help if we think we can make things more common, it
would definitely help.

> That enables us to define or extend policies in more clearly defined way 
> irrespective of what services are running.
> 
> Now it is just that we have to change in err.c and walk away with the 
> policies what we want to enforce.
> 
> let me know how this sounds Ben.

So for now, I've sent a revert patch for the Documentation/ bit to
Bjorn, and I have no (not yet at least) beef in what you do in
drivers/pci/* ... however, that said, I think it would be great to move
EEH toward having a bulk of the policy use common code as well.

It will be long road, in part due to the historical crappyness of our
EEH code, so my thinking is we should:

 - First agree on what we want the policy to be. I need to read a bit
more about DPC since that's new to me, it seems to be similar to what
our EEH does, with slighty less granularity (we can freeze access to
individual functions for example).

 - Rework err.c to implement that policy with the existing AER and DPC
code.

 - Figure out what hooks might be needed to be able to plumb EEH into
it, possibly removing a bunch of crap in arch/powerpc (yay !)

I don't think having a webex will be that practical with the timezones
involved. I'm trying to get approval to go to Plumbers in which case we
could setup a BOF but I have no guarantee at this point that I can make
it happen.

So let's try using email as much possible for now.

Cheers,
Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-20 11:03, Benjamin Herrenschmidt wrote:
> On Mon, 2018-08-20 at 10:49 +0530, poza@codeaurora.org wrote:
>> 
>> Reverting spec/Documentation which is fine by me.
>> 
>> But the good thing has happened now is; we can have very clear
>> definition for the framework to go forward.
>> e.g. how the errors have to be handled.
>> 
>> Because of those patches, the whole error framework is under common 
>> code
>> base and now has become independent of service e.g. AER, DPC etc..
> 
> Well, EEH isn't yet :-) But then the EEH code is a real mess buried in
> arch/powerpc. Sam (CC) is trying to improve that situation and I might
> step in as well to help if we think we can make things more common, it
> would definitely help.
> 
>> That enables us to define or extend policies in more clearly defined 
>> way
>> irrespective of what services are running.
>> 
>> Now it is just that we have to change in err.c and walk away with the
>> policies what we want to enforce.
>> 
>> let me know how this sounds Ben.
> 
> So for now, I've sent a revert patch for the Documentation/ bit to
> Bjorn, and I have no (not yet at least) beef in what you do in
> drivers/pci/* ... however, that said, I think it would be great to move
> EEH toward having a bulk of the policy use common code as well.
> 

In my opinion, I think revert patch can wait, because again we might 
need to change it according to improvements we bring.

> It will be long road, in part due to the historical crappyness of our
> EEH code, so my thinking is we should:
> 
>  - First agree on what we want the policy to be. I need to read a bit
> more about DPC since that's new to me, it seems to be similar to what
> our EEH does, with slighty less granularity (we can freeze access to
> individual functions for example).
> 
>  - Rework err.c to implement that policy with the existing AER and DPC
> code.
> 

I went through eeh-pci-error-recovery.txt
If I pick up folowing

"
If the OS or device driver suspects that a PCI slot has been 
EEH-isolated, there is a firmware call it can make to determine if this 
is the case. If so, then the device driver should put itself into a 
consistent state (given that it won't be able to complete any pending 
work) and start recovery of the card.  Recovery normally would consist 
of resetting the PCI device (holding the PCI #RST line high for two 
seconds), followed by setting up the device config space (the base 
address registers (BAR's), latency timer, cache line size, interrupt 
line, and so on).  This is followed by a reinitialization of the device 
driver.  In a worst-case scenario, the power to the card can be toggled, 
at least on hot-plug-capable slots.  In principle, layers far above the 
device driver probably do not need to know that the PCI card has been 
"rebooted" in this way; ideally, there should be at most a pause in 
Ethernet/disk/USB I/O while the card is being reset.

If the card cannot be recovered after three or four resets, the 
kernel/device driver should assume the worst-case scenario, that the 
card has died completely, and report this error to the sysadmin. In 
addition, error messages are reported through RTAS and also through 
syslogd (/var/log/messages) to alert the sysadmin of PCI resets. The 
correct way to deal with failed adapters is to use the standard PCI 
hotplug tools to remove and replace the dead card.
"

some differences: I find is:
Current framework does not attempt recovery 3-4 times.

Besides If I grasp eeh-pci-error-recovery.txt correctly, (although I 
could be easily wrong),
mainly the difference is coming how we reset the device.

Linux uses SBR, while EEH seems to be using #PERST.
PERST signal implementation might be board specific, e.g. we have 
io-port-expander sitting on i2c to drive #PERST.
we rely on SBR. and SBR also should be a good way to bring hotreset of 
device.
"All Lanes in the configured Link transmit TS1 Ordered Sets with the Hot 
Reset bit 15 asserted and the configured Link and Lane numbers."
although I am not sure between #PERST and SBR is there is any subtle 
differences such as some sticky bits might not be affected by SBR.
but so far SBR has served well for us.

Also I see that eeh-pci-error-recovery.txt does not seem to distinguish 
between fatal and nonfatal errors, is the behavior same for both type of 
errors in EEH ?

I would like to mention is that there should nto be any need to reset 
the card in case of NONFATAL because by definition link is functional, 
only the particular transaction
had a problem. so not sure how EEH deals with ERR_NONFATAL.

Also I am very much interested in knowing original intention of DPC 
driver to unplug/plug devices,
all I remember in some conversation was:
hotplug capable bridge might have see devices changed, so it is safer to 
remove/unplug the devices and during which .shutdown methods of driver 
is called, in case of ERR_FATAL.
although DPC is HW recovery while AER is sw recovery both should 
fundamentally act in the same way as far as device drivers callbacks are 
concerned.
(again I really dont know real motivation behind this)


Regards,
Oza.

>  - Figure out what hooks might be needed to be able to plumb EEH into
> it, possibly removing a bunch of crap in arch/powerpc (yay !)
> 
> I don't think having a webex will be that practical with the timezones
> involved. I'm trying to get approval to go to Plumbers in which case we
> could setup a BOF but I have no guarantee at this point that I can make
> it happen.
> 
> So let's try using email as much possible for now.
> 
> Cheers,
> Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Mon, 2018-08-20 at 13:26 +0530, poza@codeaurora.org wrote:
>  now, I've sent a revert patch for the Documentation/ bit to
> > Bjorn, and I have no (not yet at least) beef in what you do in
> > drivers/pci/* ... however, that said, I think it would be great to move
> > EEH toward having a bulk of the policy use common code as well.
> > 
> 
> In my opinion, I think revert patch can wait, because again we might 
> need to change it according to improvements we bring.

I doubt it will differ much from what's there ... anyway...

> > It will be long road, in part due to the historical crappyness of our
> > EEH code, so my thinking is we should:
> > 
> >  - First agree on what we want the policy to be. I need to read a bit
> > more about DPC since that's new to me, it seems to be similar to what
> > our EEH does, with slighty less granularity (we can freeze access to
> > individual functions for example).
> > 
> >  - Rework err.c to implement that policy with the existing AER and DPC
> > code.
> > 
> 
> I went through eeh-pci-error-recovery.txt
> If I pick up folowing
> 
> "
> If the OS or device driver suspects that a PCI slot has been 
> EEH-isolated, there is a firmware call it can make to determine if this 
> is the case. If so, then the device driver should put itself into a 
> consistent state (given that it won't be able to complete any pending 
> work) and start recovery of the card.  Recovery normally would consist 
> of resetting the PCI device (holding the PCI #RST line high for two 
> seconds), followed by setting up the device config space (the base 
> address registers (BAR's), latency timer, cache line size, interrupt 
> line, and so on).  This is followed by a reinitialization of the device 
> driver.  In a worst-case scenario, the power to the card can be toggled, 
> at least on hot-plug-capable slots.  In principle, layers far above the 
> device driver probably do not need to know that the PCI card has been 
> "rebooted" in this way; ideally, there should be at most a pause in 
> Ethernet/disk/USB I/O while the card is being reset.

This verbiage is a bit old :-) A bunch of it predates PCIe.

> If the card cannot be recovered after three or four resets, the 
> kernel/device driver should assume the worst-case scenario, that the 
> card has died completely, and report this error to the sysadmin. In 
> addition, error messages are reported through RTAS and also through 
> syslogd (/var/log/messages) to alert the sysadmin of PCI resets. The 
> correct way to deal with failed adapters is to use the standard PCI 
> hotplug tools to remove and replace the dead card.
> "
> 
> some differences: I find is:
> Current framework does not attempt recovery 3-4 times.
> 
> Besides If I grasp eeh-pci-error-recovery.txt correctly, (although I 
> could be easily wrong),
> mainly the difference is coming how we reset the device.

See below

> Linux uses SBR, while EEH seems to be using #PERST.
> PERST signal implementation might be board specific, e.g. we have 
> io-port-expander sitting on i2c to drive #PERST.
> we rely on SBR. and SBR also should be a good way to bring hotreset of 
> device.
> "All Lanes in the configured Link transmit TS1 Ordered Sets with the Hot 
> Reset bit 15 asserted and the configured Link and Lane numbers."
> although I am not sure between #PERST and SBR is there is any subtle 
> differences such as some sticky bits might not be affected by SBR.
> but so far SBR has served well for us.
> 
> Also I see that eeh-pci-error-recovery.txt does not seem to distinguish 
> between fatal and nonfatal errors, is the behavior same for both type of 
> errors in EEH ?

Yeah so...

> I would like to mention is that there should nto be any need to reset 
> the card in case of NONFATAL because by definition link is functional, 
> only the particular transaction
> had a problem. so not sure how EEH deals with ERR_NONFATAL.

As I believe I said earlier... NONFATAL means the link is still usable.
It doesn't mean *anything* as far as the state of the device itself is
concerned. The device microcode could have crashed for example etc...

That is why it's important to honor a driver requesting a reset.

Ignoring the verbiage in the documents for a minute, think of the big
picture... the idea is that on error, there could be many actors
involved in the recovery. The brigde, the link itself (NONFATAL vs.
FATAL matters here), the device, the driver .... 

So the design is that it's always permitted to perform a "deeper"
action than strictly necessary. IE: If the link is ok, the device might
still require a reset. If the device is ok, the platform might still
enforce a reset.... 

There can be multiple devices or drivers involved in a recovery
operation (for example if the error came from a switch).

The difference between FATAL and NON_FATAL is PCIe specific and only
really matters from the perspective that if it's FATAL the core will
require a reset. It doens't mean some other agent can't request one too
when NON_FATAL errors occur.

As for EEH, *any* error that isn't purely informational results in the
adapter being isolated physcially by some dedicated HW. It's possible
to selectively un-isolate but the policy has generally been to just go
through the reset sequence as it was generally deemed safer.

It's hard enough to properly test and validate recovery path, it's
almost impossible to test and verify every way the HW can recover from
a all type of non-fatal errors.

Thus by enforcing that on any error we simply reset the adapter provide
a slower but much more robust recovery mechanism that is also a lot
more testable.

> Also I am very much interested in knowing original intention of DPC 
> driver to unplug/plug devices,
> all I remember in some conversation was:
> hotplug capable bridge might have see devices changed, so it is safer to 
> remove/unplug the devices and during which .shutdown methods of driver 
> is called, in case of ERR_FATAL.

The device being "swapped" during an error recovery operation is ...
unlikely. Do the bridges have a way to latch that the presence detect
changed ?

> although DPC is HW recovery while AER is sw recovery both should 
> fundamentally act in the same way as far as device drivers callbacks are 
> concerned.
> (again I really dont know real motivation behind this)

The main problem with unplug/replug (as I mentioned earlier) is that it
just does NOT work for storage controllers (or similar type of
devices). The links between the storage controller and the mounted
filesystems is lost permanently, you'll most likely have to reboot the
machine.

With our current EEH implementation we can successfully recover from
fatal errors with the storage controller by resetting it.

Finally as for PERST vs. Hot Reset, this is an implementation detail.
Not all our platforms can control PERST on all devices either, we
generally prefer PERST as it provides a more reliable reset mechanism
in practice (talking from experience) but will fallback to hot reset.

Cheers,
Ben.

> 
> Regards,
> Oza.
> 
> >  - Figure out what hooks might be needed to be able to plumb EEH into
> > it, possibly removing a bunch of crap in arch/powerpc (yay !)
> > 
> > I don't think having a webex will be that practical with the timezones
> > involved. I'm trying to get approval to go to Plumbers in which case we
> > could setup a BOF but I have no guarantee at this point that I can make
> > it happen.
> > 
> > So let's try using email as much possible for now.
> > 
> > Cheers,
> > Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-20 16:52, Benjamin Herrenschmidt wrote:
> On Mon, 2018-08-20 at 13:26 +0530, poza@codeaurora.org wrote:
>>  now, I've sent a revert patch for the Documentation/ bit to
>> > Bjorn, and I have no (not yet at least) beef in what you do in
>> > drivers/pci/* ... however, that said, I think it would be great to move
>> > EEH toward having a bulk of the policy use common code as well.
>> >
>> 
>> In my opinion, I think revert patch can wait, because again we might
>> need to change it according to improvements we bring.
> 
> I doubt it will differ much from what's there ... anyway...
> 
>> > It will be long road, in part due to the historical crappyness of our
>> > EEH code, so my thinking is we should:
>> >
>> >  - First agree on what we want the policy to be. I need to read a bit
>> > more about DPC since that's new to me, it seems to be similar to what
>> > our EEH does, with slighty less granularity (we can freeze access to
>> > individual functions for example).
>> >
>> >  - Rework err.c to implement that policy with the existing AER and DPC
>> > code.
>> >
>> 
>> I went through eeh-pci-error-recovery.txt
>> If I pick up folowing
>> 
>> "
>> If the OS or device driver suspects that a PCI slot has been
>> EEH-isolated, there is a firmware call it can make to determine if 
>> this
>> is the case. If so, then the device driver should put itself into a
>> consistent state (given that it won't be able to complete any pending
>> work) and start recovery of the card.  Recovery normally would consist
>> of resetting the PCI device (holding the PCI #RST line high for two
>> seconds), followed by setting up the device config space (the base
>> address registers (BAR's), latency timer, cache line size, interrupt
>> line, and so on).  This is followed by a reinitialization of the 
>> device
>> driver.  In a worst-case scenario, the power to the card can be 
>> toggled,
>> at least on hot-plug-capable slots.  In principle, layers far above 
>> the
>> device driver probably do not need to know that the PCI card has been
>> "rebooted" in this way; ideally, there should be at most a pause in
>> Ethernet/disk/USB I/O while the card is being reset.
> 
> This verbiage is a bit old :-) A bunch of it predates PCIe.
> 
>> If the card cannot be recovered after three or four resets, the
>> kernel/device driver should assume the worst-case scenario, that the
>> card has died completely, and report this error to the sysadmin. In
>> addition, error messages are reported through RTAS and also through
>> syslogd (/var/log/messages) to alert the sysadmin of PCI resets. The
>> correct way to deal with failed adapters is to use the standard PCI
>> hotplug tools to remove and replace the dead card.
>> "
>> 
>> some differences: I find is:
>> Current framework does not attempt recovery 3-4 times.
>> 
>> Besides If I grasp eeh-pci-error-recovery.txt correctly, (although I
>> could be easily wrong),
>> mainly the difference is coming how we reset the device.
> 
> See below
> 
>> Linux uses SBR, while EEH seems to be using #PERST.
>> PERST signal implementation might be board specific, e.g. we have
>> io-port-expander sitting on i2c to drive #PERST.
>> we rely on SBR. and SBR also should be a good way to bring hotreset of
>> device.
>> "All Lanes in the configured Link transmit TS1 Ordered Sets with the 
>> Hot
>> Reset bit 15 asserted and the configured Link and Lane numbers."
>> although I am not sure between #PERST and SBR is there is any subtle
>> differences such as some sticky bits might not be affected by SBR.
>> but so far SBR has served well for us.
>> 
>> Also I see that eeh-pci-error-recovery.txt does not seem to 
>> distinguish
>> between fatal and nonfatal errors, is the behavior same for both type 
>> of
>> errors in EEH ?
> 
> Yeah so...
> 
>> I would like to mention is that there should nto be any need to reset
>> the card in case of NONFATAL because by definition link is functional,
>> only the particular transaction
>> had a problem. so not sure how EEH deals with ERR_NONFATAL.
> 
> As I believe I said earlier... NONFATAL means the link is still usable.
> It doesn't mean *anything* as far as the state of the device itself is
> concerned. The device microcode could have crashed for example etc...
> 
> That is why it's important to honor a driver requesting a reset.
> 
> Ignoring the verbiage in the documents for a minute, think of the big
> picture... the idea is that on error, there could be many actors
> involved in the recovery. The brigde, the link itself (NONFATAL vs.
> FATAL matters here), the device, the driver ....
> 
> So the design is that it's always permitted to perform a "deeper"
> action than strictly necessary. IE: If the link is ok, the device might
> still require a reset. If the device is ok, the platform might still
> enforce a reset....
> 
> There can be multiple devices or drivers involved in a recovery
> operation (for example if the error came from a switch).
> 
> The difference between FATAL and NON_FATAL is PCIe specific and only
> really matters from the perspective that if it's FATAL the core will
> require a reset. It doens't mean some other agent can't request one too
> when NON_FATAL errors occur.
> 
> As for EEH, *any* error that isn't purely informational results in the
> adapter being isolated physcially by some dedicated HW. It's possible
> to selectively un-isolate but the policy has generally been to just go
> through the reset sequence as it was generally deemed safer.
> 
> It's hard enough to properly test and validate recovery path, it's
> almost impossible to test and verify every way the HW can recover from
> a all type of non-fatal errors.
> 
> Thus by enforcing that on any error we simply reset the adapter provide
> a slower but much more robust recovery mechanism that is also a lot
> more testable.

Hi Ben,

I get the idea and get your points. and when I started implementation of 
this, I did ask these questions to myself.
but then we all fell aligned to what DPC was doing, because thats how 
the driver was dealing with ERR_FATAL.

I can put together a patch adhering to the idea, and modify err.c.
let me know if you are okay with that.
I have both DPC and AER capable root port, so I can easily validate the 
changes as we improve upon this.

besides We have to come to some common ground on policy.
1) if device driver dictates the reset we should agree to do SBR. 
(irrespective of the type of error)
2) under what circumstances framework will impose the reset, even if 
device driver did not !

probably changes might be simpler; although it will require to exercise 
some tests cases for both DPC and AER.
let me know if anything I missed here, but let me attempt to make patch 
and we can go form there.

Let me know you opinion.

Regards,
Oza.

> 
>> Also I am very much interested in knowing original intention of DPC
>> driver to unplug/plug devices,
>> all I remember in some conversation was:
>> hotplug capable bridge might have see devices changed, so it is safer 
>> to
>> remove/unplug the devices and during which .shutdown methods of driver
>> is called, in case of ERR_FATAL.
> 
> The device being "swapped" during an error recovery operation is ...
> unlikely. Do the bridges have a way to latch that the presence detect
> changed ?
> 
>> although DPC is HW recovery while AER is sw recovery both should
>> fundamentally act in the same way as far as device drivers callbacks 
>> are
>> concerned.
>> (again I really dont know real motivation behind this)
> 
> The main problem with unplug/replug (as I mentioned earlier) is that it
> just does NOT work for storage controllers (or similar type of
> devices). The links between the storage controller and the mounted
> filesystems is lost permanently, you'll most likely have to reboot the
> machine.
> 
> With our current EEH implementation we can successfully recover from
> fatal errors with the storage controller by resetting it.
> 
> Finally as for PERST vs. Hot Reset, this is an implementation detail.
> Not all our platforms can control PERST on all devices either, we
> generally prefer PERST as it provides a more reliable reset mechanism
> in practice (talking from experience) but will fallback to hot reset.
> 
> Cheers,
> Ben.
> 
>> 
>> Regards,
>> Oza.
>> 
>> >  - Figure out what hooks might be needed to be able to plumb EEH into
>> > it, possibly removing a bunch of crap in arch/powerpc (yay !)
>> >
>> > I don't think having a webex will be that practical with the timezones
>> > involved. I'm trying to get approval to go to Plumbers in which case we
>> > could setup a BOF but I have no guarantee at this point that I can make
>> > it happen.
>> >
>> > So let's try using email as much possible for now.
>> >
>> > Cheers,
>> > Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Mon, 2018-08-20 at 18:56 +0530, poza@codeaurora.org wrote:
> Hi Ben,
> 
> I get the idea and get your points. and when I started implementation of 
> this, I did ask these questions to myself.
> but then we all fell aligned to what DPC was doing, because thats how 
> the driver was dealing with ERR_FATAL.
> 
> I can put together a patch adhering to the idea, and modify err.c.
> let me know if you are okay with that.
> I have both DPC and AER capable root port, so I can easily validate the 
> changes as we improve upon this.
> 
> besides We have to come to some common ground on policy.
> 1) if device driver dictates the reset we should agree to do SBR. 
> (irrespective of the type of error)

It's not just "the driver dictates the reset". It's a combination of
all agents. Either the driver or the framework, *both* can request a
reset. Also if you have multiple devices involved (for example multiple
functions), then any of the drivers can request the reset.

> 2) under what circumstances framework will impose the reset, even if 
> device driver did not !

Well ERR_FATAL typically would be one. Make it an argument to the
framework, it's possible that EEH might always do, I have to look into
it.

> probably changes might be simpler; although it will require to exercise 
> some tests cases for both DPC and AER.
> let me know if anything I missed here, but let me attempt to make patch 
> and we can go form there.
> 
> Let me know you opinion.

I agree. Hopefully Sam will have a chance to chime in (he's caught up
with something else at the moment) as well.

Cheers,
Ben.

> Regards,
> Oza.
> 
> > 
> > > Also I am very much interested in knowing original intention of DPC
> > > driver to unplug/plug devices,
> > > all I remember in some conversation was:
> > > hotplug capable bridge might have see devices changed, so it is safer 
> > > to
> > > remove/unplug the devices and during which .shutdown methods of driver
> > > is called, in case of ERR_FATAL.
> > 
> > The device being "swapped" during an error recovery operation is ...
> > unlikely. Do the bridges have a way to latch that the presence detect
> > changed ?
> > 
> > > although DPC is HW recovery while AER is sw recovery both should
> > > fundamentally act in the same way as far as device drivers callbacks 
> > > are
> > > concerned.
> > > (again I really dont know real motivation behind this)
> > 
> > The main problem with unplug/replug (as I mentioned earlier) is that it
> > just does NOT work for storage controllers (or similar type of
> > devices). The links between the storage controller and the mounted
> > filesystems is lost permanently, you'll most likely have to reboot the
> > machine.
> > 
> > With our current EEH implementation we can successfully recover from
> > fatal errors with the storage controller by resetting it.
> > 
> > Finally as for PERST vs. Hot Reset, this is an implementation detail.
> > Not all our platforms can control PERST on all devices either, we
> > generally prefer PERST as it provides a more reliable reset mechanism
> > in practice (talking from experience) but will fallback to hot reset.
> > 
> > Cheers,
> > Ben.
> > 
> > > 
> > > Regards,
> > > Oza.
> > > 
> > > >  - Figure out what hooks might be needed to be able to plumb EEH into
> > > > it, possibly removing a bunch of crap in arch/powerpc (yay !)
> > > > 
> > > > I don't think having a webex will be that practical with the timezones
> > > > involved. I'm trying to get approval to go to Plumbers in which case we
> > > > could setup a BOF but I have no guarantee at this point that I can make
> > > > it happen.
> > > > 
> > > > So let's try using email as much possible for now.
> > > > 
> > > > Cheers,
> > > > Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-21 02:32, Benjamin Herrenschmidt wrote:
> On Mon, 2018-08-20 at 18:56 +0530, poza@codeaurora.org wrote:
>> Hi Ben,
>> 
>> I get the idea and get your points. and when I started implementation 
>> of
>> this, I did ask these questions to myself.
>> but then we all fell aligned to what DPC was doing, because thats how
>> the driver was dealing with ERR_FATAL.
>> 
>> I can put together a patch adhering to the idea, and modify err.c.
>> let me know if you are okay with that.
>> I have both DPC and AER capable root port, so I can easily validate 
>> the
>> changes as we improve upon this.
>> 
>> besides We have to come to some common ground on policy.
>> 1) if device driver dictates the reset we should agree to do SBR.
>> (irrespective of the type of error)
> 
> It's not just "the driver dictates the reset". It's a combination of
> all agents. Either the driver or the framework, *both* can request a
> reset. Also if you have multiple devices involved (for example multiple
> functions), then any of the drivers can request the reset.
> 
>> 2) under what circumstances framework will impose the reset, even if
>> device driver did not !
> 
> Well ERR_FATAL typically would be one. Make it an argument to the
> framework, it's possible that EEH might always do, I have to look into
> it.
> 
>> probably changes might be simpler; although it will require to 
>> exercise
>> some tests cases for both DPC and AER.
>> let me know if anything I missed here, but let me attempt to make 
>> patch
>> and we can go form there.
>> 
>> Let me know you opinion.
> 
> I agree. Hopefully Sam will have a chance to chime in (he's caught up
> with something else at the moment) as well.
> 
> Cheers,
> Ben.
> 
>> Regards,
>> Oza.
>> 
>> >
>> > > Also I am very much interested in knowing original intention of DPC
>> > > driver to unplug/plug devices,
>> > > all I remember in some conversation was:
>> > > hotplug capable bridge might have see devices changed, so it is safer
>> > > to
>> > > remove/unplug the devices and during which .shutdown methods of driver
>> > > is called, in case of ERR_FATAL.
>> >
>> > The device being "swapped" during an error recovery operation is ...
>> > unlikely. Do the bridges have a way to latch that the presence detect
>> > changed ?
>> >
>> > > although DPC is HW recovery while AER is sw recovery both should
>> > > fundamentally act in the same way as far as device drivers callbacks
>> > > are
>> > > concerned.
>> > > (again I really dont know real motivation behind this)
>> >
>> > The main problem with unplug/replug (as I mentioned earlier) is that it
>> > just does NOT work for storage controllers (or similar type of
>> > devices). The links between the storage controller and the mounted
>> > filesystems is lost permanently, you'll most likely have to reboot the
>> > machine.
>> >
>> > With our current EEH implementation we can successfully recover from
>> > fatal errors with the storage controller by resetting it.
>> >
>> > Finally as for PERST vs. Hot Reset, this is an implementation detail.
>> > Not all our platforms can control PERST on all devices either, we
>> > generally prefer PERST as it provides a more reliable reset mechanism
>> > in practice (talking from experience) but will fallback to hot reset.
>> >
>> > Cheers,
>> > Ben.

Ok Let me summarize the so far discussed things.

It would be nice if we all (Bjorn, Keith, Ben, Sinan) can hold consensus 
on this.

1) Right now AER and DPC both calls pcie_do_fatal_recovery(), I majorly 
see DPC as error handling and recovery agent rather than being used for 
hotplug.
    so in my opinion, both AER and DPC should have same error handling 
and recovery mechanism

    so if there is a way to figure out that in absence of pcihp, if DPC 
is being used to support hotplug then we fall back to original DPC 
mechanism (which is remove devices)
    otherwise, we fall back to drivers callbacks.

    Spec 6.7.5 Async Removal
    "
    The Surprise Down error resulting from async removal may trigger 
Downstream Port Containment (See Section 6.2.10).
    Downstream Port Containment following an async removal may be 
utilized to hold the Link of a
    Downstream Port in the Disabled LTSSM state while host software 
recovers from the side effects of an async removal.
    "

    I think above is implementation specific. but there has to be some 
way to kow if we are using DPC for hotplug or not !
    otherwise it is assumed to be used for error handling and recovery

    pcie_do_fatal_recovery should take care of above. so that we support 
both error handling and async removal from DPC point of view.


2) It is left to driver first to determine whether it wants to requests 
the reset of device or not based on sanity of link (e.g. if it is 
reading 0xffffffff back !)
    pci_channel_io_frozen is the one which should be used from framework 
to communicate driver that this is ERR_FATAL.

3) @Ben, although I am not very clear that if there is an ERR_FATAL, in 
what circumstances driver will deny the reset but framework will impose 
reset ?

4) Sinan's patch is going to ignore link states if it finds ERR_FATAL,so 
that pcihp will not trigger and unplug the devices.


5) pcie_do_nonfatal_recovery; we sould actually reset the link e.g. SBR 
if driver requests the reset of link.  (there is already TDO note 
anyway).
    if (status == PCI_ERS_RESULT_NEED_RESET) {
		/*
		 * TODO: Should call platform-specific
		 * functions to reset slot before calling
		 * drivers' slot_reset callbacks?
		 */
		status = broadcast_error_message(dev,
				state,
				"slot_reset",
				report_slot_reset);
	}


Let me know how this sounds, if we all agree on behavior I can go ahead 
with the changes.

ps: although I need input on point-1.

Regards,
Oza.

>> >
>> > >
>> > > Regards,
>> > > Oza.
>> > >
>> > > >  - Figure out what hooks might be needed to be able to plumb EEH into
>> > > > it, possibly removing a bunch of crap in arch/powerpc (yay !)
>> > > >
>> > > > I don't think having a webex will be that practical with the timezones
>> > > > involved. I'm trying to get approval to go to Plumbers in which case we
>> > > > could setup a BOF but I have no guarantee at this point that I can make
>> > > > it happen.
>> > > >
>> > > > So let's try using email as much possible for now.
>> > > >
>> > > > Cheers,
>> > > > Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Tue, 2018-08-21 at 10:44 +0530, poza@codeaurora.org wrote:
> 
> Ok Let me summarize the so far discussed things.
> 
> It would be nice if we all (Bjorn, Keith, Ben, Sinan) can hold consensus 
> on this.
> 
> 1) Right now AER and DPC both calls pcie_do_fatal_recovery(), I majorly 
> see DPC as error handling and recovery agent rather than being used for 
> hotplug.
>     so in my opinion, both AER and DPC should have same error handling 
> and recovery mechanism

Yes.

>     so if there is a way to figure out that in absence of pcihp, if DPC 
> is being used to support hotplug then we fall back to original DPC 
> mechanism (which is remove devices)

Not exactly. If the presence detect change indicates it was a hotplug
event rather.

>     otherwise, we fall back to drivers callbacks.

Driver callback should be the primary mechanism unless we think there's
a chance the device was removed (or the driver has no callbacks)

>     Spec 6.7.5 Async Removal
>     "
>     The Surprise Down error resulting from async removal may trigger 
> Downstream Port Containment (See Section 6.2.10).
>     Downstream Port Containment following an async removal may be 
> utilized to hold the Link of a
>     Downstream Port in the Disabled LTSSM state while host software 
> recovers from the side effects of an async removal.
>     "
> 
>     I think above is implementation specific. but there has to be some 
> way to kow if we are using DPC for hotplug or not !
>     otherwise it is assumed to be used for error handling and recovery
> 
>     pcie_do_fatal_recovery should take care of above. so that we support 
> both error handling and async removal from DPC point of view.
> 
> 
> 2) It is left to driver first to determine whether it wants to requests 
> the reset of device or not based on sanity of link (e.g. if it is 
> reading 0xffffffff back !)

The need to reset is the logical OR of the driver wanting a reset and
the framework wanting a reset. If *either* wants a reset, then we do a
reset. Typically the framework would request one for ERR_FATAL.

Actually this is a bit more convoluted than that. There can be more
than one driver involved in a given error event, for example because
the device might have multiple functions, or because the error could
have been reported by an upstream bridge.

So the need to reset is actually the logical OR of *any driver*
involved returning PCI_ERS_RESULT_NEED_RESET with the framework wanting
to reset.

>     pci_channel_io_frozen is the one which should be used from framework 
> to communicate driver that this is ERR_FATAL.

Maybe, not sure ... on EEH we have special HW that freezes on all non-
corrected errors but maybe for AER/DPC that's the way to go.

> 3) @Ben, although I am not very clear that if there is an ERR_FATAL, in 
> what circumstances driver will deny the reset but framework will impose 
> reset ?

The driver doesn't "deny" the reset. The driver just says it can
recover without one. 

The driver doesn't necessarily need to care  as to whether the error
was fatal or not, it cares about whether its device seems functional.
The driver can check some diagnostic registers, makes sure the firmware
hasn't crashed etc... and might want to request a reset if it's unhappy
with the state of the device for example.

The framework will impose a reset on ERR_FATAL  typcally (or because
platform policy is to always reset which might be our choice on pwoerpc
with EEH, I'll have to think about it).

> 4) Sinan's patch is going to ignore link states if it finds ERR_FATAL,so 
> that pcihp will not trigger and unplug the devices.

I would suggest checking the prsence detect change latch if it's
supported rather. If not supported, maybe ERR_FATAL is a good fallback.

> 5) pcie_do_nonfatal_recovery; we sould actually reset the link e.g. SBR 
> if driver requests the reset of link.  (there is already TDO note 
> anyway).
>     if (status == PCI_ERS_RESULT_NEED_RESET) {
> 		/*
> 		 * TODO: Should call platform-specific
> 		 * functions to reset slot before calling
> 		 * drivers' slot_reset callbacks?
> 		 */
> 		status = broadcast_error_message(dev,
> 				state,
> 				"slot_reset",
> 				report_slot_reset);
> 	}
> 

Yes.

> Let me know how this sounds, if we all agree on behavior I can go ahead 
> with the changes.

> ps: although I need input on point-1.

Ideally we would like to look at making EEH use the generic framework
as well, though because of subtle differences on how our HW freezes
stuff etc... we might have to add a few quirks to it.

Cheers,
Ben.

> Regards,
> Oza.
> 
> > > > 
> > > > > 
> > > > > Regards,
> > > > > Oza.
> > > > > 
> > > > > >  - Figure out what hooks might be needed to be able to plumb EEH into
> > > > > > it, possibly removing a bunch of crap in arch/powerpc (yay !)
> > > > > > 
> > > > > > I don't think having a webex will be that practical with the timezones
> > > > > > involved. I'm trying to get approval to go to Plumbers in which case we
> > > > > > could setup a BOF but I have no guarantee at this point that I can make
> > > > > > it happen.
> > > > > > 
> > > > > > So let's try using email as much possible for now.
> > > > > > 
> > > > > > Cheers,
> > > > > > Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Keith Busch]

On Tue, Aug 21, 2018 at 04:06:30PM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2018-08-21 at 10:44 +0530, poza@codeaurora.org wrote:
> > 
> > Ok Let me summarize the so far discussed things.
> > 
> > It would be nice if we all (Bjorn, Keith, Ben, Sinan) can hold consensus 
> > on this.
> > 
> > 1) Right now AER and DPC both calls pcie_do_fatal_recovery(), I majorly 
> > see DPC as error handling and recovery agent rather than being used for 
> > hotplug.
> >     so in my opinion, both AER and DPC should have same error handling 
> > and recovery mechanism
> 
> Yes.
>
> >     so if there is a way to figure out that in absence of pcihp, if DPC 
> > is being used to support hotplug then we fall back to original DPC 
> > mechanism (which is remove devices)
> 
> Not exactly. If the presence detect change indicates it was a hotplug
> event rather.

The actions associated with error recovery will trigger link state changes
for a lot of existing hardware. PCIEHP currently does the same removal
sequence for both link state change (DLLSC) and presence detect change
(PDC) events.

It sounds like you want pciehp to do nothing on the DLLSC events that it
currently handles, and instead do the board removal only on PDC.  If that
is the case, is the desire to not remove devices downstream a permanently
disabled link, or does that responsibility fall onto some other component?

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Sinan Kaya]

On 8/21/2018 10:37 AM, Keith Busch wrote:
> On Tue, Aug 21, 2018 at 04:06:30PM +1000, Benjamin Herrenschmidt wrote:
>> On Tue, 2018-08-21 at 10:44 +0530, poza@codeaurora.org wrote:
>>>
>>> Ok Let me summarize the so far discussed things.
>>>
>>> It would be nice if we all (Bjorn, Keith, Ben, Sinan) can hold consensus
>>> on this.
>>>
>>> 1) Right now AER and DPC both calls pcie_do_fatal_recovery(), I majorly
>>> see DPC as error handling and recovery agent rather than being used for
>>> hotplug.
>>>      so in my opinion, both AER and DPC should have same error handling
>>> and recovery mechanism
>>
>> Yes.
>>
>>>      so if there is a way to figure out that in absence of pcihp, if DPC
>>> is being used to support hotplug then we fall back to original DPC
>>> mechanism (which is remove devices)
>>
>> Not exactly. If the presence detect change indicates it was a hotplug
>> event rather.
> 
> The actions associated with error recovery will trigger link state changes
> for a lot of existing hardware. PCIEHP currently does the same removal
> sequence for both link state change (DLLSC) and presence detect change
> (PDC) events.
> 
> It sounds like you want pciehp to do nothing on the DLLSC events that it
> currently handles, and instead do the board removal only on PDC.  If that
> is the case, is the desire to not remove devices downstream a permanently
> disabled link, or does that responsibility fall onto some other component?
> 

Looking at PDC is not enough. Hotplug driver handles both physical
removal as well as the link going down due to signal integrity issues today.

If the link went down because of a pending FATAL error, AER/DPC recovers
the link automatically. There is no need for hotplug to be involved in
fatal error work.

Hotplug driver needs to handle both physical removal as well as 
intermittent link down issues though.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Keith Busch]

On Tue, Aug 21, 2018 at 11:07:31AM -0400, Sinan Kaya wrote:
> On 8/21/2018 10:37 AM, Keith Busch wrote:
> > The actions associated with error recovery will trigger link state changes
> > for a lot of existing hardware. PCIEHP currently does the same removal
> > sequence for both link state change (DLLSC) and presence detect change
> > (PDC) events.
> > 
> > It sounds like you want pciehp to do nothing on the DLLSC events that it
> > currently handles, and instead do the board removal only on PDC.  If that
> > is the case, is the desire to not remove devices downstream a permanently
> > disabled link, or does that responsibility fall onto some other component?
> > 
> 
> Looking at PDC is not enough. Hotplug driver handles both physical
> removal as well as the link going down due to signal integrity issues today.
> 
> If the link went down because of a pending FATAL error, AER/DPC recovers
> the link automatically. There is no need for hotplug to be involved in
> fatal error work.
> 
> Hotplug driver needs to handle both physical removal as well as intermittent
> link down issues though.

Back to your patch you linked to earlier, your proposal is to have
pciehp wait for DEVSTS.FED before deciding if it needs to handle the
DLLSC event. That might be a start, but it isn't enough since that
status isn't set if the downstream device reported ERR_FATAL. I think
you'd need to check the secondary status register for a Received System
Error.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Sinan Kaya]

On 8/21/2018 11:29 AM, Keith Busch wrote:
>> Hotplug driver needs to handle both physical removal as well as intermittent
>> link down issues though.
> Back to your patch you linked to earlier, your proposal is to have
> pciehp wait for DEVSTS.FED before deciding if it needs to handle the
> DLLSC event. That might be a start, but it isn't enough since that
> status isn't set if the downstream device reported ERR_FATAL. I think
> you'd need to check the secondary status register for a Received System
> Error.

Hmm, good feedback.

I was trying not to mix AER/DPC with HP but obviously I failed.
I can add a flag to struct pci_dev like aer_pending for AER.

1. AER ISR sets aer_pending
2. AER ISR issues a secondary bus reset
3. Hotplug driver bails out on aer_pending
4. AER ISR performs the recovery

It is slightly more challenging on the DPC front as HW brings down
the link automatically and hotplug driver can observe the link down
event before the DPC ISR. I'll have to go check if DPC interrupt is
pending.

Let me know if this works out.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Sinan Kaya]

On 8/21/2018 11:50 AM, Sinan Kaya wrote:
> On 8/21/2018 11:29 AM, Keith Busch wrote:
>>> Hotplug driver needs to handle both physical removal as well as 
>>> intermittent
>>> link down issues though.
>> Back to your patch you linked to earlier, your proposal is to have
>> pciehp wait for DEVSTS.FED before deciding if it needs to handle the
>> DLLSC event. That might be a start, but it isn't enough since that
>> status isn't set if the downstream device reported ERR_FATAL. I think
>> you'd need to check the secondary status register for a Received System
>> Error.
> 
> Hmm, good feedback.
> 
> I was trying not to mix AER/DPC with HP but obviously I failed.
> I can add a flag to struct pci_dev like aer_pending for AER.
> 
> 1. AER ISR sets aer_pending
> 2. AER ISR issues a secondary bus reset
> 3. Hotplug driver bails out on aer_pending
> 4. AER ISR performs the recovery
> 
> It is slightly more challenging on the DPC front as HW brings down
> the link automatically and hotplug driver can observe the link down
> event before the DPC ISR. I'll have to go check if DPC interrupt is
> pending.
> 
> Let me know if this works out.

I forgot to mention that I didn't really want to rely on Secondary
Status Registers as most of the recent PCIe controller HW don't really
implement the old PCI status register bits correctly.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Keith Busch]

On Tue, Aug 21, 2018 at 11:50:54AM -0400, Sinan Kaya wrote:
> On 8/21/2018 11:29 AM, Keith Busch wrote:
> > > Hotplug driver needs to handle both physical removal as well as intermittent
> > > link down issues though.
> > Back to your patch you linked to earlier, your proposal is to have
> > pciehp wait for DEVSTS.FED before deciding if it needs to handle the
> > DLLSC event. That might be a start, but it isn't enough since that
> > status isn't set if the downstream device reported ERR_FATAL. I think
> > you'd need to check the secondary status register for a Received System
> > Error.
> 
> Hmm, good feedback.
> 
> I was trying not to mix AER/DPC with HP but obviously I failed.
> I can add a flag to struct pci_dev like aer_pending for AER.
> 
> 1. AER ISR sets aer_pending
> 2. AER ISR issues a secondary bus reset
> 3. Hotplug driver bails out on aer_pending
> 4. AER ISR performs the recovery
> 
> It is slightly more challenging on the DPC front as HW brings down
> the link automatically and hotplug driver can observe the link down
> event before the DPC ISR. I'll have to go check if DPC interrupt is
> pending.
> 
> Let me know if this works out.

That sounds like a step in a good direction.

I think it's also safe to say the spec requires a slot capable of swapping
devices report the PDC slot event, so that should be valid criteria for
knowing if a hotplug event occured. There are other issues that we may
need to fix.

Consider a hierarchy of switches where the root port starts a DPC
event and the kernel tries to recover. Meanwhile devices downstream
switches lower in the hierarchy have changed. Since recovery doesn't do
re-enumeration and the top level downstream port didn't detect a hotplug
event, the recovery will "initialize" the wrong device to potentially
undefined results.

I think we'd need pciehp involved when error recovery rewalks the
downstream buses so pciehp may check PDC on capable ports. Implementing
'error_resume' in pciehp to de-enumerate/re-enumerate accordingly might
work ... If that happened, pciehp could call pcie_do_fatal_recovery()
and all three services could be unified!

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-21 20:07, Keith Busch wrote:
> On Tue, Aug 21, 2018 at 04:06:30PM +1000, Benjamin Herrenschmidt wrote:
>> On Tue, 2018-08-21 at 10:44 +0530, poza@codeaurora.org wrote:
>> >
>> > Ok Let me summarize the so far discussed things.
>> >
>> > It would be nice if we all (Bjorn, Keith, Ben, Sinan) can hold consensus
>> > on this.
>> >
>> > 1) Right now AER and DPC both calls pcie_do_fatal_recovery(), I majorly
>> > see DPC as error handling and recovery agent rather than being used for
>> > hotplug.
>> >     so in my opinion, both AER and DPC should have same error handling
>> > and recovery mechanism
>> 
>> Yes.
>> 
>> >     so if there is a way to figure out that in absence of pcihp, if DPC
>> > is being used to support hotplug then we fall back to original DPC
>> > mechanism (which is remove devices)
>> 
>> Not exactly. If the presence detect change indicates it was a hotplug
>> event rather.
> 
> The actions associated with error recovery will trigger link state 
> changes
> for a lot of existing hardware. PCIEHP currently does the same removal
> sequence for both link state change (DLLSC) and presence detect change
> (PDC) events.
> 
> It sounds like you want pciehp to do nothing on the DLLSC events that 
> it
> currently handles, and instead do the board removal only on PDC.  If 
> that
> is the case, is the desire to not remove devices downstream a 
> permanently
> disabled link, or does that responsibility fall onto some other 
> component?

Keith

Are you in agreement with following ?

"
Right now AER and DPC both calls pcie_do_fatal_recovery(), I majorly see 
DPC as error handling and recovery agent rather than being used for 
hotplug.
    so in my opinion, both AER and DPC should have same error handling 
and recovery mechanism

    so if there is a way to figure out that in absence of pcihp, if DPC 
is being used to support hotplug then we fall back to original DPC 
mechanism (which is remove devices)
    otherwise, we fall back to drivers callbacks.

    Spec 6.7.5 Async Removal
    "
    The Surprise Down error resulting from async removal may trigger 
Downstream Port Containment (See Section 6.2.10).
    Downstream Port Containment following an async removal may be 
utilized to hold the Link of a
    Downstream Port in the Disabled LTSSM state while host software 
recovers from the side effects of an async removal.
    "

    I think above is implementation specific. but there has to be some 
way to kow if we are using DPC for hotplug or not !
    otherwise it is assumed to be used for error handling and recovery

    pcie_do_fatal_recovery should take care of above. so that we support 
both error handling and async removal from DPC point of view.
"

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Tue, 2018-08-21 at 08:37 -0600, Keith Busch wrote:
> > 
> > >      so if there is a way to figure out that in absence of pcihp, if DPC 
> > > is being used to support hotplug then we fall back to original DPC 
> > > mechanism (which is remove devices)
> > 
> > Not exactly. If the presence detect change indicates it was a hotplug
> > event rather.
> 
> The actions associated with error recovery will trigger link state changes
> for a lot of existing hardware. PCIEHP currently does the same removal
> sequence for both link state change (DLLSC) and presence detect change
> (PDC) events.
> 
> It sounds like you want pciehp to do nothing on the DLLSC events that it
> currently handles, and instead do the board removal only on PDC.  If that
> is the case, is the desire to not remove devices downstream a permanently
> disabled link, or does that responsibility fall onto some other component?

I think there need to be some coordination between pciehb and DPC on
link state change yes.

We could still remove the device if recovery fails. For example on EEH
we have a threshold and if a device fails more than N times within the
last M minutes (I don't remember the exact numbers and I'm not in front
of the code right now) we give up.

Also under some circumstances, the link will change as a result of the
error handling doing a hot reset.

For example, if the error happens in a parent bridge and that gets
reset, the entire hierarchy underneath does too.

We need to save/restore the state of all bridges and devices (BARs
etc...) below that.

Cheers,
Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Keith Busch]

On Wed, Aug 22, 2018 at 07:14:32AM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2018-08-21 at 08:37 -0600, Keith Busch wrote:
> > > 
> > > >      so if there is a way to figure out that in absence of pcihp, if DPC 
> > > > is being used to support hotplug then we fall back to original DPC 
> > > > mechanism (which is remove devices)
> > > 
> > > Not exactly. If the presence detect change indicates it was a hotplug
> > > event rather.
> > 
> > The actions associated with error recovery will trigger link state changes
> > for a lot of existing hardware. PCIEHP currently does the same removal
> > sequence for both link state change (DLLSC) and presence detect change
> > (PDC) events.
> > 
> > It sounds like you want pciehp to do nothing on the DLLSC events that it
> > currently handles, and instead do the board removal only on PDC.  If that
> > is the case, is the desire to not remove devices downstream a permanently
> > disabled link, or does that responsibility fall onto some other component?
> 
> I think there need to be some coordination between pciehb and DPC on
> link state change yes.
> 
> We could still remove the device if recovery fails. For example on EEH
> we have a threshold and if a device fails more than N times within the
> last M minutes (I don't remember the exact numbers and I'm not in front
> of the code right now) we give up.
> 
> Also under some circumstances, the link will change as a result of the
> error handling doing a hot reset.
> 
> For example, if the error happens in a parent bridge and that gets
> reset, the entire hierarchy underneath does too.
> 
> We need to save/restore the state of all bridges and devices (BARs
> etc...) below that.

That's not good enough. You'd at least need to check SLTSTS.PDC on all
bridge DSP's beneath the parent *before* you try to restore the state
of devices beneath them. Those races are primarily why DPC currently
removes and reenumerates instead, because that's not something that can
be readily coordinated today.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Keith Busch]

On Tue, Aug 21, 2018 at 04:04:56PM -0600, Keith Busch wrote:
> > For example, if the error happens in a parent bridge and that gets
> > reset, the entire hierarchy underneath does too.
> > 
> > We need to save/restore the state of all bridges and devices (BARs
> > etc...) below that.
> 
> That's not good enough. You'd at least need to check SLTSTS.PDC on all
> bridge DSP's beneath the parent *before* you try to restore the state
> of devices beneath them. Those races are primarily why DPC currently
> removes and reenumerates instead, because that's not something that can
> be readily coordinated today.

A picture of a real scenario to go with the above comments:

                    ----------------
  --------          |              |
  |      |       -------        -------       --------------
  |  RP  | <---> | USP | SWITCH | DSP | <---> | END DEVICE |
  |      |       ------         -------       --------------
  --------          |              |
                    ----------------

The downstream port (DSP) is hotplug capable, and the root port (RP)
has DPC enabled.

Lets say you hot swap END DEVICE at a time with an inflight posted
transaction such that the RP triggers a CTO, starting a DPC event.
If you treat the RP's DPC event as just "error handling" and restore
the state after containment is released, we are in undefined behavior
because of the mistaken idententy of the device below the SWITCH DSP.

Re-enumerating the topology is easy since it has no races like that. I
understand re-enumeration is problematic for some scenarios, so if we
really need to avoid re-enumeration except when absolutely necessary,
it should be possible if we can detangle the necessary coordination
between the port service drivers.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Tue, 2018-08-21 at 16:04 -0600, Keith Busch wrote:
> > I think there need to be some coordination between pciehb and DPC on
> > link state change yes.
> > 
> > We could still remove the device if recovery fails. For example on EEH
> > we have a threshold and if a device fails more than N times within the
> > last M minutes (I don't remember the exact numbers and I'm not in front
> > of the code right now) we give up.
> > 
> > Also under some circumstances, the link will change as a result of the
> > error handling doing a hot reset.
> > 
> > For example, if the error happens in a parent bridge and that gets
> > reset, the entire hierarchy underneath does too.
> > 
> > We need to save/restore the state of all bridges and devices (BARs
> > etc...) below that.
> 
> That's not good enough. You'd at least need to check SLTSTS.PDC on all
> bridge DSP's beneath the parent *before* you try to restore the state
> of devices beneath them. Those races are primarily why DPC currently
> removes and reenumerates instead, because that's not something that can
> be readily coordinated today.

It can be probably done by a simple test & skip as you go down
restoring state, then handling the removals after the dance is
complete.

Cheers,
Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Keith Busch]

On Wed, Aug 22, 2018 at 09:06:57AM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2018-08-21 at 16:04 -0600, Keith Busch wrote:
> > > I think there need to be some coordination between pciehb and DPC on
> > > link state change yes.
> > > 
> > > We could still remove the device if recovery fails. For example on EEH
> > > we have a threshold and if a device fails more than N times within the
> > > last M minutes (I don't remember the exact numbers and I'm not in front
> > > of the code right now) we give up.
> > > 
> > > Also under some circumstances, the link will change as a result of the
> > > error handling doing a hot reset.
> > > 
> > > For example, if the error happens in a parent bridge and that gets
> > > reset, the entire hierarchy underneath does too.
> > > 
> > > We need to save/restore the state of all bridges and devices (BARs
> > > etc...) below that.
> > 
> > That's not good enough. You'd at least need to check SLTSTS.PDC on all
> > bridge DSP's beneath the parent *before* you try to restore the state
> > of devices beneath them. Those races are primarily why DPC currently
> > removes and reenumerates instead, because that's not something that can
> > be readily coordinated today.
> 
> It can be probably done by a simple test & skip as you go down
> restoring state, then handling the removals after the dance is
> complete.

You can't just test the states in config space. You're racing with
pciehp's ISR, which clears those states. You'd need to fundamentally
change pciehp to (1) not process such events until some currently
undefined criteria is established, and (2) save and export the previously
latched states while pciehp bottom half processing is stalled.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Tue, 2018-08-21 at 17:13 -0600, Keith Busch wrote:
> On Wed, Aug 22, 2018 at 09:06:57AM +1000, Benjamin Herrenschmidt wrote:
> > On Tue, 2018-08-21 at 16:04 -0600, Keith Busch wrote:
> > > > I think there need to be some coordination between pciehb and DPC on
> > > > link state change yes.
> > > > 
> > > > We could still remove the device if recovery fails. For example on EEH
> > > > we have a threshold and if a device fails more than N times within the
> > > > last M minutes (I don't remember the exact numbers and I'm not in front
> > > > of the code right now) we give up.
> > > > 
> > > > Also under some circumstances, the link will change as a result of the
> > > > error handling doing a hot reset.
> > > > 
> > > > For example, if the error happens in a parent bridge and that gets
> > > > reset, the entire hierarchy underneath does too.
> > > > 
> > > > We need to save/restore the state of all bridges and devices (BARs
> > > > etc...) below that.
> > > 
> > > That's not good enough. You'd at least need to check SLTSTS.PDC on all
> > > bridge DSP's beneath the parent *before* you try to restore the state
> > > of devices beneath them. Those races are primarily why DPC currently
> > > removes and reenumerates instead, because that's not something that can
> > > be readily coordinated today.
> > 
> > It can be probably done by a simple test & skip as you go down
> > restoring state, then handling the removals after the dance is
> > complete.
> 
> You can't just test the states in config space. You're racing with
> pciehp's ISR, which clears those states.

Sure, I meant you'd call into pciehp.. For EEH we do have some linkage
between hotplug and EEH, though a lot of it goes at the FW level for
us.

>  You'd need to fundamentally
> change pciehp to (1) not process such events until some currently
> undefined criteria is established, and (2) save and export the previously
> latched states while pciehp bottom half processing is stalled.

Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Keith Busch]

On Wed, Aug 22, 2018 at 09:06:57AM +1000, Benjamin Herrenschmidt wrote:
> It can be probably done by a simple test & skip as you go down
> restoring state, then handling the removals after the dance is
> complete.

I tested on a variety of hardware, and there are mixed results. The spec
captures the crux of the problem with checking PDC (7.5.3.11):

  Note that the in-band presence detect mechanism requires that power be
  applied to an adapter for its presence to be detected. Consequently,
  form factors that require a power controller for hot-plug must implement
  a physical pin presence detect mechanism.

Many slots don't implement power controllers, so a secondary bus reset
always triggers a PDC. We can't really ignore PDC during fatal error
handling since hot plugs are the types of actions that often trigger
fatal errors..

Does it sound okay to trust PDC anyway? It's no worse than what would
happen currently, and it doesn't affect non-hotplug slots.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Sinan Kaya]

On 8/29/2018 8:01 PM, Keith Busch wrote:
> On Wed, Aug 22, 2018 at 09:06:57AM +1000, Benjamin Herrenschmidt wrote:
>> It can be probably done by a simple test & skip as you go down
>> restoring state, then handling the removals after the dance is
>> complete.
> 
> I tested on a variety of hardware, and there are mixed results. The spec
> captures the crux of the problem with checking PDC (7.5.3.11):
> 
>    Note that the in-band presence detect mechanism requires that power be
>    applied to an adapter for its presence to be detected. Consequently,
>    form factors that require a power controller for hot-plug must implement
>    a physical pin presence detect mechanism.
> 
> Many slots don't implement power controllers, so a secondary bus reset
> always triggers a PDC. We can't really ignore PDC during fatal error
> handling since hot plugs are the types of actions that often trigger
> fatal errors..
> 
> Does it sound okay to trust PDC anyway? It's no worse than what would
> happen currently, and it doesn't affect non-hotplug slots.
> 

Why does hotplug operations cause a fatal error? DPC driver is only monitoring
fatal errors today.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Keith Busch]

On Wed, Aug 29, 2018 at 05:10:53PM -0700, Sinan Kaya wrote:
> Why does hotplug operations cause a fatal error? DPC driver is only monitoring
> fatal errors today.

Oh, not considering DPC capabilities; just AER. A hotplug capable
switch only suppresses Suprise Link Down, but its possible other link
or transaction errors occur along with the event.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Wed, 2018-08-29 at 18:01 -0600, Keith Busch wrote:
> On Wed, Aug 22, 2018 at 09:06:57AM +1000, Benjamin Herrenschmidt wrote:
> > It can be probably done by a simple test & skip as you go down
> > restoring state, then handling the removals after the dance is
> > complete.
> 
> I tested on a variety of hardware, and there are mixed results. The spec
> captures the crux of the problem with checking PDC (7.5.3.11):
> 
>   Note that the in-band presence detect mechanism requires that power be
>   applied to an adapter for its presence to be detected. Consequently,
>   form factors that require a power controller for hot-plug must implement
>   a physical pin presence detect mechanism.
> 
> Many slots don't implement power controllers, so a secondary bus reset
> always triggers a PDC. We can't really ignore PDC during fatal error
> handling since hot plugs are the types of actions that often trigger
> fatal errors..
> 
> Does it sound okay to trust PDC anyway? It's no worse than what would
> happen currently, and it doesn't affect non-hotplug slots.

I think so.

As you say, it's not worse and worst case, we try the recovery and
fail, which can then lead to an unplug if we wish to do so. No biggie.

Cheers,
Ben,

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Keith Busch]

On Mon, Aug 20, 2018 at 09:22:27PM +1000, Benjamin Herrenschmidt wrote:
> The main problem with unplug/replug (as I mentioned earlier) is that it
> just does NOT work for storage controllers (or similar type of
> devices). The links between the storage controller and the mounted
> filesystems is lost permanently, you'll most likely have to reboot the
> machine.

You probably shouldn't mount raw storage devices if they can be hot
added/removed. There are device mappers for that! :)

And you can't just change DPC device removal. A DPC event triggers
the link down, and that will trigger pciehp to disconnect the subtree
anyway. Having DPC do it too just means you get the same behavior with
or without enabling STLCTL.DLLSC.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[poza]

On 2018-08-20 21:23, Keith Busch wrote:
> On Mon, Aug 20, 2018 at 09:22:27PM +1000, Benjamin Herrenschmidt wrote:
>> The main problem with unplug/replug (as I mentioned earlier) is that 
>> it
>> just does NOT work for storage controllers (or similar type of
>> devices). The links between the storage controller and the mounted
>> filesystems is lost permanently, you'll most likely have to reboot the
>> machine.
> 
> You probably shouldn't mount raw storage devices if they can be hot
> added/removed. There are device mappers for that! :)
> 
> And you can't just change DPC device removal. A DPC event triggers
> the link down, and that will trigger pciehp to disconnect the subtree
> anyway. Having DPC do it too just means you get the same behavior with
> or without enabling STLCTL.DLLSC.

Hi Keith,

what about the bridges which are not hotplug capable ?

Besides, following patch is trying to ignore link event if there is 
fatal error reporting.
[PATCH v8 1/2] PCI: pciehp: Ignore link events when there is a fatal 
error pending

Regards,
Oza.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Keith Busch]

On Mon, Aug 20, 2018 at 09:43:48PM +0530, poza@codeaurora.org wrote:
> On 2018-08-20 21:23, Keith Busch wrote:
> > On Mon, Aug 20, 2018 at 09:22:27PM +1000, Benjamin Herrenschmidt wrote:
> > > The main problem with unplug/replug (as I mentioned earlier) is that
> > > it
> > > just does NOT work for storage controllers (or similar type of
> > > devices). The links between the storage controller and the mounted
> > > filesystems is lost permanently, you'll most likely have to reboot the
> > > machine.
> > 
> > You probably shouldn't mount raw storage devices if they can be hot
> > added/removed. There are device mappers for that! :)
> > 
> > And you can't just change DPC device removal. A DPC event triggers
> > the link down, and that will trigger pciehp to disconnect the subtree
> > anyway. Having DPC do it too just means you get the same behavior with
> > or without enabling STLCTL.DLLSC.
> 
> Hi Keith,
> 
> what about the bridges which are not hotplug capable ?

That would have to mean SLTCTL.DLSSC isn't enabled. The point was to
make DPC control behave the same regardless of slot control.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Mon, 2018-08-20 at 09:53 -0600, Keith Busch wrote:
> On Mon, Aug 20, 2018 at 09:22:27PM +1000, Benjamin Herrenschmidt wrote:
> > The main problem with unplug/replug (as I mentioned earlier) is that it
> > just does NOT work for storage controllers (or similar type of
> > devices). The links between the storage controller and the mounted
> > filesystems is lost permanently, you'll most likely have to reboot the
> > machine.
> 
> You probably shouldn't mount raw storage devices if they can be hot
> added/removed. There are device mappers for that! :)

This is not about hot adding/removing, it's about error recovery.

> And you can't just change DPC device removal. A DPC event triggers
> the link down, and that will trigger pciehp to disconnect the subtree
> anyway. Having DPC do it too just means you get the same behavior with
> or without enabling STLCTL.DLLSC.

This is wrong. EEH can trigger a link down to and we don't remove the
subtree in that case. We allow the drivers to recover.

Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Sinan Kaya]

On 8/20/2018 5:05 PM, Benjamin Herrenschmidt wrote:
> On Mon, 2018-08-20 at 09:53 -0600, Keith Busch wrote:
>> On Mon, Aug 20, 2018 at 09:22:27PM +1000, Benjamin Herrenschmidt wrote:
>>> The main problem with unplug/replug (as I mentioned earlier) is that it
>>> just does NOT work for storage controllers (or similar type of
>>> devices). The links between the storage controller and the mounted
>>> filesystems is lost permanently, you'll most likely have to reboot the
>>> machine.
>>
>> You probably shouldn't mount raw storage devices if they can be hot
>> added/removed. There are device mappers for that! :)
> 
> This is not about hot adding/removing, it's about error recovery.
> 
>> And you can't just change DPC device removal. A DPC event triggers
>> the link down, and that will trigger pciehp to disconnect the subtree
>> anyway. Having DPC do it too just means you get the same behavior with
>> or without enabling STLCTL.DLLSC.
> 
> This is wrong. EEH can trigger a link down to and we don't remove the
> subtree in that case. We allow the drivers to recover.
> 

I have a patch to solve this issue.

https://lkml.org/lkml/2018/8/19/124

Hotplug driver removes the devices on link down events and re-enumerates
on insertion.

I am trying to separate fatal error handling from hotplug.

> Ben.
> 
> 
> 

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Keith Busch]

On Mon, Aug 20, 2018 at 05:21:30PM -0400, Sinan Kaya wrote:
> On 8/20/2018 5:05 PM, Benjamin Herrenschmidt wrote:
> > On Mon, 2018-08-20 at 09:53 -0600, Keith Busch wrote:
> > > On Mon, Aug 20, 2018 at 09:22:27PM +1000, Benjamin Herrenschmidt wrote:
> > > > The main problem with unplug/replug (as I mentioned earlier) is that it
> > > > just does NOT work for storage controllers (or similar type of
> > > > devices). The links between the storage controller and the mounted
> > > > filesystems is lost permanently, you'll most likely have to reboot the
> > > > machine.
> > > 
> > > You probably shouldn't mount raw storage devices if they can be hot
> > > added/removed. There are device mappers for that! :)
> > 
> > This is not about hot adding/removing, it's about error recovery.
> > 
> > > And you can't just change DPC device removal. A DPC event triggers
> > > the link down, and that will trigger pciehp to disconnect the subtree
> > > anyway. Having DPC do it too just means you get the same behavior with
> > > or without enabling STLCTL.DLLSC.
> > 
> > This is wrong. EEH can trigger a link down to and we don't remove the
> > subtree in that case. We allow the drivers to recover.
> > 
> 
> I have a patch to solve this issue.
> 
> https://lkml.org/lkml/2018/8/19/124
> 
> Hotplug driver removes the devices on link down events and re-enumerates
> on insertion.
> 
> I am trying to separate fatal error handling from hotplug.

I'll try to take a look. We can't always count on pciehp to do the
removal when a removal occurs, though. The PCIe specification contains
an implementation note that DPC may be used in place of hotplug surprise.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Mon, 2018-08-20 at 15:35 -0600, Keith Busch wrote:
> On
> > I have a patch to solve this issue.
> > 
> > https://lkml.org/lkml/2018/8/19/124
> > 
> > Hotplug driver removes the devices on link down events and re-enumerates
> > on insertion.
> > 
> > I am trying to separate fatal error handling from hotplug.
> 
> I'll try to take a look. We can't always count on pciehp to do the
> removal when a removal occurs, though. The PCIe specification contains
> an implementation note that DPC may be used in place of hotplug surprise.

Can't you use the presence detect to differenciate ?

Also, I don't have the specs at hand right now, but does the hotplug
brigde have a way to "latch' the change in presence detect so we can
see if it has transitioned even if it's back on ?

Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Sinan Kaya]

On 8/20/2018 5:53 PM, Benjamin Herrenschmidt wrote:
>>> Hotplug driver removes the devices on link down events and re-enumerates
>>> on insertion.
>>>
>>> I am trying to separate fatal error handling from hotplug.
>> I'll try to take a look. We can't always count on pciehp to do the
>> removal when a removal occurs, though. The PCIe specification contains
>> an implementation note that DPC may be used in place of hotplug surprise.
> Can't you use the presence detect to differenciate ?
> 
> Also, I don't have the specs at hand right now, but does the hotplug
> brigde have a way to "latch' the change in presence detect so we can
> see if it has transitioned even if it's back on ?

There is only presence detect change and link layer change. No actual
state information.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Mon, 2018-08-20 at 18:02 -0400, Sinan Kaya wrote:
> On 8/20/2018 5:53 PM, Benjamin Herrenschmidt wrote:
> > > > Hotplug driver removes the devices on link down events and re-enumerates
> > > > on insertion.
> > > > 
> > > > I am trying to separate fatal error handling from hotplug.
> > > 
> > > I'll try to take a look. We can't always count on pciehp to do the
> > > removal when a removal occurs, though. The PCIe specification contains
> > > an implementation note that DPC may be used in place of hotplug surprise.
> > 
> > Can't you use the presence detect to differenciate ?
> > 
> > Also, I don't have the specs at hand right now, but does the hotplug
> > brigde have a way to "latch' the change in presence detect so we can
> > see if it has transitioned even if it's back on ?
> 
> There is only presence detect change and link layer change. No actual
> state information.

It does latch that it has changed tho right ? So if presence detect
hasn't changed, we can assume it's an error and not an unplug ?

We could discriminate that way to reduce the risk of doing a recovery
without unbind on something that was actually removed and replaced.

Cheers,
Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Sinan Kaya]

On 8/20/2018 6:04 PM, Benjamin Herrenschmidt wrote:
> On Mon, 2018-08-20 at 18:02 -0400, Sinan Kaya wrote:
>> On 8/20/2018 5:53 PM, Benjamin Herrenschmidt wrote:
>>>>> Hotplug driver removes the devices on link down events and re-enumerates
>>>>> on insertion.
>>>>>
>>>>> I am trying to separate fatal error handling from hotplug.
>>>>
>>>> I'll try to take a look. We can't always count on pciehp to do the
>>>> removal when a removal occurs, though. The PCIe specification contains
>>>> an implementation note that DPC may be used in place of hotplug surprise.
>>>
>>> Can't you use the presence detect to differenciate ?
>>>
>>> Also, I don't have the specs at hand right now, but does the hotplug
>>> brigde have a way to "latch' the change in presence detect so we can
>>> see if it has transitioned even if it's back on ?
>>
>> There is only presence detect change and link layer change. No actual
>> state information.
> 
> It does latch that it has changed tho right ? So if presence detect
> hasn't changed, we can assume it's an error and not an unplug ?
> 
> We could discriminate that way to reduce the risk of doing a recovery
> without unbind on something that was actually removed and replaced.
> 

I proposed this as one of the possible solutions but presence detect is
optional and also presence detect interrupt can be delivered after
link layer interrupt as well. No guarantees with respect to the order of
link layer interrupt and presence detect interrupts delivery.

I instead look at fatal error pending bit in device status register to
decide if the link down was initiated due to a pending fatal error or
somebody actually removed the card.

If fatal error is pending, wait until it is cleared. If link is healthy,
return gracefully.

Otherwise, proceed with the hotplug removal.


> Cheers,
> Ben.
> 
> 
> 

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Mon, 2018-08-20 at 18:13 -0400, Sinan Kaya wrote:
> 
> I proposed this as one of the possible solutions but presence detect is
> optional 

Is it optional on hotplug capable bridges ? I thought it wasn't ...

> and also presence detect interrupt can be delivered after

We don't care about the interrupt, we just need to look at the value of
the change latch.

> link layer interrupt as well. No guarantees with respect to the order of
> link layer interrupt and presence detect interrupts delivery.
> 
> I instead look at fatal error pending bit in device status register to
> decide if the link down was initiated due to a pending fatal error or
> somebody actually removed the card.
> 
> If fatal error is pending, wait until it is cleared. If link is healthy,
> return gracefully.
> 
> Otherwise, proceed with the hotplug removal.

That could work too but it would be safer to also use the change in
presence detect if available.

Cheers,
Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Lukas Wunner]

On Mon, Aug 20, 2018 at 06:13:03PM -0400, Sinan Kaya wrote:
> presence detect is optional

Hm, how so?  Do you have a spec reference for that?

Thanks,

Lukas

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Keith Busch]

On Wed, Aug 22, 2018 at 11:13:33AM +0200, Lukas Wunner wrote:
> On Mon, Aug 20, 2018 at 06:13:03PM -0400, Sinan Kaya wrote:
> > presence detect is optional
> 
> Hm, how so?  Do you have a spec reference for that?

Yeah, presence detect state and change are required if the slot supports
hot plug. If the slot doesn't support these, error handling becomes a
lot simpler.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Sinan Kaya]

On 8/22/2018 10:38 AM, Keith Busch wrote:
> On Wed, Aug 22, 2018 at 11:13:33AM +0200, Lukas Wunner wrote:
>> On Mon, Aug 20, 2018 at 06:13:03PM -0400, Sinan Kaya wrote:
>>> presence detect is optional
>>
>> Hm, how so?  Do you have a spec reference for that?
> 
> Yeah, presence detect state and change are required if the slot supports
> hot plug. If the slot doesn't support these, error handling becomes a
> lot simpler.
> 

I double checked the slot capabilities register. You are right, it is
required.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Keith Busch]

On Tue, Aug 21, 2018 at 08:04:46AM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2018-08-20 at 18:02 -0400, Sinan Kaya wrote:
> > On 8/20/2018 5:53 PM, Benjamin Herrenschmidt wrote:
> > > > > Hotplug driver removes the devices on link down events and re-enumerates
> > > > > on insertion.
> > > > > 
> > > > > I am trying to separate fatal error handling from hotplug.
> > > > 
> > > > I'll try to take a look. We can't always count on pciehp to do the
> > > > removal when a removal occurs, though. The PCIe specification contains
> > > > an implementation note that DPC may be used in place of hotplug surprise.
> > > 
> > > Can't you use the presence detect to differenciate ?
> > > 
> > > Also, I don't have the specs at hand right now, but does the hotplug
> > > brigde have a way to "latch' the change in presence detect so we can
> > > see if it has transitioned even if it's back on ?
> > 
> > There is only presence detect change and link layer change. No actual
> > state information.
> 
> It does latch that it has changed tho right ? So if presence detect
> hasn't changed, we can assume it's an error and not an unplug ?
> 
> We could discriminate that way to reduce the risk of doing a recovery
> without unbind on something that was actually removed and replaced.

There might be some potential here.

There actualy is a Slot Status Presence Detect State (PDS), but it isn't
latched. The link and presence change (PDC) are latched.

A potential problem may be a PDC event handling observes PDS set to the
last known state if the remove + add happens fast enough, and then no
action is taken in the current implementation.

The DPC capability learned from such issues, and they will latch the
status until acknowledged so those sorts of races are not possible.

But anyway, maybe we can change pciehp to ignore PDS and always
re-enumerate when PDC is observed, and that may very well simplify a lot
of this.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Mon, 2018-08-20 at 16:13 -0600, Keith Busch wrote:
> 
> There might be some potential here.
> 
> There actualy is a Slot Status Presence Detect State (PDS), but it isn't
> latched. The link and presence change (PDC) are latched.
> 
> A potential problem may be a PDC event handling observes PDS set to the
> last known state if the remove + add happens fast enough, and then no
> action is taken in the current implementation.
> 
> The DPC capability learned from such issues, and they will latch the
> status until acknowledged so those sorts of races are not possible.
> 
> But anyway, maybe we can change pciehp to ignore PDS and always
> re-enumerate when PDC is observed, and that may very well simplify a lot
> of this.

That sounds reasonable to me.

Cheers,
Ben.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Keith Busch]

On Tue, Aug 21, 2018 at 08:19:52AM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2018-08-20 at 16:13 -0600, Keith Busch wrote:
> > But anyway, maybe we can change pciehp to ignore PDS and always
> > re-enumerate when PDC is observed, and that may very well simplify a lot
> > of this.
> 
> That sounds reasonable to me.

It seems 4.19 is way ahead of me! The latest pciehp driver looks really
good. This'll be more straight forward to separate errors from enumeration
than I thought, and Sinan's patch earlier is probably just fine in that
direction. Will look again tomorrow.

Re: [PATCH 1/1] PCI/AER: prevent pcie_do_fatal_recovery from using device after it is removed

[Benjamin Herrenschmidt]

On Sat, 2018-08-18 at 21:19 -0500, Bjorn Helgaas wrote:
> 
> > Ugh... I just saw the changes you did to Documentation/PCI/pci-error-
> > recovery.txt and I would very much like to revert those !
> > 
> > Bjorn, you shouldn't let changes to the PCI error handling through
> > without acks from us, it looks like we didn't notice (we can't possibly
> > monitor all lists).
> 
> Sorry, they were certainly very visible on linux-pci, but I should
> have added you to cc if you weren't there already.  Please update
> MAINTAINERS if it's incomplete or out of date (I can't possibly know
> who might be interested in every change).

Right but we designed the error handling (we = powerpc). Due to churn
with people I think nobody's been actively monitoring linux-pci and I
was myself caught up with a bunch of other things.

> > Bjorn, please revert all of those changes.
> 
> Please send the appropriate patches and we'll go from there.

PATCH] Partial revert of "PCI/AER: Handle ERR_FATAL with removal and re-enumeration of devices"

[Benjamin Herrenschmidt]

This partially reverts commit 7e9084b36740b2ec263ea35efb203001f755e1d8.

This only reverts the Documentation/PCI/pci-error-recovery.txt changes

Those changes are incorrect, they change the documentation to adapt
to the (imho incorrect) AER implementation, and as a result making
it no longer match the EEH implementation.

I believe the policy described originally in this document is what
should be implemented by everybody and the changes done by that commit
would compromise, among others, the ability to recover from errors with
storage devices.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 Documentation/PCI/pci-error-recovery.txt | 35 +++++++-----------------
 1 file changed, 10 insertions(+), 25 deletions(-)

diff --git a/Documentation/PCI/pci-error-recovery.txt b/Documentation/PCI/pci-error-recovery.txt
index 688b69121e82..0b6bb3ef449e 100644
--- a/Documentation/PCI/pci-error-recovery.txt
+++ b/Documentation/PCI/pci-error-recovery.txt
@@ -110,7 +110,7 @@ The actual steps taken by a platform to recover from a PCI error
 event will be platform-dependent, but will follow the general
 sequence described below.
 
-STEP 0: Error Event: ERR_NONFATAL
+STEP 0: Error Event
 -------------------
 A PCI bus error is detected by the PCI hardware.  On powerpc, the slot
 is isolated, in that all I/O is blocked: all reads return 0xffffffff,
@@ -228,7 +228,13 @@ proceeds to either STEP3 (Link Reset) or to STEP 5 (Resume Operations).
 If any driver returned PCI_ERS_RESULT_NEED_RESET, then the platform
 proceeds to STEP 4 (Slot Reset)
 
-STEP 3: Slot Reset
+STEP 3: Link Reset
+------------------
+The platform resets the link.  This is a PCI-Express specific step
+and is done whenever a fatal error has been detected that can be
+"solved" by resetting the link.
+
+STEP 4: Slot Reset
 ------------------
 
 In response to a return value of PCI_ERS_RESULT_NEED_RESET, the
@@ -314,7 +320,7 @@ Failure).
 >>> However, it probably should.
 
 
-STEP 4: Resume Operations
+STEP 5: Resume Operations
 -------------------------
 The platform will call the resume() callback on all affected device
 drivers if all drivers on the segment have returned
@@ -326,7 +332,7 @@ a result code.
 At this point, if a new error happens, the platform will restart
 a new error recovery sequence.
 
-STEP 5: Permanent Failure
+STEP 6: Permanent Failure
 -------------------------
 A "permanent failure" has occurred, and the platform cannot recover
 the device.  The platform will call error_detected() with a
@@ -349,27 +355,6 @@ errors. See the discussion in powerpc/eeh-pci-error-recovery.txt
 for additional detail on real-life experience of the causes of
 software errors.
 
-STEP 0: Error Event: ERR_FATAL
--------------------
-PCI bus error is detected by the PCI hardware. On powerpc, the slot is
-isolated, in that all I/O is blocked: all reads return 0xffffffff, all
-writes are ignored.
-
-STEP 1: Remove devices
---------------------
-Platform removes the devices depending on the error agent, it could be
-this port for all subordinates or upstream component (likely downstream
-port)
-
-STEP 2: Reset link
---------------------
-The platform resets the link.  This is a PCI-Express specific step and is
-done whenever a fatal error has been detected that can be "solved" by
-resetting the link.
-
-STEP 3: Re-enumerate the devices
---------------------
-Initiates the re-enumeration.
 
 Conclusion; General Remarks
 ---------------------------

Re: PATCH] Partial revert of "PCI/AER: Handle ERR_FATAL with removal and re-enumeration of devices"

[Bjorn Helgaas]

On Mon, Aug 20, 2018 at 02:39:04PM +1000, Benjamin Herrenschmidt wrote:
> This partially reverts commit 7e9084b36740b2ec263ea35efb203001f755e1d8.
> 
> This only reverts the Documentation/PCI/pci-error-recovery.txt changes
> 
> Those changes are incorrect, they change the documentation to adapt
> to the (imho incorrect) AER implementation, and as a result making
> it no longer match the EEH implementation.
> 
> I believe the policy described originally in this document is what
> should be implemented by everybody and the changes done by that commit
> would compromise, among others, the ability to recover from errors with
> storage devices.

I think we should align EEH, AER, and DPC as much as possible,
including making this documentation match the code.

Because of its name, this file *looks* like it should match the code
in the PCI core, i.e., drivers/pci/...  So I think it would be
confusing to simply apply this revert without making a more direct
connection between this documentation and the powerpc-specific EEH
code.

If we can change AER & DPC to correspond to EEH, then I think it would
make sense to apply this revert along with those AER & DPC changes so
the documentation stays in step with the code.

> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  Documentation/PCI/pci-error-recovery.txt | 35 +++++++-----------------
>  1 file changed, 10 insertions(+), 25 deletions(-)
> 
> diff --git a/Documentation/PCI/pci-error-recovery.txt b/Documentation/PCI/pci-error-recovery.txt
> index 688b69121e82..0b6bb3ef449e 100644
> --- a/Documentation/PCI/pci-error-recovery.txt
> +++ b/Documentation/PCI/pci-error-recovery.txt
> @@ -110,7 +110,7 @@ The actual steps taken by a platform to recover from a PCI error
>  event will be platform-dependent, but will follow the general
>  sequence described below.
>  
> -STEP 0: Error Event: ERR_NONFATAL
> +STEP 0: Error Event
>  -------------------
>  A PCI bus error is detected by the PCI hardware.  On powerpc, the slot
>  is isolated, in that all I/O is blocked: all reads return 0xffffffff,
> @@ -228,7 +228,13 @@ proceeds to either STEP3 (Link Reset) or to STEP 5 (Resume Operations).
>  If any driver returned PCI_ERS_RESULT_NEED_RESET, then the platform
>  proceeds to STEP 4 (Slot Reset)
>  
> -STEP 3: Slot Reset
> +STEP 3: Link Reset
> +------------------
> +The platform resets the link.  This is a PCI-Express specific step
> +and is done whenever a fatal error has been detected that can be
> +"solved" by resetting the link.
> +
> +STEP 4: Slot Reset
>  ------------------
>  
>  In response to a return value of PCI_ERS_RESULT_NEED_RESET, the
> @@ -314,7 +320,7 @@ Failure).
>  >>> However, it probably should.
>  
>  
> -STEP 4: Resume Operations
> +STEP 5: Resume Operations
>  -------------------------
>  The platform will call the resume() callback on all affected device
>  drivers if all drivers on the segment have returned
> @@ -326,7 +332,7 @@ a result code.
>  At this point, if a new error happens, the platform will restart
>  a new error recovery sequence.
>  
> -STEP 5: Permanent Failure
> +STEP 6: Permanent Failure
>  -------------------------
>  A "permanent failure" has occurred, and the platform cannot recover
>  the device.  The platform will call error_detected() with a
> @@ -349,27 +355,6 @@ errors. See the discussion in powerpc/eeh-pci-error-recovery.txt
>  for additional detail on real-life experience of the causes of
>  software errors.
>  
> -STEP 0: Error Event: ERR_FATAL
> --------------------
> -PCI bus error is detected by the PCI hardware. On powerpc, the slot is
> -isolated, in that all I/O is blocked: all reads return 0xffffffff, all
> -writes are ignored.
> -
> -STEP 1: Remove devices
> ---------------------
> -Platform removes the devices depending on the error agent, it could be
> -this port for all subordinates or upstream component (likely downstream
> -port)
> -
> -STEP 2: Reset link
> ---------------------
> -The platform resets the link.  This is a PCI-Express specific step and is
> -done whenever a fatal error has been detected that can be "solved" by
> -resetting the link.
> -
> -STEP 3: Re-enumerate the devices
> ---------------------
> -Initiates the re-enumeration.
>  
>  Conclusion; General Remarks
>  ---------------------------
> 
> 

Re: PATCH] Partial revert of "PCI/AER: Handle ERR_FATAL with removal and re-enumeration of devices"

[poza]

On 2018-08-22 01:20, Bjorn Helgaas wrote:
> On Mon, Aug 20, 2018 at 02:39:04PM +1000, Benjamin Herrenschmidt wrote:
>> This partially reverts commit 
>> 7e9084b36740b2ec263ea35efb203001f755e1d8.
>> 
>> This only reverts the Documentation/PCI/pci-error-recovery.txt changes
>> 
>> Those changes are incorrect, they change the documentation to adapt
>> to the (imho incorrect) AER implementation, and as a result making
>> it no longer match the EEH implementation.
>> 
>> I believe the policy described originally in this document is what
>> should be implemented by everybody and the changes done by that commit
>> would compromise, among others, the ability to recover from errors 
>> with
>> storage devices.
> 
> I think we should align EEH, AER, and DPC as much as possible,
> including making this documentation match the code.
> 
> Because of its name, this file *looks* like it should match the code
> in the PCI core, i.e., drivers/pci/...  So I think it would be
> confusing to simply apply this revert without making a more direct
> connection between this documentation and the powerpc-specific EEH
> code.
> 
> If we can change AER & DPC to correspond to EEH, then I think it would
> make sense to apply this revert along with those AER & DPC changes so
> the documentation stays in step with the code.

I agree with you Bjorn, but it looks like we are far from holding some 
consensus on the behavior.
the other mail-chain [PCI/AER: prevent pcie_do_fatal_recovery from using 
device after it is removed]
has gone long enough, and touched lot of things all together. hence I 
would like to just use this mail-chain just to discuss
about AER, DPC behaviour.

let me put forward the proposal here to this mail-chain.

1) Right now AER and DPC both calls pcie_do_fatal_recovery(), I majorly 
see DPC as error handling and recovery agent rather than being used for 
hotplug.
    so in my opinion, both AER and DPC should have same error handling 
and recovery mechanism

    so if there is a way to figure out that in absence of pcihp, if DPC 
is being used to support hotplug then we fall back to original DPC 
mechanism (which
    is remove devices)
    otherwise, we fall back to drivers callbacks.

    Spec 6.7.5 Async Removal
    "
    The Surprise Down error resulting from async removal may trigger 
Downstream Port Containment (See Section 6.2.10).
    Downstream Port Containment following an async removal may be 
utilized to hold the Link of a
    Downstream Port in the Disabled LTSSM state while host software 
recovers from the side effects of an async removal.
    "
    pcie_do_fatal_recovery should take care of above. so that we support 
both error handling and async removal from DPC point of view.


2) pci_channel_io_frozen is the one which should be used from framework 
to communicate driver that this is ERR_FATAL, the it is left to the 
driver(s) to see if they want reset of the link (SBR).

3) pcie_do_nonfatal_recovery; we sould actually reset the link e.g. SBR 
if driver requests the reset of link.  (there is already TDO note 
anyway).
    if (status == PCI_ERS_RESULT_NEED_RESET) {
         /*
          * TODO: Should call platform-specific
          * functions to reset slot before calling
          * drivers' slot_reset callbacks?
          */
         status = broadcast_error_message(dev,
                 state,
                 "slot_reset",
                 report_slot_reset);
     }


Regards,
Oza.


> 
>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> ---
>>  Documentation/PCI/pci-error-recovery.txt | 35 
>> +++++++-----------------
>>  1 file changed, 10 insertions(+), 25 deletions(-)
>> 
>> diff --git a/Documentation/PCI/pci-error-recovery.txt 
>> b/Documentation/PCI/pci-error-recovery.txt
>> index 688b69121e82..0b6bb3ef449e 100644
>> --- a/Documentation/PCI/pci-error-recovery.txt
>> +++ b/Documentation/PCI/pci-error-recovery.txt
>> @@ -110,7 +110,7 @@ The actual steps taken by a platform to recover 
>> from a PCI error
>>  event will be platform-dependent, but will follow the general
>>  sequence described below.
>> 
>> -STEP 0: Error Event: ERR_NONFATAL
>> +STEP 0: Error Event
>>  -------------------
>>  A PCI bus error is detected by the PCI hardware.  On powerpc, the 
>> slot
>>  is isolated, in that all I/O is blocked: all reads return 0xffffffff,
>> @@ -228,7 +228,13 @@ proceeds to either STEP3 (Link Reset) or to STEP 
>> 5 (Resume Operations).
>>  If any driver returned PCI_ERS_RESULT_NEED_RESET, then the platform
>>  proceeds to STEP 4 (Slot Reset)
>> 
>> -STEP 3: Slot Reset
>> +STEP 3: Link Reset
>> +------------------
>> +The platform resets the link.  This is a PCI-Express specific step
>> +and is done whenever a fatal error has been detected that can be
>> +"solved" by resetting the link.
>> +
>> +STEP 4: Slot Reset
>>  ------------------
>> 
>>  In response to a return value of PCI_ERS_RESULT_NEED_RESET, the
>> @@ -314,7 +320,7 @@ Failure).
>>  >>> However, it probably should.
>> 
>> 
>> -STEP 4: Resume Operations
>> +STEP 5: Resume Operations
>>  -------------------------
>>  The platform will call the resume() callback on all affected device
>>  drivers if all drivers on the segment have returned
>> @@ -326,7 +332,7 @@ a result code.
>>  At this point, if a new error happens, the platform will restart
>>  a new error recovery sequence.
>> 
>> -STEP 5: Permanent Failure
>> +STEP 6: Permanent Failure
>>  -------------------------
>>  A "permanent failure" has occurred, and the platform cannot recover
>>  the device.  The platform will call error_detected() with a
>> @@ -349,27 +355,6 @@ errors. See the discussion in 
>> powerpc/eeh-pci-error-recovery.txt
>>  for additional detail on real-life experience of the causes of
>>  software errors.
>> 
>> -STEP 0: Error Event: ERR_FATAL
>> --------------------
>> -PCI bus error is detected by the PCI hardware. On powerpc, the slot 
>> is
>> -isolated, in that all I/O is blocked: all reads return 0xffffffff, 
>> all
>> -writes are ignored.
>> -
>> -STEP 1: Remove devices
>> ---------------------
>> -Platform removes the devices depending on the error agent, it could 
>> be
>> -this port for all subordinates or upstream component (likely 
>> downstream
>> -port)
>> -
>> -STEP 2: Reset link
>> ---------------------
>> -The platform resets the link.  This is a PCI-Express specific step 
>> and is
>> -done whenever a fatal error has been detected that can be "solved" by
>> -resetting the link.
>> -
>> -STEP 3: Re-enumerate the devices
>> ---------------------
>> -Initiates the re-enumeration.
>> 
>>  Conclusion; General Remarks
>>  ---------------------------
>> 
>>