[PATCH] pci: Don't set RCB bit in LNKCTL if the upstream bridge hasn't

[PATCH] pci: Don't set RCB bit in LNKCTL if the upstream bridge hasn't

[Johannes Thumshirn]

The Read Completion Boundary bit must only be set on a device or endpoint if
it is set on the upstream bridge.

Fixes: 7a1562d4f ("PCI: Apply _HPX Link Control settings to all devices with a link")
Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/pci/probe.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ab00267..5007b96 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1439,6 +1439,16 @@ static void program_hpp_type1(struct pci_dev *dev, struct hpp_type1 *hpp)
 		dev_warn(&dev->dev, "PCI-X settings not supported\n");
 }
 
+static bool pcie_get_upstream_rcb(struct pci_dev *dev)
+{
+	struct pci_dev *bridge = pci_upstream_bridge(dev);
+	u16 lnkctl;
+
+	pcie_capability_read_word(bridge, PCI_EXP_LNKCTL, &lnkctl);
+
+	return lnkctl & PCI_EXP_LNKCTL_RCB;
+}
+
 static void program_hpp_type2(struct pci_dev *dev, struct hpp_type2 *hpp)
 {
 	int pos;
@@ -1468,9 +1478,21 @@ static void program_hpp_type2(struct pci_dev *dev, struct hpp_type2 *hpp)
 			~hpp->pci_exp_devctl_and, hpp->pci_exp_devctl_or);
 
 	/* Initialize Link Control Register */
-	if (pcie_cap_has_lnkctl(dev))
+	if (pcie_cap_has_lnkctl(dev)) {
+		bool us_rcb;
+		u16 clear;
+		u16 set;
+
+		us_rcb = pcie_get_upstream_rcb(dev);
+
+		clear = ~hpp->pci_exp_lnkctl_and;
+		set = hpp->pci_exp_lnkctl_or;
+		if (!us_rcb)
+			set &= ~PCI_EXP_LNKCTL_RCB;
+
 		pcie_capability_clear_and_set_word(dev, PCI_EXP_LNKCTL,
-			~hpp->pci_exp_lnkctl_and, hpp->pci_exp_lnkctl_or);
+						  clear, set);
+	}
 
 	/* Find Advanced Error Reporting Enhanced Capability */
 	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
-- 
1.8.5.6

Re: [PATCH] pci: Don't set RCB bit in LNKCTL if the upstream bridge hasn't

[Hannes Reinecke]

On 10/26/2016 03:53 PM, Johannes Thumshirn wrote:
> The Read Completion Boundary bit must only be set on a device or endpoint if
> it is set on the upstream bridge.
> 
> Fixes: 7a1562d4f ("PCI: Apply _HPX Link Control settings to all devices with a link")
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  drivers/pci/probe.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
This fixes a regression where the mlx4 driver would not load (and, in
fact, crash) on certain Ivybridge servers.

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH] pci: Don't set RCB bit in LNKCTL if the upstream bridge hasn't

[Bjorn Helgaas]

Hi Johannes,

On Wed, Oct 26, 2016 at 03:53:34PM +0200, Johannes Thumshirn wrote:
> The Read Completion Boundary bit must only be set on a device or endpoint if
> it is set on the upstream bridge.
> 
> Fixes: 7a1562d4f ("PCI: Apply _HPX Link Control settings to all devices with a link")

Can you please include a spec citation and a pointer to the bug report?

> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  drivers/pci/probe.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ab00267..5007b96 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1439,6 +1439,16 @@ static void program_hpp_type1(struct pci_dev *dev, struct hpp_type1 *hpp)
>  		dev_warn(&dev->dev, "PCI-X settings not supported\n");
>  }
>  
> +static bool pcie_get_upstream_rcb(struct pci_dev *dev)
> +{
> +	struct pci_dev *bridge = pci_upstream_bridge(dev);
> +	u16 lnkctl;
> +
> +	pcie_capability_read_word(bridge, PCI_EXP_LNKCTL, &lnkctl);
> +
> +	return lnkctl & PCI_EXP_LNKCTL_RCB;
> +}
> +
>  static void program_hpp_type2(struct pci_dev *dev, struct hpp_type2 *hpp)
>  {
>  	int pos;
> @@ -1468,9 +1478,21 @@ static void program_hpp_type2(struct pci_dev *dev, struct hpp_type2 *hpp)
>  			~hpp->pci_exp_devctl_and, hpp->pci_exp_devctl_or);
>  
>  	/* Initialize Link Control Register */
> -	if (pcie_cap_has_lnkctl(dev))
> +	if (pcie_cap_has_lnkctl(dev)) {
> +		bool us_rcb;
> +		u16 clear;
> +		u16 set;
> +
> +		us_rcb = pcie_get_upstream_rcb(dev);
> +
> +		clear = ~hpp->pci_exp_lnkctl_and;
> +		set = hpp->pci_exp_lnkctl_or;
> +		if (!us_rcb)
> +			set &= ~PCI_EXP_LNKCTL_RCB;
> +
>  		pcie_capability_clear_and_set_word(dev, PCI_EXP_LNKCTL,
> -			~hpp->pci_exp_lnkctl_and, hpp->pci_exp_lnkctl_or);
> +						  clear, set);
> +	}
>  
>  	/* Find Advanced Error Reporting Enhanced Capability */
>  	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> -- 
> 1.8.5.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pci: Don't set RCB bit in LNKCTL if the upstream bridge hasn't

