[PATCH] PCI/AER: Do not clear AER bits if we don't own AER

[PATCH] PCI/AER: Do not clear AER bits if we don't own AER

[Alexandru Gagniuc]

When we don't own AER, we shouldn't touch the AER error bits. This
happens unconditionally on device probe(). Clearing AER bits
willy-nilly might cause firmware to miss errors. Instead
these bits should get cleared by FFS, or via ACPI _HPX method.

This race is mostly of theoretical significance, as it is not easy to
reasonably demonstrate it in testing.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 drivers/pci/pcie/aer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index a2e88386af28..18037a2a8231 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -383,6 +383,9 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
 	if (!pci_is_pcie(dev))
 		return -ENODEV;
 
+	if (pcie_aer_get_firmware_first(dev))
+		return -EIO;
+
 	pos = dev->aer_cap;
 	if (!pos)
 		return -EIO;
-- 
2.14.3

Re: [PATCH] PCI/AER: Do not clear AER bits if we don't own AER

[Sinan Kaya]

On 7/17/2018 8:31 AM, Alexandru Gagniuc wrote:
> +	if (pcie_aer_get_firmware_first(dev))
> +		return -EIO;

Can you move this to closer to the caller pci_aer_init()?

Re: [PATCH] PCI/AER: Do not clear AER bits if we don't own AER

[Alex G.]

On 07/17/2018 10:41 AM, Sinan Kaya wrote:
> 
> On 7/17/2018 8:31 AM, Alexandru Gagniuc wrote:
>> +    if (pcie_aer_get_firmware_first(dev))
>> +        return -EIO;
> 
> Can you move this to closer to the caller pci_aer_init()?

I could move it there. although pci_cleanup_aer_error_status_regs() is 
called directly from pci_restore_state.  Of course, aer_cap should be 
zero in this case, and we'd still bail out.
I find the intent clearer if we check it here rather than having to do 
the mental parsing of the state of aer_cap.

Alex

Re: [PATCH] PCI/AER: Do not clear AER bits if we don't own AER

[Sinan Kaya]

On 7/19/2018 8:55 AM, Alex G. wrote:
> I find the intent clearer if we check it here rather than having to do 
> the mental parsing of the state of aer_cap.

I don't feel too strong about my comment to be honest. This was a
style/maintenance comment.

It feels like we are putting pcie_aer_get_firmware_first() into core
functions unnecessarily after your change. I understand the need for
your change. I'm asking if it is the right place or not.

pcie_aer_get_firmware_first() should be called from either the init or
probe function so that the rest of the AER functions do not get called
from any other context.

If someone adds another AER function, we might need to add another
pcie_aer_get_firmware_first() check there. So, we have unnecessary code
duplication.

Re: [PATCH] PCI/AER: Do not clear AER bits if we don't own AER

[Alex G.]

On 07/19/2018 11:58 AM, Sinan Kaya wrote:
> 
> On 7/19/2018 8:55 AM, Alex G. wrote:
>> I find the intent clearer if we check it here rather than having to do 
>> the mental parsing of the state of aer_cap.
> 
> I don't feel too strong about my comment to be honest. This was a
> style/maintenance comment.
> 
> It feels like we are putting pcie_aer_get_firmware_first() into core
> functions unnecessarily after your change. I understand the need for
> your change. I'm asking if it is the right place or not.
> 
> pcie_aer_get_firmware_first() should be called from either the init or
> probe function so that the rest of the AER functions do not get called
> from any other context.
> 
> If someone adds another AER function, we might need to add another
> pcie_aer_get_firmware_first() check there. So, we have unnecessary code
> duplication.

We could move the aer_cap and get_ffs() check into one function that we 
end up calling all over the place. I understand your concern about code 
duplication, and I agree with it. I don't think that at this point it's 
that big of a deal, although we might need to guard every aer_() call.

So moving all the checks in a pcie_aer_is_kernel_first() makes sense.

Alex

[PATCH v2] PCI/AER: Do not clear AER bits if we don't own AER

[Alexandru Gagniuc]

When we don't own AER, we shouldn't touch the AER error bits. Clearing
error bits willy-nilly might cause firmware to miss some errors. In
theory, these bits get cleared by FFS, or via ACPI _HPX method. These
mechanisms are not subject to the problem.

This race is mostly of theoretical significance, since I can't
reasonably demonstrate this race in the lab.

On a side-note, pcie_aer_is_kernel_first() is created to alleviate the
need for two checks: aer_cap and get_firmware_first().

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 drivers/pci/pcie/aer.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index a2e88386af28..85c3e173c025 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -307,6 +307,12 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev)
 		aer_set_firmware_first(dev);
 	return dev->__aer_firmware_first;
 }
+
+static bool pcie_aer_is_kernel_first(struct pci_dev *dev)
+{
+	return !!dev->aer_cap && !pcie_aer_get_firmware_first(dev);
+}
+
 #define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
 				 PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)
 
@@ -337,10 +343,7 @@ bool aer_acpi_firmware_first(void)
 
 int pci_enable_pcie_error_reporting(struct pci_dev *dev)
 {
-	if (pcie_aer_get_firmware_first(dev))
-		return -EIO;
-
-	if (!dev->aer_cap)
+	if (!pcie_aer_is_kernel_first(dev))
 		return -EIO;
 
 	return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_AER_FLAGS);
@@ -349,7 +352,7 @@ EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting);
 
 int pci_disable_pcie_error_reporting(struct pci_dev *dev)
 {
-	if (pcie_aer_get_firmware_first(dev))
+	if (!pcie_aer_is_kernel_first(dev))
 		return -EIO;
 
 	return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL,
@@ -383,10 +386,10 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
 	if (!pci_is_pcie(dev))
 		return -ENODEV;
 
-	pos = dev->aer_cap;
-	if (!pos)
+	if (pcie_aer_is_kernel_first(dev))
 		return -EIO;
 
+	pos = dev->aer_cap;
 	port_type = pci_pcie_type(dev);
 	if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
 		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);
-- 
2.17.1

Re: [PATCH v2] PCI/AER: Do not clear AER bits if we don't own AER

[Alex G.]