[Hannes Reinecke]

On 10/26/2016 09:43 PM, Bjorn Helgaas wrote:
> Hi Johannes,
> 
> On Wed, Oct 26, 2016 at 03:53:34PM +0200, Johannes Thumshirn wrote:
>> The Read Completion Boundary bit must only be set on a device or endpoint if
>> it is set on the upstream bridge.
>>
>> Fixes: 7a1562d4f ("PCI: Apply _HPX Link Control settings to all devices with a link")
> 
> Can you please include a spec citation and a pointer to the bug report?
> 
PCI Express Base Specification 1.1,
section 2.3.1.1. Data Return for Read Requests:

The Read Completion Boundary (RCB) parameter determines the naturally
aligned address boundaries on which a Read Request may be serviced with
multiple Completions
o For a Root Complex, RCB is 64 bytes or 128 bytes
  o This value is reported through a configuration register
    (see Section 7.8)
  Note: Bridges and Endpoints may implement a corresponding command
  bit which may be set by system software to indicate the RCB value
  for the Root Complex, allowing the Bridge/Endpoint to optimize its
  behavior when the Root Complex’s RCB is 128 bytes.
o For all other system elements, RCB is 128 bytes

In this particular case the _HPX method was causing the RCB for all PCI
devices to be set to 128 bytes, while the root bridge remained at 64 bytes.
While this is arguably a BIOS bug, earlier linux version (ie without the
mentioned patch) were running fine, so this is actually a regression.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH] pci: Don't set RCB bit in LNKCTL if the upstream bridge hasn't

[Bjorn Helgaas]

On Thu, Oct 27, 2016 at 07:42:27AM +0200, Hannes Reinecke wrote:
> On 10/26/2016 09:43 PM, Bjorn Helgaas wrote:
> > Hi Johannes,
> > 
> > On Wed, Oct 26, 2016 at 03:53:34PM +0200, Johannes Thumshirn wrote:
> >> The Read Completion Boundary bit must only be set on a device or endpoint if
> >> it is set on the upstream bridge.
> >>
> >> Fixes: 7a1562d4f ("PCI: Apply _HPX Link Control settings to all devices with a link")
> > 
> > Can you please include a spec citation and a pointer to the bug report?
> > 
> PCI Express Base Specification 1.1,
> section 2.3.1.1. Data Return for Read Requests:
> 
> The Read Completion Boundary (RCB) parameter determines the naturally
> aligned address boundaries on which a Read Request may be serviced with
> multiple Completions
> o For a Root Complex, RCB is 64 bytes or 128 bytes
>   o This value is reported through a configuration register
>     (see Section 7.8)
>   Note: Bridges and Endpoints may implement a corresponding command
>   bit which may be set by system software to indicate the RCB value
>   for the Root Complex, allowing the Bridge/Endpoint to optimize its
>   behavior when the Root Complex’s RCB is 128 bytes.
> o For all other system elements, RCB is 128 bytes
> 
> In this particular case the _HPX method was causing the RCB for all PCI
> devices to be set to 128 bytes, while the root bridge remained at 64 bytes.
> While this is arguably a BIOS bug, earlier linux version (ie without the
> mentioned patch) were running fine, so this is actually a regression.

Thanks!  I can fold this into the changelog.

I assume you didn't mention a bugzilla or similar URL because this was
found internally?  I'd still like a clue about what this issue looks
like to a user, because that helps connect future problem reports with
this fix.

And I suppose that since 7a1562d4f2d0 appeared in v3.18, we maybe
should consider marking the fix for stable?

Bjorn

Re: [PATCH] pci: Don't set RCB bit in LNKCTL if the upstream bridge hasn't

[Hannes Reinecke]

On 10/27/2016 01:51 PM, Bjorn Helgaas wrote:
> On Thu, Oct 27, 2016 at 07:42:27AM +0200, Hannes Reinecke wrote:
>> On 10/26/2016 09:43 PM, Bjorn Helgaas wrote:
>>> Hi Johannes,
>>>
>>> On Wed, Oct 26, 2016 at 03:53:34PM +0200, Johannes Thumshirn wrote:
>>>> The Read Completion Boundary bit must only be set on a device or endpoint if
>>>> it is set on the upstream bridge.
>>>>
>>>> Fixes: 7a1562d4f ("PCI: Apply _HPX Link Control settings to all devices with a link")
>>>
>>> Can you please include a spec citation and a pointer to the bug report?
>>>
>> PCI Express Base Specification 1.1,
>> section 2.3.1.1. Data Return for Read Requests:
>>
>> The Read Completion Boundary (RCB) parameter determines the naturally
>> aligned address boundaries on which a Read Request may be serviced with
>> multiple Completions
>> o For a Root Complex, RCB is 64 bytes or 128 bytes
>>   o This value is reported through a configuration register
>>     (see Section 7.8)
>>   Note: Bridges and Endpoints may implement a corresponding command
>>   bit which may be set by system software to indicate the RCB value
>>   for the Root Complex, allowing the Bridge/Endpoint to optimize its
>>   behavior when the Root Complex’s RCB is 128 bytes.
>> o For all other system elements, RCB is 128 bytes
>>
>> In this particular case the _HPX method was causing the RCB for all PCI
>> devices to be set to 128 bytes, while the root bridge remained at 64 bytes.
>> While this is arguably a BIOS bug, earlier linux version (ie without the
>> mentioned patch) were running fine, so this is actually a regression.
> 
> Thanks!  I can fold this into the changelog.
> 
> I assume you didn't mention a bugzilla or similar URL because this was
> found internally?  I'd still like a clue about what this issue looks
> like to a user, because that helps connect future problem reports with
> this fix.
> 
We do have a bugzilla report, but as this is a) on the SUSE internal
bugzilla and b) a customer issue I didn't include a reference here.