On 07/23/2018 11:52 AM, Alexandru Gagniuc wrote:
> When we don't own AER, we shouldn't touch the AER error bits. Clearing
> error bits willy-nilly might cause firmware to miss some errors. In
> theory, these bits get cleared by FFS, or via ACPI _HPX method. These
> mechanisms are not subject to the problem.
> 
> This race is mostly of theoretical significance, since I can't
> reasonably demonstrate this race in the lab.
> 
> On a side-note, pcie_aer_is_kernel_first() is created to alleviate the
> need for two checks: aer_cap and get_firmware_first().
> 
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>   drivers/pci/pcie/aer.c | 17 ++++++++++-------
>   1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index a2e88386af28..85c3e173c025 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -307,6 +307,12 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev)
>   		aer_set_firmware_first(dev);
>   	return dev->__aer_firmware_first;
>   }
> +
> +static bool pcie_aer_is_kernel_first(struct pci_dev *dev)
> +{
> +	return !!dev->aer_cap && !pcie_aer_get_firmware_first(dev);
> +}
> +
>   #define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
>   				 PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)
>   
> @@ -337,10 +343,7 @@ bool aer_acpi_firmware_first(void)
>   
>   int pci_enable_pcie_error_reporting(struct pci_dev *dev)
>   {
> -	if (pcie_aer_get_firmware_first(dev))
> -		return -EIO;
> -
> -	if (!dev->aer_cap)
> +	if (!pcie_aer_is_kernel_first(dev))
>   		return -EIO;
>   
>   	return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_AER_FLAGS);
> @@ -349,7 +352,7 @@ EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting);
>   
>   int pci_disable_pcie_error_reporting(struct pci_dev *dev)
>   {
> -	if (pcie_aer_get_firmware_first(dev))
> +	if (!pcie_aer_is_kernel_first(dev))
>   		return -EIO;
>   
>   	return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL,
> @@ -383,10 +386,10 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
>   	if (!pci_is_pcie(dev))
>   		return -ENODEV;
>   
> -	pos = dev->aer_cap;
> -	if (!pos)
> +	if (pcie_aer_is_kernel_first(dev))

This here is missing a '!'. It's in my local branch, so I must have 
exported the patch before I fixed that. I'll get that fixed next rev.

>   		return -EIO;
>   
> +	pos = dev->aer_cap;
>   	port_type = pci_pcie_type(dev);
>   	if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
>   		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);
> 

[PATCH v3] PCI/AER: Do not clear AER bits if we don't own AER

[Alexandru Gagniuc]

When we don't own AER, we shouldn't touch the AER error bits. Clearing
error bits willy-nilly might cause firmware to miss some errors. In
theory, these bits get cleared by FFS, or via ACPI _HPX method. These
mechanisms are not subject to the problem.

This race is mostly of theoretical significance, since I can't
reasonably demonstrate this race in the lab.

On a side-note, pcie_aer_is_kernel_first() is created to alleviate the
need for two checks: aer_cap and get_firmware_first().

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---

Changes since v2:
  - Added missing negation in pci_cleanup_aer_error_status_regs()

 drivers/pci/pcie/aer.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index a2e88386af28..40e5c86271d1 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -307,6 +307,12 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev)
 		aer_set_firmware_first(dev);
 	return dev->__aer_firmware_first;
 }
+
+static bool pcie_aer_is_kernel_first(struct pci_dev *dev)
+{
+	return !!dev->aer_cap && !pcie_aer_get_firmware_first(dev);
+}
+
 #define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
 				 PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)
 
@@ -337,10 +343,7 @@ bool aer_acpi_firmware_first(void)
 
 int pci_enable_pcie_error_reporting(struct pci_dev *dev)
 {
-	if (pcie_aer_get_firmware_first(dev))
-		return -EIO;
-
-	if (!dev->aer_cap)
+	if (!pcie_aer_is_kernel_first(dev))
 		return -EIO;
 
 	return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_AER_FLAGS);
@@ -349,7 +352,7 @@ EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting);
 
 int pci_disable_pcie_error_reporting(struct pci_dev *dev)
 {
-	if (pcie_aer_get_firmware_first(dev))
+	if (!pcie_aer_is_kernel_first(dev))
 		return -EIO;
 
 	return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL,
@@ -383,10 +386,10 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
 	if (!pci_is_pcie(dev))
 		return -ENODEV;
 
-	pos = dev->aer_cap;
-	if (!pos)
+	if (!pcie_aer_is_kernel_first(dev))
 		return -EIO;
 
+	pos = dev->aer_cap;
 	port_type = pci_pcie_type(dev);
 	if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
 		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);
-- 
2.17.1

Re: [PATCH v3] PCI/AER: Do not clear AER bits if we don't own AER

[Bjorn Helgaas]

On Mon, Jul 30, 2018 at 06:35:31PM -0500, Alexandru Gagniuc wrote:
> When we don't own AER, we shouldn't touch the AER error bits. Clearing
> error bits willy-nilly might cause firmware to miss some errors. In
> theory, these bits get cleared by FFS, or via ACPI _HPX method. These
> mechanisms are not subject to the problem.

What's FFS?

I guess you mean FFS and _HPX are not subject to the problem because
they're supplied by firmware, so firmware would be responsible for
looking at the bits before clearing them?

> This race is mostly of theoretical significance, since I can't
> reasonably demonstrate this race in the lab.
> 
> On a side-note, pcie_aer_is_kernel_first() is created to alleviate the
> need for two checks: aer_cap and get_firmware_first().
> 
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
> 
> Changes since v2:
>   - Added missing negation in pci_cleanup_aer_error_status_regs()
> 
>  drivers/pci/pcie/aer.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index a2e88386af28..40e5c86271d1 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -307,6 +307,12 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev)
>  		aer_set_firmware_first(dev);
>  	return dev->__aer_firmware_first;
>  }
> +
> +static bool pcie_aer_is_kernel_first(struct pci_dev *dev)
> +{
> +	return !!dev->aer_cap && !pcie_aer_get_firmware_first(dev);
> +}

I think it complicates things to have both "firmware_first" and
"kernel_first" interfaces, so I would prefer to stick with the
existing "firmware_first" style.

>  #define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
>  				 PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)
>  
> @@ -337,10 +343,7 @@ bool aer_acpi_firmware_first(void)
>  
>  int pci_enable_pcie_error_reporting(struct pci_dev *dev)
>  {
> -	if (pcie_aer_get_firmware_first(dev))
> -		return -EIO;
> -
> -	if (!dev->aer_cap)
> +	if (!pcie_aer_is_kernel_first(dev))
>  		return -EIO;
>  
>  	return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_AER_FLAGS);

This change doesn't actually fix anything, does it?  It looks like a
cleanup that doesn't change the behavior.

> @@ -349,7 +352,7 @@ EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting);
>  
>  int pci_disable_pcie_error_reporting(struct pci_dev *dev)
>  {
> -	if (pcie_aer_get_firmware_first(dev))
> +	if (!pcie_aer_is_kernel_first(dev))
>  		return -EIO;

This change does effectively add a test for dev->aer_cap.  That makes
sense in terms of symmetry with pci_enable_pcie_error_reporting(),
but I think it should be a separate patch because it's conceptually
separate from the change below.

We should keep the existing behavior (but add the symmetry) here for
now, but it's not clear to me that these paths should care about AER
or firmware-first at all.  PCI_EXP_DEVCTL is not an AER register and
we have the _HPX mechanism for firmware to influence it (which these
paths currently ignore).  I suspect we should program these reporting
enable bits in the core enumeration path instead of having drivers
call these interfaces.

If/when we make changes along these lines, the history will be easier
to follow if *this* change is not connected with the change below to
pci_cleanup_aer_error_status_regs().

>  	return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL,
> @@ -383,10 +386,10 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
>  	if (!pci_is_pcie(dev))
>  		return -ENODEV;
>  
> -	pos = dev->aer_cap;
> -	if (!pos)
> +	if (!pcie_aer_is_kernel_first(dev))
>  		return -EIO;

This part makes sense to me, but I think I would rather have it match
the existing style in pci_enable_pcie_error_reporting(), i.e., keep
the test for dev->aer_cap and add a test for
pcie_aer_get_firmware_first().

> +	pos = dev->aer_cap;
>  	port_type = pci_pcie_type(dev);
>  	if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
>  		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);
> -- 
> 2.17.1
> 

Re: [PATCH v3] PCI/AER: Do not clear AER bits if we don't own AER

[Alex G.]

On 08/07/2018 08:14 PM, Bjorn Helgaas wrote:
> On Mon, Jul 30, 2018 at 06:35:31PM -0500, Alexandru Gagniuc wrote:
>> When we don't own AER, we shouldn't touch the AER error bits. Clearing
>> error bits willy-nilly might cause firmware to miss some errors. In
>> theory, these bits get cleared by FFS, or via ACPI _HPX method. These
>> mechanisms are not subject to the problem.
> 
> What's FFS?

Firmware-first. Nobody likes spelling it out, and all other proposed 
acronyms are insanely tong-twisting. So, FFS.

> I guess you mean FFS and _HPX are not subject to the problem because
> they're supplied by firmware, so firmware would be responsible for
> looking at the bits before clearing them?

Exactly.

>> This race is mostly of theoretical significance, since I can't
>> reasonably demonstrate this race in the lab.
>>
>> On a side-note, pcie_aer_is_kernel_first() is created to alleviate the
>> need for two checks: aer_cap and get_firmware_first().
>>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> ---
>>
>> Changes since v2:
>>    - Added missing negation in pci_cleanup_aer_error_status_regs()
>>
>>   drivers/pci/pcie/aer.c | 17 ++++++++++-------
>>   1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> index a2e88386af28..40e5c86271d1 100644
>> --- a/drivers/pci/pcie/aer.c
>> +++ b/drivers/pci/pcie/aer.c
>> @@ -307,6 +307,12 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev)
>>   		aer_set_firmware_first(dev);
>>   	return dev->__aer_firmware_first;
>>   }
>> +
>> +static bool pcie_aer_is_kernel_first(struct pci_dev *dev)
>> +{
>> +	return !!dev->aer_cap && !pcie_aer_get_firmware_first(dev);
>> +}
> 
> I think it complicates things to have both "firmware_first" and
> "kernel_first" interfaces, so I would prefer to stick with the
> existing "firmware_first" style.
> 
>>   #define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
>>   				 PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)
>>   
>> @@ -337,10 +343,7 @@ bool aer_acpi_firmware_first(void)
>>   
>>   int pci_enable_pcie_error_reporting(struct pci_dev *dev)
>>   {
>> -	if (pcie_aer_get_firmware_first(dev))
>> -		return -EIO;
>> -
>> -	if (!dev->aer_cap)
>> +	if (!pcie_aer_is_kernel_first(dev))
>>   		return -EIO;
>>   
>>   	return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_AER_FLAGS);
> 
> This change doesn't actually fix anything, does it?  It looks like a
> cleanup that doesn't change the behavior.

Initially (v1), this was a one-liner, but someone had a complaint about 
having pcie_aer_get_firmware_first() boilerplate all over the place. 
That's why I added the "kernel_first" function (previous comment), and 
then updated this here for completeness. I'm also fine with v1.

>> @@ -349,7 +352,7 @@ EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting);
>>   
>>   int pci_disable_pcie_error_reporting(struct pci_dev *dev)
>>   {
>> -	if (pcie_aer_get_firmware_first(dev))
>> +	if (!pcie_aer_is_kernel_first(dev))
>>   		return -EIO;
> 
> This change does effectively add a test for dev->aer_cap.  That makes
> sense in terms of symmetry with pci_enable_pcie_error_reporting(),
> but I think it should be a separate patch because it's conceptually
> separate from the change below.
> 
> We should keep the existing behavior (but add the symmetry) here for
> now, but it's not clear to me that these paths should care about AER
> or firmware-first at all.  PCI_EXP_DEVCTL is not an AER register and
> we have the _HPX mechanism for firmware to influence it (which these
> paths currently ignore).  I suspect we should program these reporting
> enable bits in the core enumeration path instead of having drivers
> call these interfaces.

The headache is that FFS needs the reporting bit to stay enabled in 
order to get AER notifications. Disabling things here could really break 
firmware. Of course, that's a cyclical argument, since FW is broken by 
definition.

> If/when we make changes along these lines, the history will be easier
> to follow if *this* change is not connected with the change below to
> pci_cleanup_aer_error_status_regs().

I agree. I think it might be preferred then to go with v1, and leave the 
refactoring to a later time, since the extra changes are cosmetical and 
social.

>>   	return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL,
>> @@ -383,10 +386,10 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
>>   	if (!pci_is_pcie(dev))
>>   		return -ENODEV;
>>   
>> -	pos = dev->aer_cap;
>> -	if (!pos)
>> +	if (!pcie_aer_is_kernel_first(dev))
>>   		return -EIO;
> 
> This part makes sense to me, but I think I would rather have it match
> the existing style in pci_enable_pcie_error_reporting(), i.e., keep
> the test for dev->aer_cap and add a test for
> pcie_aer_get_firmware_first().

Had it that way in v1.

Alex

>> +	pos = dev->aer_cap;
>>   	port_type = pci_pcie_type(dev);
>>   	if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
>>   		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);
>> -- 
>> 2.17.1
>>