However, the symptoms are:

[    8.648872] mlx4_core: Mellanox ConnectX core driver v2.2-1 (Feb, 2014)
[    8.648889] mlx4_core: Initializing 0000:41:00.0
[   10.068642] mlx4_core 0000:41:00.0: command 0xfff failed: fw status = 0x1
[   10.068645] mlx4_core 0000:41:00.0: MAP_FA command failed, aborting
[   10.068659] mlx4_core 0000:41:00.0: Failed to start FW, aborting
[   10.068661] mlx4_core 0000:41:00.0: Failed to init fw, aborting.
[   11.071536] mlx4_core: probe of 0000:41:00.0 failed with error -5

> And I suppose that since 7a1562d4f2d0 appeared in v3.18, we maybe
> should consider marking the fix for stable?
> 
Yes, please.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH] pci: Don't set RCB bit in LNKCTL if the upstream bridge hasn't

[Johannes Thumshirn]

On Thu, Oct 27, 2016 at 06:51:52AM -0500, Bjorn Helgaas wrote:
> On Thu, Oct 27, 2016 at 07:42:27AM +0200, Hannes Reinecke wrote:
> > On 10/26/2016 09:43 PM, Bjorn Helgaas wrote:
> > > Hi Johannes,
> > > 
> > > On Wed, Oct 26, 2016 at 03:53:34PM +0200, Johannes Thumshirn wrote:
> > >> The Read Completion Boundary bit must only be set on a device or endpoint if
> > >> it is set on the upstream bridge.
> > >>
> > >> Fixes: 7a1562d4f ("PCI: Apply _HPX Link Control settings to all devices with a link")
> > > 
> > > Can you please include a spec citation and a pointer to the bug report?
> > > 
> > PCI Express Base Specification 1.1,
> > section 2.3.1.1. Data Return for Read Requests:
> > 
> > The Read Completion Boundary (RCB) parameter determines the naturally
> > aligned address boundaries on which a Read Request may be serviced with
> > multiple Completions
> > o For a Root Complex, RCB is 64 bytes or 128 bytes
> >   o This value is reported through a configuration register
> >     (see Section 7.8)
> >   Note: Bridges and Endpoints may implement a corresponding command
> >   bit which may be set by system software to indicate the RCB value
> >   for the Root Complex, allowing the Bridge/Endpoint to optimize its
> >   behavior when the Root Complex’s RCB is 128 bytes.
> > o For all other system elements, RCB is 128 bytes
> > 
> > In this particular case the _HPX method was causing the RCB for all PCI
> > devices to be set to 128 bytes, while the root bridge remained at 64 bytes.
> > While this is arguably a BIOS bug, earlier linux version (ie without the
> > mentioned patch) were running fine, so this is actually a regression.
> 
> Thanks!  I can fold this into the changelog.
> 
> I assume you didn't mention a bugzilla or similar URL because this was
> found internally?  I'd still like a clue about what this issue looks
> like to a user, because that helps connect future problem reports with
> this fix.

Sharing the bugzilla number won't be of any help here, as it's not accessible.
The user visible issue was, that mlx4_core.ko wasn't able to issue the
MLX4_CMD_MAP_FA command to it's firmware resulting in the adapter undergoing
it's error recovery and ultimately hitting a BUG_ON() (which is subject to
another patch). Even without asserting it won't load and hence not provide any
network service. 

We haven't seen this behaviour on other drivers and it is likely a BIOS
error (the wrong setting of PCI_EXP_LNKCTL_RCB) but that wasn't an issue
before 7a1562d4f2d0 hence we do have a regression to kernels < 3.18.

> 
> And I suppose that since 7a1562d4f2d0 appeared in v3.18, we maybe
> should consider marking the fix for stable?

Yes probably, but on the other hand we haven't heard of any other cases than
this one (mlx4 NIC/HCA with a particular IBM IvyBridge Server, the next generation
didn't expose the bug).

Btw: Did you see I've sent a v2 as Alex Graf pointed out there might be a
possible NULL pointer dereference when accessing the "bridge" pointer in
pcie_get_upstream_rcb()?

Thanks,
	Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850