Re: [PATCH v2] PCI/AER: Do not clear AER bits if we don't own AER

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1767 bytes --]

Hi Alexandru,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pci/next]
[also build test ERROR on v4.18-rc6 next-20180724]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Alexandru-Gagniuc/PCI-AER-Do-not-clear-AER-bits-if-we-don-t-own-AER/20180724-235320
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: x86_64-randconfig-x008-201829 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/pci/pcie/aer.c: In function 'pci_enable_pcie_error_reporting':
>> drivers/pci/pcie/aer.c:371:7: error: implicit declaration of function 'pcie_aer_is_kernel_first'; did you mean 'pcie_aer_get_firmware_first'? [-Werror=implicit-function-declaration]
     if (!pcie_aer_is_kernel_first(dev))
          ^~~~~~~~~~~~~~~~~~~~~~~~
          pcie_aer_get_firmware_first
   cc1: some warnings being treated as errors

vim +371 drivers/pci/pcie/aer.c

   365	
   366	#define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
   367					 PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)
   368	
   369	int pci_enable_pcie_error_reporting(struct pci_dev *dev)
   370	{
 > 371		if (!pcie_aer_is_kernel_first(dev))
   372			return -EIO;
   373	
   374		return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_AER_FLAGS);
   375	}
   376	EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting);
   377	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27463 bytes --]

Re: [PATCH v2] PCI/AER: Do not clear AER bits if we don't own AER

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1719 bytes --]

Hi Alexandru,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pci/next]
[also build test ERROR on v4.18-rc6 next-20180724]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Alexandru-Gagniuc/PCI-AER-Do-not-clear-AER-bits-if-we-don-t-own-AER/20180724-235320
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: x86_64-randconfig-s1-07250001 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/pci/pcie/aer.c: In function 'pci_enable_pcie_error_reporting':
>> drivers/pci/pcie/aer.c:371:7: error: implicit declaration of function 'pcie_aer_is_kernel_first' [-Werror=implicit-function-declaration]
     if (!pcie_aer_is_kernel_first(dev))
          ^~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/pcie_aer_is_kernel_first +371 drivers/pci/pcie/aer.c

   365	
   366	#define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
   367					 PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)
   368	
   369	int pci_enable_pcie_error_reporting(struct pci_dev *dev)
   370	{
 > 371		if (!pcie_aer_is_kernel_first(dev))
   372			return -EIO;
   373	
   374		return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_AER_FLAGS);
   375	}
   376	EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting);
   377	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31241 bytes --]

Re: [PATCH] PCI/AER: Do not clear AER bits if we don't own AER

[Bjorn Helgaas]

On Tue, Jul 17, 2018 at 10:31:23AM -0500, Alexandru Gagniuc wrote:
> When we don't own AER, we shouldn't touch the AER error bits. This
> happens unconditionally on device probe(). Clearing AER bits
> willy-nilly might cause firmware to miss errors. Instead
> these bits should get cleared by FFS, or via ACPI _HPX method.
> 
> This race is mostly of theoretical significance, as it is not easy to
> reasonably demonstrate it in testing.
> 
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  drivers/pci/pcie/aer.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index a2e88386af28..18037a2a8231 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -383,6 +383,9 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
>  	if (!pci_is_pcie(dev))
>  		return -ENODEV;
>  
> +	if (pcie_aer_get_firmware_first(dev))
> +		return -EIO;

I like this patch.

Do we need the same thing in the following places that also clear AER
status bits or write AER control bits?

  enable_ecrc_checking()
  disable_ecrc_checking()
  pci_cleanup_aer_uncorrect_error_status()
  pci_aer_clear_fatal_status()

>  	pos = dev->aer_cap;
>  	if (!pos)
>  		return -EIO;
> -- 
> 2.14.3
> 

Re: [PATCH] PCI/AER: Do not clear AER bits if we don't own AER

[Alex_Gagniuc]

On 08/09/2018 09:16 AM, Bjorn Helgaas wrote:
> On Tue, Jul 17, 2018 at 10:31:23AM -0500, Alexandru Gagniuc wrote:
>> When we don't own AER, we shouldn't touch the AER error bits. This
>> happens unconditionally on device probe(). Clearing AER bits
>> willy-nilly might cause firmware to miss errors. Instead
>> these bits should get cleared by FFS, or via ACPI _HPX method.
>>
>> This race is mostly of theoretical significance, as it is not easy to
>> reasonably demonstrate it in testing.
>>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> ---
>>   drivers/pci/pcie/aer.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> index a2e88386af28..18037a2a8231 100644
>> --- a/drivers/pci/pcie/aer.c
>> +++ b/drivers/pci/pcie/aer.c
>> @@ -383,6 +383,9 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
>>   	if (!pci_is_pcie(dev))
>>   		return -ENODEV;
>>   
>> +	if (pcie_aer_get_firmware_first(dev))
>> +		return -EIO;
> 
> I like this patch.
> 
> Do we need the same thing in the following places that also clear AER
> status bits or write AER control bits?

In theory, every exported function would guard for this. I think the 
idea a long long time ago was that the check happens during 
initialization, and the others are not hit.

>    enable_ecrc_checking()
>    disable_ecrc_checking()

I don't immediately see how this would affect FFS, but the bits are part 
of the AER capability structure. According to the FFS model, those would 
be owned by FW, and we'd have to avoid touching them.

>    pci_cleanup_aer_uncorrect_error_status()

This probably should be guarded. It's only called from a few specific 
drivers, so the impact is not as high as being called from the core.

>    pci_aer_clear_fatal_status()

This is only called when doing fatal_recovery, right?
For practical considerations this is not an issue today. The ACPI error 
handling code currently crashes when it encounters any fatal error, so 
we wouldn't hit this in the FFS case.

If the ACPI code pulls its thinking appendage out of the other end of 
the digestive tract, then we could be hitting this in the future. For 
correctness, guarding makes sense.

The PCIe standards contact I usually talk to about these PCIe subtleties 
is currently on vacation. The number one issue was a FFS corner case 
with OS clearing bits on probe. The other functions you mention are a 
corner case of a corner case. The big fish is 
pci_cleanup_aer_error_status_regs() on probe(), and it would be nice to 
have that resolved.

I'll sync up with Austin when he gets back to see about the other 
functions though I suspect we'll end up fixing them as well.

Alex

>>   	pos = dev->aer_cap;
>>   	if (!pos)
>>   		return -EIO;
>> -- 
>> 2.14.3
>>
> 

Re: [PATCH] PCI/AER: Do not clear AER bits if we don't own AER

[Bjorn Helgaas]

On Thu, Aug 09, 2018 at 04:46:32PM +0000, Alex_Gagniuc@Dellteam.com wrote:
> On 08/09/2018 09:16 AM, Bjorn Helgaas wrote:
> > On Tue, Jul 17, 2018 at 10:31:23AM -0500, Alexandru Gagniuc wrote:
> >> When we don't own AER, we shouldn't touch the AER error bits. This
> >> happens unconditionally on device probe(). Clearing AER bits
> >> willy-nilly might cause firmware to miss errors. Instead
> >> these bits should get cleared by FFS, or via ACPI _HPX method.
> >>
> >> This race is mostly of theoretical significance, as it is not easy to
> >> reasonably demonstrate it in testing.
> >>
> >> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> >> ---
> >>   drivers/pci/pcie/aer.c | 3 +++
> >>   1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> >> index a2e88386af28..18037a2a8231 100644
> >> --- a/drivers/pci/pcie/aer.c
> >> +++ b/drivers/pci/pcie/aer.c
> >> @@ -383,6 +383,9 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
> >>   	if (!pci_is_pcie(dev))
> >>   		return -ENODEV;
> >>   
> >> +	if (pcie_aer_get_firmware_first(dev))
> >> +		return -EIO;
> > 
> > I like this patch.
> > 
> > Do we need the same thing in the following places that also clear AER
> > status bits or write AER control bits?
> 
> In theory, every exported function would guard for this. I think the 
> idea a long long time ago was that the check happens during 
> initialization, and the others are not hit.
> 
> >    enable_ecrc_checking()
> >    disable_ecrc_checking()
> 
> I don't immediately see how this would affect FFS, but the bits are part 
> of the AER capability structure. According to the FFS model, those would 
> be owned by FW, and we'd have to avoid touching them.

Per ACPI v6.2, sec 18.3.2.4, the HEST may contain entries for Root
Ports that contain the FIRMWARE_FIRST flag as well as values the OS is
supposed to write to several AER capability registers.  It looks like
we currently ignore everything except the FIRMWARE_FIRST and GLOBAL
flags (ACPI_HEST_FIRMWARE_FIRST and ACPI_HEST_GLOBAL in Linux).

That seems like a pretty major screwup and more than I want to fix
right now.

> >    pci_cleanup_aer_uncorrect_error_status()
> 
> This probably should be guarded. It's only called from a few specific 
> drivers, so the impact is not as high as being called from the core.
> 
> >    pci_aer_clear_fatal_status()
> 
> This is only called when doing fatal_recovery, right?

True.  It takes a lot of analysis to convince oneself that this is not
used in the firmware-first path, so I think we should add a guard
there.

> For practical considerations this is not an issue today. The ACPI error 
> handling code currently crashes when it encounters any fatal error, so 
> we wouldn't hit this in the FFS case.

I wasn't aware the firmware-first path was *that* broken.  Are there
problem reports for this?  Is this a regression?

> The PCIe standards contact I usually talk to about these PCIe subtleties 
> is currently on vacation. The number one issue was a FFS corner case 
> with OS clearing bits on probe. The other functions you mention are a 
> corner case of a corner case. The big fish is 
> pci_cleanup_aer_error_status_regs() on probe(), and it would be nice to 
> have that resolved.
> 
> I'll sync up with Austin when he gets back to see about the other 
> functions though I suspect we'll end up fixing them as well.

I'd like to fix all the obvious cases at once (excluding the ECRC
stuff).  What do you think about the following patch?


commit 15ed68dcc26864c849a12a36db4d4771bad7991f
Author: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Date:   Tue Jul 17 10:31:23 2018 -0500

    PCI/AER: Don't clear AER bits if error handling is Firmware-First
    
    If the platform requests Firmware-First error handling, firmware is
    responsible for reading and clearing AER status bits.  If OSPM also clears
    them, we may miss errors.  See ACPI v6.2, sec 18.3.2.5 and 18.4.
    
    This race is mostly of theoretical significance, as it is not easy to
    reasonably demonstrate it in testing.
    
    Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
    [bhelgaas: add similar guards to pci_cleanup_aer_uncorrect_error_status()
    and pci_aer_clear_fatal_status()]
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index c6cc855bfa22..4e823ae051a7 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -397,6 +397,9 @@ int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
 	if (!pos)
 		return -EIO;
 
+	if (pcie_aer_get_firmware_first(dev))
+		return -EIO;
+
 	/* Clear status bits for ERR_NONFATAL errors only */
 	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
 	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &sev);
@@ -417,6 +420,9 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev)
 	if (!pos)
 		return;
 
+	if (pcie_aer_get_firmware_first(dev))
+		return;
+
 	/* Clear status bits for ERR_FATAL errors only */
 	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
 	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &sev);
@@ -438,6 +444,9 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
 	if (!pos)
 		return -EIO;
 
+	if (pcie_aer_get_firmware_first(dev))
+		return -EIO;
+
 	port_type = pci_pcie_type(dev);
 	if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
 		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);

Re: [PATCH] PCI/AER: Do not clear AER bits if we don't own AER

[Alex G.]

On 08/09/2018 01:29 PM, Bjorn Helgaas wrote:
> On Thu, Aug 09, 2018 at 04:46:32PM +0000, Alex_Gagniuc@Dellteam.com wrote:
>> On 08/09/2018 09:16 AM, Bjorn Helgaas wrote:
(snip_
>>>     enable_ecrc_checking()
>>>     disable_ecrc_checking()
>>
>> I don't immediately see how this would affect FFS, but the bits are part
>> of the AER capability structure. According to the FFS model, those would
>> be owned by FW, and we'd have to avoid touching them.
> 
> Per ACPI v6.2, sec 18.3.2.4, the HEST may contain entries for Root
> Ports that contain the FIRMWARE_FIRST flag as well as values the OS is
> supposed to write to several AER capability registers.  It looks like
> we currently ignore everything except the FIRMWARE_FIRST and GLOBAL
> flags (ACPI_HEST_FIRMWARE_FIRST and ACPI_HEST_GLOBAL in Linux).
> 
> That seems like a pretty major screwup and more than I want to fix
> right now.

The logic is not very clear, but I think it goes like this:
For GLOBAL and FFS, disable native AER everywhere.
When !GLOBAL and FFS, then only disable native AER for the root port 
described by the HEST entry.

aer_acpi_firmware_first() doesn't care about context. Though check 
aer_set_firmware_first(), where we pass a pci_device as a context.

The FFS implementations I've seen have one HEST entry, with GLOBAL and 
FFS. I have yet to see more fine-grained control of root ports. I 
suspect that if we had this finer control from HEST, we'd honor it.

I do eventually want to get a test BIOS with different HEST entries, and 
make sure things work as intended. Though BIOS team is overloaded with 
other requests.

>>>     pci_cleanup_aer_uncorrect_error_status()
>>
>> This probably should be guarded. It's only called from a few specific
>> drivers, so the impact is not as high as being called from the core.
>>
>>>     pci_aer_clear_fatal_status()
>>
>> This is only called when doing fatal_recovery, right?
> 
> True.  It takes a lot of analysis to convince oneself that this is not
> used in the firmware-first path, so I think we should add a guard
> there.

I agree. GHES has a header severity and a severity field for each 
section. All BIOSes I've seen do fatal/fatal, though a BIOS that would 
report correctable/fatal, would bypass the apei code and take us here.

>> For practical considerations this is not an issue today. The ACPI error
>> handling code currently crashes when it encounters any fatal error, so
>> we wouldn't hit this in the FFS case.
> 
> I wasn't aware the firmware-first path was *that* broken.  Are there
> problem reports for this?  Is this a regression?

It's been like this since, I believe, 3.10, and probably much earlier. 
All reports that I have seen of linux crashing on surprise hot-plug have 
been caused by the panic() call in the apei code. Dell BIOSes do an 
extreme amount of work to determine when it's safe to _not_ report 
errors to the OS, since all known OSes crash on this path.

Fun fact: there's even dedicated hardware to accomplish the above.

>> The PCIe standards contact I usually talk to about these PCIe subtleties
>> is currently on vacation. The number one issue was a FFS corner case
>> with OS clearing bits on probe. The other functions you mention are a
>> corner case of a corner case. The big fish is
>> pci_cleanup_aer_error_status_regs() on probe(), and it would be nice to
>> have that resolved.
>>
>> I'll sync up with Austin when he gets back to see about the other
>> functions though I suspect we'll end up fixing them as well.
> 
> I'd like to fix all the obvious cases at once (excluding the ECRC
> stuff).  What do you think about the following patch?

That looks very sensible to me. Thank you for updating it :).
I'm pulling in the changes right now to run some quick tests, and I 
don't expect any trouble.

Alex

> commit 15ed68dcc26864c849a12a36db4d4771bad7991f
> Author: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Date:   Tue Jul 17 10:31:23 2018 -0500
> 
>      PCI/AER: Don't clear AER bits if error handling is Firmware-First
>      
>      If the platform requests Firmware-First error handling, firmware is
>      responsible for reading and clearing AER status bits.  If OSPM also clears
>      them, we may miss errors.  See ACPI v6.2, sec 18.3.2.5 and 18.4.
>      
>      This race is mostly of theoretical significance, as it is not easy to
>      reasonably demonstrate it in testing.
>      
>      Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>      [bhelgaas: add similar guards to pci_cleanup_aer_uncorrect_error_status()
>      and pci_aer_clear_fatal_status()]
>      Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index c6cc855bfa22..4e823ae051a7 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -397,6 +397,9 @@ int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
>   	if (!pos)
>   		return -EIO;
>   
> +	if (pcie_aer_get_firmware_first(dev))
> +		return -EIO;
> +
>   	/* Clear status bits for ERR_NONFATAL errors only */
>   	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
>   	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &sev);
> @@ -417,6 +420,9 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev)
>   	if (!pos)
>   		return;
>   
> +	if (pcie_aer_get_firmware_first(dev))
> +		return;
> +
>   	/* Clear status bits for ERR_FATAL errors only */
>   	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
>   	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &sev);
> @@ -438,6 +444,9 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
>   	if (!pos)
>   		return -EIO;
>   
> +	if (pcie_aer_get_firmware_first(dev))
> +		return -EIO;
> +
>   	port_type = pci_pcie_type(dev);
>   	if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
>   		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);
> 

Re: [PATCH] PCI/AER: Do not clear AER bits if we don't own AER

[Bjorn Helgaas]

On Thu, Aug 09, 2018 at 02:00:23PM -0500, Alex G. wrote:
> On 08/09/2018 01:29 PM, Bjorn Helgaas wrote:
> > On Thu, Aug 09, 2018 at 04:46:32PM +0000, Alex_Gagniuc@Dellteam.com wrote:
> > > On 08/09/2018 09:16 AM, Bjorn Helgaas wrote:
> (snip_
> > > >     enable_ecrc_checking()
> > > >     disable_ecrc_checking()
> > > 
> > > I don't immediately see how this would affect FFS, but the bits are part
> > > of the AER capability structure. According to the FFS model, those would
> > > be owned by FW, and we'd have to avoid touching them.
> > 
> > Per ACPI v6.2, sec 18.3.2.4, the HEST may contain entries for Root
> > Ports that contain the FIRMWARE_FIRST flag as well as values the OS is
> > supposed to write to several AER capability registers.  It looks like
> > we currently ignore everything except the FIRMWARE_FIRST and GLOBAL
> > flags (ACPI_HEST_FIRMWARE_FIRST and ACPI_HEST_GLOBAL in Linux).
> > 
> > That seems like a pretty major screwup and more than I want to fix
> > right now.
> 
> The logic is not very clear, but I think it goes like this:
> For GLOBAL and FFS, disable native AER everywhere.
> When !GLOBAL and FFS, then only disable native AER for the root port
> described by the HEST entry.

I agree the code is convoluted, but that sounds right to me.

What I meant is that we ignore the values the HEST entry tells us
we're supposed to write to Device Control and the AER Uncorrectable
Error Mask, Uncorrectable Error Severity, Correctable Error Mask, and
AER Capabilities and Control.

> > > For practical considerations this is not an issue today. The ACPI error
> > > handling code currently crashes when it encounters any fatal error, so
> > > we wouldn't hit this in the FFS case.
> > 
> > I wasn't aware the firmware-first path was *that* broken.  Are there
> > problem reports for this?  Is this a regression?
> 
> It's been like this since, I believe, 3.10, and probably much earlier. All
> reports that I have seen of linux crashing on surprise hot-plug have been
> caused by the panic() call in the apei code. Dell BIOSes do an extreme
> amount of work to determine when it's safe to _not_ report errors to the OS,
> since all known OSes crash on this path.

Oh, is this the __ghes_panic() path?  If so, I'm going to turn away
and plead ignorance unless the PCI core is doing something wrong that
eventually results in that panic.

Re: [PATCH] PCI/AER: Do not clear AER bits if we don't own AER

[Alex G.]

On 08/09/2018 02:18 PM, Bjorn Helgaas wrote:
> On Thu, Aug 09, 2018 at 02:00:23PM -0500, Alex G. wrote:
>> On 08/09/2018 01:29 PM, Bjorn Helgaas wrote:
>>> On Thu, Aug 09, 2018 at 04:46:32PM +0000, Alex_Gagniuc@Dellteam.com wrote:
>>>> On 08/09/2018 09:16 AM, Bjorn Helgaas wrote:
>> (snip_
>>>>>      enable_ecrc_checking()
>>>>>      disable_ecrc_checking()
>>>>
>>>> I don't immediately see how this would affect FFS, but the bits are part
>>>> of the AER capability structure. According to the FFS model, those would
>>>> be owned by FW, and we'd have to avoid touching them.
>>>
>>> Per ACPI v6.2, sec 18.3.2.4, the HEST may contain entries for Root
>>> Ports that contain the FIRMWARE_FIRST flag as well as values the OS is
>>> supposed to write to several AER capability registers.  It looks like
>>> we currently ignore everything except the FIRMWARE_FIRST and GLOBAL
>>> flags (ACPI_HEST_FIRMWARE_FIRST and ACPI_HEST_GLOBAL in Linux).
>>>
>>> That seems like a pretty major screwup and more than I want to fix
>>> right now.
>>
>> The logic is not very clear, but I think it goes like this:
>> For GLOBAL and FFS, disable native AER everywhere.
>> When !GLOBAL and FFS, then only disable native AER for the root port
>> described by the HEST entry.
> 
> I agree the code is convoluted, but that sounds right to me.
> 
> What I meant is that we ignore the values the HEST entry tells us
> we're supposed to write to Device Control and the AER Uncorrectable
> Error Mask, Uncorrectable Error Severity, Correctable Error Mask, and
> AER Capabilities and Control.

Wait, what? _HPX has the same information. This is madness!
Since root ports are not hot-swappable, the BIOS normally programs those 
registers. Even if linux doesn't apply said masks, the programming BIOS 
did should be sufficient to have *cough* correct *cough* behavior.

>>>> For practical considerations this is not an issue today. The ACPI error
>>>> handling code currently crashes when it encounters any fatal error, so
>>>> we wouldn't hit this in the FFS case.
>>>
>>> I wasn't aware the firmware-first path was *that* broken.  Are there
>>> problem reports for this?  Is this a regression?
>>
>> It's been like this since, I believe, 3.10, and probably much earlier. All
>> reports that I have seen of linux crashing on surprise hot-plug have been
>> caused by the panic() call in the apei code. Dell BIOSes do an extreme
>> amount of work to determine when it's safe to _not_ report errors to the OS,
>> since all known OSes crash on this path.
> 
> Oh, is this the __ghes_panic() path?  If so, I'm going to turn away
> and plead ignorance unless the PCI core is doing something wrong that
> eventually results in that panic.

I agree, and I'll quote you on that!

Alex

Re: [PATCH v3] PCI: Check for PCIe downtraining conditions

[Bjorn Helgaas]

[+cc Mike (hfi1)]

On Mon, Jul 16, 2018 at 10:28:35PM +0000, Alex_Gagniuc@Dellteam.com wrote:
> On 7/16/2018 4:17 PM, Bjorn Helgaas wrote:
> >> ...
> >> The easiest way to detect this is with pcie_print_link_status(),
> >> since the bottleneck is usually the link that is downtrained. It's not
> >> a perfect solution, but it works extremely well in most cases.
> > 
> > This is an interesting idea.  I have two concerns:
> > 
> > Some drivers already do this on their own, and we probably don't want
> > duplicate output for those devices.  In most cases (ixgbe and mlx* are
> > exceptions), the drivers do this unconditionally so we *could* remove
> > it from the driver if we add it to the core.  The dmesg order would
> > change, and the message wouldn't be associated with the driver as it
> > now is.
> 
> Oh, there are only 8 users of that. Even I could patch up the drivers to 
> remove the call, assuming we reach agreement about this change.
> 
> > Also, I think some of the GPU devices might come up at a lower speed,
> > then download firmware, then reset the device so it comes up at a
> > higher speed.  I think this patch will make us complain about about
> > the low initial speed, which might confuse users.
> 
> I spoke to one of the PCIe spec writers. It's allowable for a device to 
> downtrain speed or width. It would also be extremely dumb to downtrain 
> with the intent to re-train at a higher speed later, but it's possible 
> devices do dumb stuff like that. That's why it's an informational 
> message, instead of a warning.

FWIW, here's some of the discussion related to hfi1 from [1]:

  > Btw, why is the driver configuring the PCIe link speed?  Isn't
  > this something we should be handling in the PCI core?

  The device comes out of reset at the 5GT/s speed. The driver
  downloads device firmware, programs PCIe registers, and co-ordinates
  the transition to 8GT/s.

  This recipe is device specific and is therefore implemented in the
  hfi1 driver built on top of PCI core functions and macros.

Also several DRM drivers seem to do this (see cik_pcie_gen3_enable(),
si_pcie_gen3_enable()); from [2]:

  My understanding was that some platfoms only bring up the link in gen 1
  mode for compatibility reasons. 

[1] https://lkml.kernel.org/r/32E1700B9017364D9B60AED9960492BC627FF54C@fmsmsx120.amr.corp.intel.com
[2] https://lkml.kernel.org/r/BN6PR12MB1809BD30AA5B890C054F9832F7B50@BN6PR12MB1809.namprd12.prod.outlook.com

> Another case: Some devices (lower-end GPUs) use silicon (and marketing) 
> that advertises x16, but they're only routed for x8. I'm okay with 
> seeing an informational message in this case. In fact, I didn't know 
> that my Quadro card for three years is only wired for x8 until I was 
> testing this patch.

Yeah, it's probably OK.  I don't want bug reports from people who
think something's broken when it's really just a hardware limitation
of their system.  But hopefully the message is not alarming.

> > So I'm not sure whether it's better to do this in the core for all
> > devices, or if we should just add it to the high-performance drivers
> > that really care.
> 
> You're thinking "do I really need that bandwidth" because I'm using a 
> function called "_bandwidth_". The point of the change is very far from 
> that: it is to help in system troubleshooting by detecting downtraining 
> conditions.

I'm not sure what you think I'm thinking :)  My question is whether
it's worthwhile to print this extra information for *every* PCIe
device, given that your use case is the tiny percentage of broken
systems.

If we only printed the info in the "bw_avail < bw_cap" case, i.e.,
when the device is capable of more than it's getting, that would make
a lot of sense to me.  The normal case line is more questionable.  I
think the reason that's there is because the network drivers are very
performance sensitive and like to see that info all the time.

Maybe we need something like this:

  pcie_print_link_status(struct pci_dev *dev, int verbose)
  {
    ...
    if (bw_avail >= bw_cap) {
      if (verbose)
        pci_info(dev, "... available PCIe bandwidth ...");
    } else
      pci_info(dev, "... available PCIe bandwidth, limited by ...");
  }

So the core could print only the potential problems with:

  pcie_print_link_status(dev, 0);

and drivers that really care even if there's no problem could do:

  pcie_print_link_status(dev, 1);

> >> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> [snip]
> >> +	/* Look from the device up to avoid downstream ports with no devices. */
> >> +	if ((pci_pcie_type(dev) != PCI_EXP_TYPE_ENDPOINT) &&
> >> +	    (pci_pcie_type(dev) != PCI_EXP_TYPE_LEG_END) &&
> >> +	    (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM))
> >> +		return;
> > 
> > Do we care about Upstream Ports here?  
> 
> YES! Switches. e.g. an x16 switch with 4x downstream ports could 
> downtrain at 8x and 4x, and we'd never catch it.

OK, I think I see your point: if the upstream port *could* do 16x but
only trains to 4x, and two endpoints below it are both capable of 4x,
the endpoints *think* they're happy but in fact they have to share 4x
when they could use more.

Bjorn

[PATCH v2] PCI/AER: Do not clear AER bits if we don't own AER

[Alexandru Gagniuc]

When we don't own AER, we shouldn't touch the AER error bits. Clearing
error bits willy-nilly might cause firmware to miss some errors. In
theory, these bits get cleared by FFS, or via ACPI _HPX method. These
mechanisms are not subject to the problem.

This race is mostly of theoretical significance, since I can't
reasonably demonstrate this race in the lab.

On a side-note, pcie_aer_is_kernel_first() is created to alleviate the
need for two checks: aer_cap and get_firmware_first().

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 drivers/pci/pcie/aer.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index a2e88386af28..85c3e173c025 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -307,6 +307,12 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev)
 		aer_set_firmware_first(dev);
 	return dev->__aer_firmware_first;
 }
+
+static bool pcie_aer_is_kernel_first(struct pci_dev *dev)
+{
+	return !!dev->aer_cap && !pcie_aer_get_firmware_first(dev);
+}
+
 #define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
 				 PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)
 
@@ -337,10 +343,7 @@ bool aer_acpi_firmware_first(void)
 
 int pci_enable_pcie_error_reporting(struct pci_dev *dev)
 {
-	if (pcie_aer_get_firmware_first(dev))
-		return -EIO;
-
-	if (!dev->aer_cap)
+	if (!pcie_aer_is_kernel_first(dev))
 		return -EIO;
 
 	return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_AER_FLAGS);
@@ -349,7 +352,7 @@ EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting);
 
 int pci_disable_pcie_error_reporting(struct pci_dev *dev)
 {
-	if (pcie_aer_get_firmware_first(dev))
+	if (!pcie_aer_is_kernel_first(dev))
 		return -EIO;
 
 	return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL,
@@ -383,10 +386,10 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
 	if (!pci_is_pcie(dev))
 		return -ENODEV;
 
-	pos = dev->aer_cap;
-	if (!pos)
+	if (pcie_aer_is_kernel_first(dev))
 		return -EIO;
 
+	pos = dev->aer_cap;
 	port_type = pci_pcie_type(dev);
 	if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
 		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);
-- 
2.17.1

Re: [PATCH v2] PCI/AER: Do not clear AER bits if we don't own AER

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2176 bytes --]

Hi Alexandru,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on pci/next]
[also build test WARNING on v4.18-rc6 next-20180724]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Alexandru-Gagniuc/PCI-AER-Do-not-clear-AER-bits-if-we-don-t-own-AER/20180725-003708
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: x86_64-randconfig-s0-07250049 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   In file included from include/linux/string.h:6:0,
                    from include/linux/uuid.h:20,
                    from include/linux/cper.h:24,
                    from drivers/pci/pcie/aer.c:15:
   drivers/pci/pcie/aer.c: In function 'pci_enable_pcie_error_reporting':
   drivers/pci/pcie/aer.c:371:7: error: implicit declaration of function 'pcie_aer_is_kernel_first' [-Werror=implicit-function-declaration]
     if (!pcie_aer_is_kernel_first(dev))
          ^
   include/linux/compiler.h:58:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> drivers/pci/pcie/aer.c:371:2: note: in expansion of macro 'if'
     if (!pcie_aer_is_kernel_first(dev))
     ^~
   cc1: some warnings being treated as errors

vim +/if +371 drivers/pci/pcie/aer.c

   365	
   366	#define	PCI_EXP_AER_FLAGS	(PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
   367					 PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE)
   368	
   369	int pci_enable_pcie_error_reporting(struct pci_dev *dev)
   370	{
 > 371		if (!pcie_aer_is_kernel_first(dev))
   372			return -EIO;
   373	
   374		return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_AER_FLAGS);
   375	}
   376	EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting);
   377	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32082 bytes --]