pcie_get_minimum_link returns 0 width

linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* pcie_get_minimum_link returns 0 width
       [not found] <20130825100157.GA16500@lb-tlvb-yuvalmin.il.broadcom.com>
@ 2013-08-25 11:21 ` Yuval Mintz
  2013-08-26 18:27   ` Keller, Jacob E
  2013-08-26 22:05   ` Bjorn Helgaas
  0 siblings, 2 replies; 16+ messages in thread
From: Yuval Mintz @ 2013-08-25 11:21 UTC (permalink / raw)
  To: jacob.e.keller@intel.com
  Cc: bhelgaas@google.com, linux-pci@vger.kernel.org,
	netdev@vger.kernel.org

Hi,

I tried adding support for the newly added 'pcie_get_minimum_link' into the
bnx2x driver, but found out the some of my devices started showing width x0.

By adding debug prints, I've found out there were devices up the chain that
Showed 0 when their PCI_EXP_LNKSTA was read by said function.
However, when I tried looking via lspci the output claimed the width was x4.

lspci -vt output:
[0000:00]-+-00.0  Intel Corporation 5000P Chipset Memory Controller Hub
	     +-02.0-[09-12]--+-00.0-[0a-11]--+-00.0-[0b-0d]--
					     +-01.0-[0e-10]--+-00.0  Broadcom
					Corporation NetXtreme II BCM57710
					10-Gigabit PCIe [Everest]

Where:
00:02.0 PCI bridge: Intel Corporation 5000 Series Chipset PCI Express x4 Port
2 (rev 93)
09:00.0 PCI bridge: Intel Corporation 6311ESB/6321ESB PCI Express Upstream
Port (rev 01)
0a:01.0 PCI bridge: Intel Corporation 6311ESB/6321ESB PCI Express
Downstream Port E2 (rev 01)
0e:00.0 Ethernet controller: Broadcom Corporation NetXtreme II BCM57710
10-Gigabit PCIe [Everest] (rev 01)

The output for "lspci -vvvv | grep LnkSta for all four is:
LnkSta:	Speed 2.5GT/s, Width x4, TrErr- Train- SlotClk+ DLActive- BWMgmt-
ABWMgmt-

But added prints inside the function's loop show:
LnkSta 1041 [000e:00.00]
LnkSta 0000 [000a:01.00]
LnkSta 0000 [0009:00.00]
LnkSta 3041 [0000:02.00]
(PCI_EXP_LNKSTA value, bus->number, PCI_SLOT, PCI_FUNC)

Thanks,
Yuval


^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: pcie_get_minimum_link returns 0 width
  2013-08-25 11:21 ` pcie_get_minimum_link returns 0 width Yuval Mintz
@ 2013-08-26 18:27   ` Keller, Jacob E
  2013-08-26 22:05   ` Bjorn Helgaas
  1 sibling, 0 replies; 16+ messages in thread
From: Keller, Jacob E @ 2013-08-26 18:27 UTC (permalink / raw)
  To: Yuval Mintz
  Cc: bhelgaas@google.com, linux-pci@vger.kernel.org,
	netdev@vger.kernel.org

> -----Original Message-----
> From: Yuval Mintz [mailto:yuvalmin@broadcom.com]
> Sent: Sunday, August 25, 2013 4:22 AM
> To: Keller, Jacob E
> Cc: bhelgaas@google.com; linux-pci@vger.kernel.org;
> netdev@vger.kernel.org
> Subject: pcie_get_minimum_link returns 0 width
> 
> Hi,
> 
> I tried adding support for the newly added 'pcie_get_minimum_link' into
> the
> bnx2x driver, but found out the some of my devices started showing
> width x0.
> 
> By adding debug prints, I've found out there were devices up the chain
> that
> Showed 0 when their PCI_EXP_LNKSTA was read by said function.
> However, when I tried looking via lspci the output claimed the width was
> x4.
> 
> lspci -vt output:
> [0000:00]-+-00.0  Intel Corporation 5000P Chipset Memory Controller
> Hub
> 	     +-02.0-[09-12]--+-00.0-[0a-11]--+-00.0-[0b-0d]--
> 					     +-01.0-[0e-10]--+-00.0
> Broadcom
> 					Corporation NetXtreme II
> BCM57710
> 					10-Gigabit PCIe [Everest]
> 
> Where:
> 00:02.0 PCI bridge: Intel Corporation 5000 Series Chipset PCI Express x4
> Port
> 2 (rev 93)
> 09:00.0 PCI bridge: Intel Corporation 6311ESB/6321ESB PCI Express
> Upstream
> Port (rev 01)
> 0a:01.0 PCI bridge: Intel Corporation 6311ESB/6321ESB PCI Express
> Downstream Port E2 (rev 01)
> 0e:00.0 Ethernet controller: Broadcom Corporation NetXtreme II
> BCM57710
> 10-Gigabit PCIe [Everest] (rev 01)
> 
> The output for "lspci -vvvv | grep LnkSta for all four is:
> LnkSta:	Speed 2.5GT/s, Width x4, TrErr- Train- SlotClk+ DLActive-
> BWMgmt-
> ABWMgmt-
> 
> But added prints inside the function's loop show:
> LnkSta 1041 [000e:00.00]
> LnkSta 0000 [000a:01.00]
> LnkSta 0000 [0009:00.00]
> LnkSta 3041 [0000:02.00]
> (PCI_EXP_LNKSTA value, bus->number, PCI_SLOT, PCI_FUNC)
> 
> Thanks,
> Yuval

Interesting... It looks like the entire LnkSta  read failed for the two in the middle..  I don't know how much I can help on this issue because I don't have a machine that exhibits this symptom.. Any suggestions?

Regards,
Jake

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: pcie_get_minimum_link returns 0 width
  2013-08-25 11:21 ` pcie_get_minimum_link returns 0 width Yuval Mintz
  2013-08-26 18:27   ` Keller, Jacob E
@ 2013-08-26 22:05   ` Bjorn Helgaas
  2013-08-26 23:36     ` Yuval Mintz
  1 sibling, 1 reply; 16+ messages in thread
From: Bjorn Helgaas @ 2013-08-26 22:05 UTC (permalink / raw)
  To: Yuval Mintz
  Cc: jacob.e.keller@intel.com, linux-pci@vger.kernel.org,
	netdev@vger.kernel.org

On Sun, Aug 25, 2013 at 5:21 AM, Yuval Mintz <yuvalmin@broadcom.com> wrote:
> Hi,
>
> I tried adding support for the newly added 'pcie_get_minimum_link' into the
> bnx2x driver, but found out the some of my devices started showing width x0.
>
> By adding debug prints, I've found out there were devices up the chain that
> Showed 0 when their PCI_EXP_LNKSTA was read by said function.
> However, when I tried looking via lspci the output claimed the width was x4.

I don't see a 'pcie_get_minimum_link()' function in my current tree
(git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git "next"
branch).  Maybe seeing your patch (with the debug prints) would give
me a hook for somewhere to start looking.

Can you figure out why lspci shows the correct value and this kernel
interface does not?  The current lspci source is in
git://git.kernel.org/pub/scm/utils/pciutils/pciutils.git and the
relevant code is probably in cap_express_link() here:
http://git.kernel.org/cgit/utils/pciutils/pciutils.git/tree/ls-caps.c#n750

> lspci -vt output:
> [0000:00]-+-00.0  Intel Corporation 5000P Chipset Memory Controller Hub
>              +-02.0-[09-12]--+-00.0-[0a-11]--+-00.0-[0b-0d]--
>                                              +-01.0-[0e-10]--+-00.0  Broadcom
>                                         Corporation NetXtreme II BCM57710
>                                         10-Gigabit PCIe [Everest]
>
> Where:
> 00:02.0 PCI bridge: Intel Corporation 5000 Series Chipset PCI Express x4 Port
> 2 (rev 93)
> 09:00.0 PCI bridge: Intel Corporation 6311ESB/6321ESB PCI Express Upstream
> Port (rev 01)
> 0a:01.0 PCI bridge: Intel Corporation 6311ESB/6321ESB PCI Express
> Downstream Port E2 (rev 01)
> 0e:00.0 Ethernet controller: Broadcom Corporation NetXtreme II BCM57710
> 10-Gigabit PCIe [Everest] (rev 01)
>
> The output for "lspci -vvvv | grep LnkSta for all four is:
> LnkSta: Speed 2.5GT/s, Width x4, TrErr- Train- SlotClk+ DLActive- BWMgmt-
> ABWMgmt-
>
> But added prints inside the function's loop show:
> LnkSta 1041 [000e:00.00]
> LnkSta 0000 [000a:01.00]
> LnkSta 0000 [0009:00.00]
> LnkSta 3041 [0000:02.00]
> (PCI_EXP_LNKSTA value, bus->number, PCI_SLOT, PCI_FUNC)
>
> Thanks,
> Yuval
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: pcie_get_minimum_link returns 0 width
  2013-08-26 22:05   ` Bjorn Helgaas
@ 2013-08-26 23:36     ` Yuval Mintz
  2013-08-26 23:57       ` Bjorn Helgaas
  0 siblings, 1 reply; 16+ messages in thread
From: Yuval Mintz @ 2013-08-26 23:36 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: jacob.e.keller@intel.com, linux-pci@vger.kernel.org,
	netdev@vger.kernel.org

> > Hi,
> >
> > I tried adding support for the newly added 'pcie_get_minimum_link' into
> the
> > bnx2x driver, but found out the some of my devices started showing width
> x0.
> >
> > By adding debug prints, I've found out there were devices up the chain that
> > Showed 0 when their PCI_EXP_LNKSTA was read by said function.
> > However, when I tried looking via lspci the output claimed the width was
> x4.
> 
> I don't see a 'pcie_get_minimum_link()' function in my current tree
> (git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git "next"
> branch).  Maybe seeing your patch (with the debug prints) would give
> me a hook for somewhere to start looking.

You've acked the patch and it was applied in net-next; Obviously it has yet
to migrate to your tree. 

> 
> Can you figure out why lspci shows the correct value and this kernel
> interface does not?  The current lspci source is in
> git://git.kernel.org/pub/scm/utils/pciutils/pciutils.git and the
> relevant code is probably in cap_express_link() here:
> http://git.kernel.org/cgit/utils/pciutils/pciutils.git/tree/ls-caps.c#n750

Traced the read as going via the pci-sysfs. Where should I look for the bus's
read ops?

Thanks,
Yuval


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: pcie_get_minimum_link returns 0 width
  2013-08-26 23:36     ` Yuval Mintz
@ 2013-08-26 23:57       ` Bjorn Helgaas
  2013-08-27  7:40         ` Yuval Mintz
  0 siblings, 1 reply; 16+ messages in thread
From: Bjorn Helgaas @ 2013-08-26 23:57 UTC (permalink / raw)
  To: Yuval Mintz
  Cc: jacob.e.keller@intel.com, linux-pci@vger.kernel.org,
	netdev@vger.kernel.org

On Mon, Aug 26, 2013 at 5:36 PM, Yuval Mintz <yuvalmin@broadcom.com> wrote:
>> > Hi,
>> >
>> > I tried adding support for the newly added 'pcie_get_minimum_link' into
>> the
>> > bnx2x driver, but found out the some of my devices started showing width
>> x0.
>> >
>> > By adding debug prints, I've found out there were devices up the chain that
>> > Showed 0 when their PCI_EXP_LNKSTA was read by said function.
>> > However, when I tried looking via lspci the output claimed the width was
>> x4.
>>
>> I don't see a 'pcie_get_minimum_link()' function in my current tree
>> (git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git "next"
>> branch).  Maybe seeing your patch (with the debug prints) would give
>> me a hook for somewhere to start looking.
>
> You've acked the patch and it was applied in net-next; Obviously it has yet
> to migrate to your tree.

Oh, yeah, that's right.  I knew it sounded familiar, but I didn't
remember where it went.

Looking at its implementation, one obvious difference is that
pcie_get_minimum_link() traverses up the hierarchy and keeps track of
the minimum values it finds.  lspci, on the other hand, just reads
PCI_EXP_LNKSTA from a single device and decodes it.

You said lspci reports x4 for every device from the Root Port all the
way down to your NIC, so I would think the minimum from
pcie_get_minimum_link() would be x4.  But apparently it's not.  Maybe
there's a bug in it.  Can you post the complete "lspci -vv" output
along with your debug patch and output?

Bjorn

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: pcie_get_minimum_link returns 0 width
  2013-08-26 23:57       ` Bjorn Helgaas
@ 2013-08-27  7:40         ` Yuval Mintz
  2013-08-27 13:57           ` Bjorn Helgaas
  0 siblings, 1 reply; 16+ messages in thread
From: Yuval Mintz @ 2013-08-27  7:40 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: jacob.e.keller@intel.com, linux-pci@vger.kernel.org,
	netdev@vger.kernel.org

> On Mon, Aug 26, 2013 at 5:36 PM, Yuval Mintz <yuvalmin@broadcom.com>
> wrote:
> >> > Hi,
> >> >
> >> > I tried adding support for the newly added 'pcie_get_minimum_link' into
> >> the
> >> > bnx2x driver, but found out the some of my devices started showing
> width
> >> x0.
> >> >
> >> > By adding debug prints, I've found out there were devices up the chain
> that
> >> > Showed 0 when their PCI_EXP_LNKSTA was read by said function.
> >> > However, when I tried looking via lspci the output claimed the width was
> >> x4.
> Looking at its implementation, one obvious difference is that
> pcie_get_minimum_link() traverses up the hierarchy and keeps track of
> the minimum values it finds.  lspci, on the other hand, just reads
> PCI_EXP_LNKSTA from a single device and decodes it.
> 
> You said lspci reports x4 for every device from the Root Port all the
> way down to your NIC, so I would think the minimum from
> pcie_get_minimum_link() would be x4.  But apparently it's not.  Maybe
> there's a bug in it.  Can you post the complete "lspci -vv" output
> along with your debug patch and output?
> 
> Bjorn

Here's the patch:
---
 drivers/pci/pci.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c71e78c..72cb87b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3601,13 +3601,19 @@ int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed,
 		enum pcie_link_width next_width;
 
 		ret = pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
-		if (ret)
+		if (ret) {
+			printk(KERN_ERR "Failed to Read LNKSTA\n");
 			return ret;
+		}
 
 		next_speed = pcie_link_speed[lnksta & PCI_EXP_LNKSTA_CLS];
 		next_width = (lnksta & PCI_EXP_LNKSTA_NLW) >>
 			PCI_EXP_LNKSTA_NLW_SHIFT;
 
+		printk(KERN_ERR "LnkSta %04x [%04x:%02x.%02x]\n",
+		       lnksta, dev->bus->number, PCI_SLOT(dev->devfn),
+		       PCI_FUNC(dev->devfn));
+
 		if (next_speed < *speed)
 			*speed = next_speed;
 
-- 
1.7.1

And here are the lscpi results for 09:00.0 and 0a:01.0
(The two devices for which the loop returns 0 width):

Sysfs do read [Pos 0, len 64]
09:00.0 Class 0604: Device 8086:3500 (rev 01)
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr+ Stepping- SERR+ FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0, Cache Line Size: 64 bytes
	Bus: primary=09, secondary=0a, subordinate=11, sec-latency=0
	Memory behind bridge: fb800000-fd7fffff
	Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort+ <SERR- <PERR-
	BridgeCtl: Parity+ SERR+ NoISA- VGA- MAbort- >Reset- FastB2B-
		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
	Capabilities: Sysfs do read [Pos 68, len 4]
[44] Express (v1) Upstream Port, MSI 00
Sysfs do read [Pos 72, len 16]
		DevCap:	MaxPayload 256 bytes, PhantFunc 0, Latency L0s <64ns, L1 <1us
			ExtTag- AttnBtn- AttnInd- PwrInd- RBE- FLReset-SlotPowerLimit 0.000W
		DevCtl:	Report errors: Correctable- Non-Fatal+ Fatal+ Unsupported-
			RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
			MaxPayload 256 bytes, MaxReadReq 4096 bytes
		DevSta:	CorrErr- UncorrErr- FatalErr+ UnsuppReq+ AuxPwr- TransPend-
		LnkCap:	Port #0, Speed 2.5GT/s, Width x8, ASPM L0s, Latency L0 unlimited, L1 unlimited
			ClockPM- Surprise- LLActRep- BwNot-
		LnkCtl:	ASPM Disabled; Disabled- Retrain- CommClk-
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 2.5GT/s, Width x4, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
	Capabilities: Sysfs do read [Pos 112, len 4]
[70] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
Sysfs do read [Pos 116, len 4]
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: Sysfs do read [Pos 128, len 4]
[80] Sysfs do read [Pos 132, len 4]
Subsystem: Device 0000:0000
Sysfs do read [Pos 256, len 4]
	Capabilities: [100 v1] Advanced Error Reporting
Sysfs do read [Pos 260, len 24]
		UESta:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq+ ACSViol-
		UEMsk:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq+ ACSViol-
		UESvrt:	DLP+ SDES- TLP+ FCP+ CmpltTO+ CmpltAbrt+ UnxCmplt+ RxOF+ MalfTLP+ ECRC- UnsupReq+ ACSViol-
		CESta:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr-
		CEMsk:	RxErr+ BadTLP+ BadDLLP+ Rollover+ Timeout+ NonFatalErr-
		AERCap:	First Error Pointer: 14, GenCap- CGenEn- ChkCap- ChkEn-
	Kernel driver in use: pcieport
	Kernel modules: shpchp

Sysfs do read [Pos 0, len 64]
0a:01.0 Class 0604: Device 8086:3514 (rev 01)
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr+ Stepping- SERR+ FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0, Cache Line Size: 64 bytes
	Bus: primary=0a, secondary=0e, subordinate=10, sec-latency=0
	Memory behind bridge: fb800000-fd7fffff
	Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort+ <SERR- <PERR-
	BridgeCtl: Parity+ SERR+ NoISA- VGA- MAbort- >Reset- FastB2B-
		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
	Capabilities: Sysfs do read [Pos 68, len 4]
[44] Express (v1) Downstream Port (Slot-), MSI 00
Sysfs do read [Pos 72, len 16]
		DevCap:	MaxPayload 256 bytes, PhantFunc 0, Latency L0s <64ns, L1 <1us
			ExtTag- RBE- FLReset-
		DevCtl:	Report errors: Correctable- Non-Fatal+ Fatal+ Unsupported-
			RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
			MaxPayload 256 bytes, MaxReadReq 4096 bytes
		DevSta:	CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr- TransPend-
		LnkCap:	Port #2, Speed 2.5GT/s, Width x4, ASPM L0s, Latency L0 unlimited, L1 unlimited
			ClockPM- Surprise- LLActRep- BwNot-
		LnkCtl:	ASPM Disabled; Disabled- Retrain- CommClk-
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 2.5GT/s, Width x4, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
	Capabilities: Sysfs do read [Pos 96, len 4]
[60] MSI: Enable- Count=1/1 Maskable- 64bit+
Sysfs do read [Pos 100, len 10]
		Address: 0000000000000000  Data: 0000
	Capabilities: Sysfs do read [Pos 112, len 4]
[70] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
Sysfs do read [Pos 116, len 4]
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: Sysfs do read [Pos 128, len 4]
[80] Sysfs do read [Pos 132, len 4]
Subsystem: Device 0000:0000
Sysfs do read [Pos 256, len 4]
	Capabilities: [100 v1] Advanced Error Reporting
Sysfs do read [Pos 260, len 24]
		UESta:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
		UEMsk:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq+ ACSViol-
		UESvrt:	DLP+ SDES- TLP+ FCP+ CmpltTO+ CmpltAbrt+ UnxCmplt+ RxOF+ MalfTLP+ ECRC- UnsupReq+ ACSViol-
		CESta:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr-
		CEMsk:	RxErr+ BadTLP+ BadDLLP+ Rollover+ Timeout+ NonFatalErr-
		AERCap:	First Error Pointer: 00, GenCap- CGenEn- ChkCap- ChkEn-
	Kernel driver in use: pcieport
	Kernel modules: shpchp

Thanks,
Yuval


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: pcie_get_minimum_link returns 0 width
  2013-08-27  7:40         ` Yuval Mintz
@ 2013-08-27 13:57           ` Bjorn Helgaas
  2013-08-27 16:13             ` Bjorn Helgaas
  2013-08-27 16:44             ` [PATCH] PCI: correctly check availability of PCIe Link Cap/Status/Control registers Jiang Liu
  0 siblings, 2 replies; 16+ messages in thread
From: Bjorn Helgaas @ 2013-08-27 13:57 UTC (permalink / raw)
  To: Yuval Mintz
  Cc: jacob.e.keller@intel.com, linux-pci@vger.kernel.org,
	netdev@vger.kernel.org, Jiang Liu

[+cc Jiang]

On Tue, Aug 27, 2013 at 1:40 AM, Yuval Mintz <yuvalmin@broadcom.com> wrote:
>> On Mon, Aug 26, 2013 at 5:36 PM, Yuval Mintz <yuvalmin@broadcom.com>
>> wrote:
>> >> > Hi,
>> >> >
>> >> > I tried adding support for the newly added 'pcie_get_minimum_link' into
>> >> the
>> >> > bnx2x driver, but found out the some of my devices started showing
>> width
>> >> x0.
>> >> >
>> >> > By adding debug prints, I've found out there were devices up the chain
>> that
>> >> > Showed 0 when their PCI_EXP_LNKSTA was read by said function.
>> >> > However, when I tried looking via lspci the output claimed the width was
>> >> x4.
>> Looking at its implementation, one obvious difference is that
>> pcie_get_minimum_link() traverses up the hierarchy and keeps track of
>> the minimum values it finds.  lspci, on the other hand, just reads
>> PCI_EXP_LNKSTA from a single device and decodes it.
>>
>> You said lspci reports x4 for every device from the Root Port all the
>> way down to your NIC, so I would think the minimum from
>> pcie_get_minimum_link() would be x4.  But apparently it's not.  Maybe
>> there's a bug in it.  Can you post the complete "lspci -vv" output
>> along with your debug patch and output?
>>
>> Bjorn
>
> Here's the patch:
> ---
>  drivers/pci/pci.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index c71e78c..72cb87b 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3601,13 +3601,19 @@ int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed,
>                 enum pcie_link_width next_width;
>
>                 ret = pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
> -               if (ret)
> +               if (ret) {
> +                       printk(KERN_ERR "Failed to Read LNKSTA\n");
>                         return ret;
> +               }
>
>                 next_speed = pcie_link_speed[lnksta & PCI_EXP_LNKSTA_CLS];
>                 next_width = (lnksta & PCI_EXP_LNKSTA_NLW) >>
>                         PCI_EXP_LNKSTA_NLW_SHIFT;
>
> +               printk(KERN_ERR "LnkSta %04x [%04x:%02x.%02x]\n",
> +                      lnksta, dev->bus->number, PCI_SLOT(dev->devfn),
> +                      PCI_FUNC(dev->devfn));

I think pcie_cap_has_lnkctl() is incorrect: it currently thinks only
Root Ports, Endpoints, and Legacy Endpoints have link registers.  But
I think switch ports (Upstream Ports and Downstream Ports) also have
link registers (PCIe spec r3.0, sec 7.8).  Jiang?

>                 if (next_speed < *speed)
>                         *speed = next_speed;
>
> --
> 1.7.1
>
> And here are the lscpi results for 09:00.0 and 0a:01.0
> (The two devices for which the loop returns 0 width):
>
> Sysfs do read [Pos 0, len 64]
> 09:00.0 Class 0604: Device 8086:3500 (rev 01)
>         Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr+ Stepping- SERR+ FastB2B- DisINTx-
>         Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
>         Latency: 0, Cache Line Size: 64 bytes
>         Bus: primary=09, secondary=0a, subordinate=11, sec-latency=0
>         Memory behind bridge: fb800000-fd7fffff
>         Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort+ <SERR- <PERR-
>         BridgeCtl: Parity+ SERR+ NoISA- VGA- MAbort- >Reset- FastB2B-
>                 PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
>         Capabilities: Sysfs do read [Pos 68, len 4]
> [44] Express (v1) Upstream Port, MSI 00
> Sysfs do read [Pos 72, len 16]
>                 DevCap: MaxPayload 256 bytes, PhantFunc 0, Latency L0s <64ns, L1 <1us
>                         ExtTag- AttnBtn- AttnInd- PwrInd- RBE- FLReset-SlotPowerLimit 0.000W
>                 DevCtl: Report errors: Correctable- Non-Fatal+ Fatal+ Unsupported-
>                         RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
>                         MaxPayload 256 bytes, MaxReadReq 4096 bytes
>                 DevSta: CorrErr- UncorrErr- FatalErr+ UnsuppReq+ AuxPwr- TransPend-
>                 LnkCap: Port #0, Speed 2.5GT/s, Width x8, ASPM L0s, Latency L0 unlimited, L1 unlimited
>                         ClockPM- Surprise- LLActRep- BwNot-
>                 LnkCtl: ASPM Disabled; Disabled- Retrain- CommClk-
>                         ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
>                 LnkSta: Speed 2.5GT/s, Width x4, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
>         Capabilities: Sysfs do read [Pos 112, len 4]
> [70] Power Management version 2
>                 Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
> Sysfs do read [Pos 116, len 4]
>                 Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
>         Capabilities: Sysfs do read [Pos 128, len 4]
> [80] Sysfs do read [Pos 132, len 4]
> Subsystem: Device 0000:0000
> Sysfs do read [Pos 256, len 4]
>         Capabilities: [100 v1] Advanced Error Reporting
> Sysfs do read [Pos 260, len 24]
>                 UESta:  DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq+ ACSViol-
>                 UEMsk:  DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq+ ACSViol-
>                 UESvrt: DLP+ SDES- TLP+ FCP+ CmpltTO+ CmpltAbrt+ UnxCmplt+ RxOF+ MalfTLP+ ECRC- UnsupReq+ ACSViol-
>                 CESta:  RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr-
>                 CEMsk:  RxErr+ BadTLP+ BadDLLP+ Rollover+ Timeout+ NonFatalErr-
>                 AERCap: First Error Pointer: 14, GenCap- CGenEn- ChkCap- ChkEn-
>         Kernel driver in use: pcieport
>         Kernel modules: shpchp
>
> Sysfs do read [Pos 0, len 64]
> 0a:01.0 Class 0604: Device 8086:3514 (rev 01)
>         Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr+ Stepping- SERR+ FastB2B- DisINTx-
>         Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
>         Latency: 0, Cache Line Size: 64 bytes
>         Bus: primary=0a, secondary=0e, subordinate=10, sec-latency=0
>         Memory behind bridge: fb800000-fd7fffff
>         Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort+ <SERR- <PERR-
>         BridgeCtl: Parity+ SERR+ NoISA- VGA- MAbort- >Reset- FastB2B-
>                 PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
>         Capabilities: Sysfs do read [Pos 68, len 4]
> [44] Express (v1) Downstream Port (Slot-), MSI 00
> Sysfs do read [Pos 72, len 16]
>                 DevCap: MaxPayload 256 bytes, PhantFunc 0, Latency L0s <64ns, L1 <1us
>                         ExtTag- RBE- FLReset-
>                 DevCtl: Report errors: Correctable- Non-Fatal+ Fatal+ Unsupported-
>                         RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
>                         MaxPayload 256 bytes, MaxReadReq 4096 bytes
>                 DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr- TransPend-
>                 LnkCap: Port #2, Speed 2.5GT/s, Width x4, ASPM L0s, Latency L0 unlimited, L1 unlimited
>                         ClockPM- Surprise- LLActRep- BwNot-
>                 LnkCtl: ASPM Disabled; Disabled- Retrain- CommClk-
>                         ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
>                 LnkSta: Speed 2.5GT/s, Width x4, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
>         Capabilities: Sysfs do read [Pos 96, len 4]
> [60] MSI: Enable- Count=1/1 Maskable- 64bit+
> Sysfs do read [Pos 100, len 10]
>                 Address: 0000000000000000  Data: 0000
>         Capabilities: Sysfs do read [Pos 112, len 4]
> [70] Power Management version 2
>                 Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
> Sysfs do read [Pos 116, len 4]
>                 Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
>         Capabilities: Sysfs do read [Pos 128, len 4]
> [80] Sysfs do read [Pos 132, len 4]
> Subsystem: Device 0000:0000
> Sysfs do read [Pos 256, len 4]
>         Capabilities: [100 v1] Advanced Error Reporting
> Sysfs do read [Pos 260, len 24]
>                 UESta:  DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
>                 UEMsk:  DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq+ ACSViol-
>                 UESvrt: DLP+ SDES- TLP+ FCP+ CmpltTO+ CmpltAbrt+ UnxCmplt+ RxOF+ MalfTLP+ ECRC- UnsupReq+ ACSViol-
>                 CESta:  RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr-
>                 CEMsk:  RxErr+ BadTLP+ BadDLLP+ Rollover+ Timeout+ NonFatalErr-
>                 AERCap: First Error Pointer: 00, GenCap- CGenEn- ChkCap- ChkEn-
>         Kernel driver in use: pcieport
>         Kernel modules: shpchp
>
> Thanks,
> Yuval
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: pcie_get_minimum_link returns 0 width
  2013-08-27 13:57           ` Bjorn Helgaas
@ 2013-08-27 16:13             ` Bjorn Helgaas
  2013-08-27 16:44             ` [PATCH] PCI: correctly check availability of PCIe Link Cap/Status/Control registers Jiang Liu
  1 sibling, 0 replies; 16+ messages in thread
From: Bjorn Helgaas @ 2013-08-27 16:13 UTC (permalink / raw)
  To: Yuval Mintz
  Cc: jacob.e.keller@intel.com, linux-pci@vger.kernel.org,
	netdev@vger.kernel.org, Jiang Liu

On Tue, Aug 27, 2013 at 07:57:20AM -0600, Bjorn Helgaas wrote:
> [+cc Jiang]
> 
> On Tue, Aug 27, 2013 at 1:40 AM, Yuval Mintz <yuvalmin@broadcom.com> wrote:
> >> On Mon, Aug 26, 2013 at 5:36 PM, Yuval Mintz <yuvalmin@broadcom.com>
> >> wrote:
> >> >> > Hi,
> >> >> >
> >> >> > I tried adding support for the newly added 'pcie_get_minimum_link' into
> >> >> the
> >> >> > bnx2x driver, but found out the some of my devices started showing
> >> width
> >> >> x0.
> >> >> >
> >> >> > By adding debug prints, I've found out there were devices up the chain
> >> that
> >> >> > Showed 0 when their PCI_EXP_LNKSTA was read by said function.
> >> >> > However, when I tried looking via lspci the output claimed the width was
> >> >> x4.
> >> Looking at its implementation, one obvious difference is that
> >> pcie_get_minimum_link() traverses up the hierarchy and keeps track of
> >> the minimum values it finds.  lspci, on the other hand, just reads
> >> PCI_EXP_LNKSTA from a single device and decodes it.
> >>
> >> You said lspci reports x4 for every device from the Root Port all the
> >> way down to your NIC, so I would think the minimum from
> >> pcie_get_minimum_link() would be x4.  But apparently it's not.  Maybe
> >> there's a bug in it.  Can you post the complete "lspci -vv" output
> >> along with your debug patch and output?
> >>
> >> Bjorn
> >
> > Here's the patch:
> > ---
> >  drivers/pci/pci.c |    8 +++++++-
> >  1 files changed, 7 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index c71e78c..72cb87b 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -3601,13 +3601,19 @@ int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed,
> >                 enum pcie_link_width next_width;
> >
> >                 ret = pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
> > -               if (ret)
> > +               if (ret) {
> > +                       printk(KERN_ERR "Failed to Read LNKSTA\n");
> >                         return ret;
> > +               }
> >
> >                 next_speed = pcie_link_speed[lnksta & PCI_EXP_LNKSTA_CLS];
> >                 next_width = (lnksta & PCI_EXP_LNKSTA_NLW) >>
> >                         PCI_EXP_LNKSTA_NLW_SHIFT;
> >
> > +               printk(KERN_ERR "LnkSta %04x [%04x:%02x.%02x]\n",
> > +                      lnksta, dev->bus->number, PCI_SLOT(dev->devfn),
> > +                      PCI_FUNC(dev->devfn));
> 
> I think pcie_cap_has_lnkctl() is incorrect: it currently thinks only
> Root Ports, Endpoints, and Legacy Endpoints have link registers.  But
> I think switch ports (Upstream Ports and Downstream Ports) also have
> link registers (PCIe spec r3.0, sec 7.8).  Jiang?

Yuval, can you try the patch below?


PCI: Allow access to link-related registers for switch and bridge ports

From: Bjorn Helgaas <bhelgaas@google.com>

Every PCIe device has a link, except Root Complex Integrated Endpoints
and Root Complex Event Collectors.  Previously we didn't give access
to PCIe capability link-related registers for Upstream Ports, Downstream
Ports, and bridges, so attempts to read PCI_EXP_LNKCTL returned zero.
See PCIe spec r3.0, sec 7.8 and 1.3.2.3.

Reported-by: Yuval Mintz <yuvalmin@broadcom.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/access.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 1cc2366..46dd5ad 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -485,9 +485,8 @@ static inline bool pcie_cap_has_lnkctl(const struct pci_dev *dev)
 	int type = pci_pcie_type(dev);
 
 	return pcie_cap_version(dev) > 1 ||
-	       type == PCI_EXP_TYPE_ROOT_PORT ||
-	       type == PCI_EXP_TYPE_ENDPOINT ||
-	       type == PCI_EXP_TYPE_LEG_END;
+	       !(type == PCI_EXP_TYPE_RC_END ||
+	         type == PCI_EXP_TYPE_RC_EC);
 }
 
 static inline bool pcie_cap_has_sltctl(const struct pci_dev *dev)

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH] PCI: correctly check availability of PCIe Link Cap/Status/Control registers
  2013-08-27 13:57           ` Bjorn Helgaas
  2013-08-27 16:13             ` Bjorn Helgaas
@ 2013-08-27 16:44             ` Jiang Liu
  2013-08-27 17:07               ` Bjorn Helgaas
  1 sibling, 1 reply; 16+ messages in thread
From: Jiang Liu @ 2013-08-27 16:44 UTC (permalink / raw)
  To: Bjorn Helgaas, Yuval Mintz, jacob . e . keller @ intel . com,
	Yu Zhao
  Cc: liuj97, Jiang Liu, linux-pci @ vger . kernel . org

From: Jiang Liu <jiang.liu@huawei.com>

PCIe Base Spec 1.0 Section 5.8 states that "The PCI Express Capabilities,
Device Capabilities, Device Status/Control, Link Capabilities and
Link Status/Control registers are required for all PCI Express devices."

And PCIe Base Spec 3.0 Section 7.8 states that "The Link Capabilities,
Link Status, and Link Control registers are required for all Root Ports,
Switch Ports, Bridges, and Endpoints that are not Root Complex Integrated
Endpoints."

Changeset 1b6b8ce2ac372e "PCI: only save/restore existent registers in the
PCIe capability" introduces pcie_cap_has_lnkctl(), which assumes:
1) PCIe root port, endpoint and legacy endpoint with 1.0 PCIe Capability
   implement Link Cap/Status/Control registers.
2) All PCIe devices with 2.0 PCIe Capability implemnet Link Cap/Status/
   Control registers.

The above assumptions don't conform to PCIe base specifications, so change
pcie_cap_has_lnkctl() to follow PCIe specifications.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
Hi Bjorn,
	Yuval has a PCIe 1.0 swither, so triggers the bug.  I'm not sure
why Yu Zhao has implemented pcie_cap_has_linctl() in that way, any special
consideration here?

Hi Yuval,
	Could you please help to test this patch? Due to hardware resource
constraints, I have just compiled the patch on my laptop without testing.

Thanks!

---
 drivers/pci/access.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 1cc2366..23ff366 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -484,10 +484,14 @@ static inline bool pcie_cap_has_lnkctl(const struct pci_dev *dev)
 {
 	int type = pci_pcie_type(dev);
 
-	return pcie_cap_version(dev) > 1 ||
-	       type == PCI_EXP_TYPE_ROOT_PORT ||
-	       type == PCI_EXP_TYPE_ENDPOINT ||
-	       type == PCI_EXP_TYPE_LEG_END;
+	return	pcie_cap_version(dev) == 1 ||
+		type == PCI_EXP_TYPE_ENDPOINT ||
+		type == PCI_EXP_TYPE_LEG_END ||
+		type == PCI_EXP_TYPE_ROOT_PORT ||
+		type == PCI_EXP_TYPE_UPSTREAM ||
+		type == PCI_EXP_TYPE_DOWNSTREAM ||
+		type == PCI_EXP_TYPE_PCI_BRIDGE ||
+		type == PCI_EXP_TYPE_PCIE_BRIDGE;
 }
 
 static inline bool pcie_cap_has_sltctl(const struct pci_dev *dev)
-- 
1.8.1.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] PCI: correctly check availability of PCIe Link Cap/Status/Control registers
  2013-08-27 16:44             ` [PATCH] PCI: correctly check availability of PCIe Link Cap/Status/Control registers Jiang Liu
@ 2013-08-27 17:07               ` Bjorn Helgaas
  2013-08-27 18:02                 ` Keller, Jacob E
  2013-08-28  0:08                 ` Jiang Liu
  0 siblings, 2 replies; 16+ messages in thread
From: Bjorn Helgaas @ 2013-08-27 17:07 UTC (permalink / raw)
  To: Jiang Liu
  Cc: Yuval Mintz, jacob . e . keller @ intel . com, Yu Zhao, Jiang Liu,
	linux-pci @ vger . kernel . org

On Tue, Aug 27, 2013 at 10:44 AM, Jiang Liu <liuj97@gmail.com> wrote:
> From: Jiang Liu <jiang.liu@huawei.com>
>
> PCIe Base Spec 1.0 Section 5.8 states that "The PCI Express Capabilities,
> Device Capabilities, Device Status/Control, Link Capabilities and
> Link Status/Control registers are required for all PCI Express devices."

And spec 1.1 amended that (in sec 7.8, not 5.8) to say that the link
registers are not required for Root Complex Integrated Endpoints.
Integrated endpoints and event collectors don't have links, so even if
some RC-integrated devices do implement link registers per spec 1.0, I
don't think there's any point in allowing access to them.

> And PCIe Base Spec 3.0 Section 7.8 states that "The Link Capabilities,
> Link Status, and Link Control registers are required for all Root Ports,
> Switch Ports, Bridges, and Endpoints that are not Root Complex Integrated
> Endpoints."
>
> Changeset 1b6b8ce2ac372e "PCI: only save/restore existent registers in the
> PCIe capability" introduces pcie_cap_has_lnkctl(), which assumes:
> 1) PCIe root port, endpoint and legacy endpoint with 1.0 PCIe Capability
>    implement Link Cap/Status/Control registers.
> 2) All PCIe devices with 2.0 PCIe Capability implemnet Link Cap/Status/
>    Control registers.
>
> The above assumptions don't conform to PCIe base specifications, so change
> pcie_cap_has_lnkctl() to follow PCIe specifications.
>
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> ---
> Hi Bjorn,
>         Yuval has a PCIe 1.0 swither, so triggers the bug.  I'm not sure
> why Yu Zhao has implemented pcie_cap_has_linctl() in that way, any special
> consideration here?
>
> Hi Yuval,
>         Could you please help to test this patch? Due to hardware resource
> constraints, I have just compiled the patch on my laptop without testing.
>
> Thanks!
>
> ---
>  drivers/pci/access.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index 1cc2366..23ff366 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -484,10 +484,14 @@ static inline bool pcie_cap_has_lnkctl(const struct pci_dev *dev)
>  {
>         int type = pci_pcie_type(dev);
>
> -       return pcie_cap_version(dev) > 1 ||
> -              type == PCI_EXP_TYPE_ROOT_PORT ||
> -              type == PCI_EXP_TYPE_ENDPOINT ||
> -              type == PCI_EXP_TYPE_LEG_END;
> +       return  pcie_cap_version(dev) == 1 ||
> +               type == PCI_EXP_TYPE_ENDPOINT ||
> +               type == PCI_EXP_TYPE_LEG_END ||
> +               type == PCI_EXP_TYPE_ROOT_PORT ||
> +               type == PCI_EXP_TYPE_UPSTREAM ||
> +               type == PCI_EXP_TYPE_DOWNSTREAM ||
> +               type == PCI_EXP_TYPE_PCI_BRIDGE ||
> +               type == PCI_EXP_TYPE_PCIE_BRIDGE;
>  }
>
>  static inline bool pcie_cap_has_sltctl(const struct pci_dev *dev)
> --
> 1.8.1.2
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] PCI: correctly check availability of PCIe Link Cap/Status/Control registers
  2013-08-27 17:07               ` Bjorn Helgaas
@ 2013-08-27 18:02                 ` Keller, Jacob E
  2013-08-27 18:11                   ` Bjorn Helgaas
  2013-08-28  0:08                 ` Jiang Liu
  1 sibling, 1 reply; 16+ messages in thread
From: Keller, Jacob E @ 2013-08-27 18:02 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Jiang Liu, Yuval Mintz, Zhao, Yu, Jiang Liu,
	linux-pci @ vger . kernel . org

T24gVHVlLCAyMDEzLTA4LTI3IGF0IDExOjA3IC0wNjAwLCBCam9ybiBIZWxnYWFzIHdyb3RlOg0K
PiBPbiBUdWUsIEF1ZyAyNywgMjAxMyBhdCAxMDo0NCBBTSwgSmlhbmcgTGl1IDxsaXVqOTdAZ21h
aWwuY29tPiB3cm90ZToNCj4gPiBGcm9tOiBKaWFuZyBMaXUgPGppYW5nLmxpdUBodWF3ZWkuY29t
Pg0KPiA+DQo+ID4gUENJZSBCYXNlIFNwZWMgMS4wIFNlY3Rpb24gNS44IHN0YXRlcyB0aGF0ICJU
aGUgUENJIEV4cHJlc3MgQ2FwYWJpbGl0aWVzLA0KPiA+IERldmljZSBDYXBhYmlsaXRpZXMsIERl
dmljZSBTdGF0dXMvQ29udHJvbCwgTGluayBDYXBhYmlsaXRpZXMgYW5kDQo+ID4gTGluayBTdGF0
dXMvQ29udHJvbCByZWdpc3RlcnMgYXJlIHJlcXVpcmVkIGZvciBhbGwgUENJIEV4cHJlc3MgZGV2
aWNlcy4iDQo+IA0KPiBBbmQgc3BlYyAxLjEgYW1lbmRlZCB0aGF0IChpbiBzZWMgNy44LCBub3Qg
NS44KSB0byBzYXkgdGhhdCB0aGUgbGluaw0KPiByZWdpc3RlcnMgYXJlIG5vdCByZXF1aXJlZCBm
b3IgUm9vdCBDb21wbGV4IEludGVncmF0ZWQgRW5kcG9pbnRzLg0KPiBJbnRlZ3JhdGVkIGVuZHBv
aW50cyBhbmQgZXZlbnQgY29sbGVjdG9ycyBkb24ndCBoYXZlIGxpbmtzLCBzbyBldmVuIGlmDQo+
IHNvbWUgUkMtaW50ZWdyYXRlZCBkZXZpY2VzIGRvIGltcGxlbWVudCBsaW5rIHJlZ2lzdGVycyBw
ZXIgc3BlYyAxLjAsIEkNCj4gZG9uJ3QgdGhpbmsgdGhlcmUncyBhbnkgcG9pbnQgaW4gYWxsb3dp
bmcgYWNjZXNzIHRvIHRoZW0uDQo+IA0KDQpEb2VzIHRoaXMgbWVhbiB0aGF0IHBjaWVfZ2V0X21p
bmltdW1fbGluayBzaG91bGQgYmUgbW9kaWZpZWQgdG8gY2hlY2sgdG8NCm1ha2Ugc3VyZSB0aGUg
ZGV2aWNlIGhhcyBhIGxpbmtzdGF0dXMsIGFuZCBvbmx5IGtlZXAgdHJhY2sgb2YgdmFsdWVzIGZv
cg0Kd2hpY2ggdGhlcmUgaXMgYSBtaW5pbXVtPw0KDQpUaGFua3MsDQpKYWtlDQoNCj4gPiBBbmQg
UENJZSBCYXNlIFNwZWMgMy4wIFNlY3Rpb24gNy44IHN0YXRlcyB0aGF0ICJUaGUgTGluayBDYXBh
YmlsaXRpZXMsDQo+ID4gTGluayBTdGF0dXMsIGFuZCBMaW5rIENvbnRyb2wgcmVnaXN0ZXJzIGFy
ZSByZXF1aXJlZCBmb3IgYWxsIFJvb3QgUG9ydHMsDQo+ID4gU3dpdGNoIFBvcnRzLCBCcmlkZ2Vz
LCBhbmQgRW5kcG9pbnRzIHRoYXQgYXJlIG5vdCBSb290IENvbXBsZXggSW50ZWdyYXRlZA0KPiA+
IEVuZHBvaW50cy4iDQo+ID4NCj4gPiBDaGFuZ2VzZXQgMWI2YjhjZTJhYzM3MmUgIlBDSTogb25s
eSBzYXZlL3Jlc3RvcmUgZXhpc3RlbnQgcmVnaXN0ZXJzIGluIHRoZQ0KPiA+IFBDSWUgY2FwYWJp
bGl0eSIgaW50cm9kdWNlcyBwY2llX2NhcF9oYXNfbG5rY3RsKCksIHdoaWNoIGFzc3VtZXM6DQo+
ID4gMSkgUENJZSByb290IHBvcnQsIGVuZHBvaW50IGFuZCBsZWdhY3kgZW5kcG9pbnQgd2l0aCAx
LjAgUENJZSBDYXBhYmlsaXR5DQo+ID4gICAgaW1wbGVtZW50IExpbmsgQ2FwL1N0YXR1cy9Db250
cm9sIHJlZ2lzdGVycy4NCj4gPiAyKSBBbGwgUENJZSBkZXZpY2VzIHdpdGggMi4wIFBDSWUgQ2Fw
YWJpbGl0eSBpbXBsZW1uZXQgTGluayBDYXAvU3RhdHVzLw0KPiA+ICAgIENvbnRyb2wgcmVnaXN0
ZXJzLg0KPiA+DQo+ID4gVGhlIGFib3ZlIGFzc3VtcHRpb25zIGRvbid0IGNvbmZvcm0gdG8gUENJ
ZSBiYXNlIHNwZWNpZmljYXRpb25zLCBzbyBjaGFuZ2UNCj4gPiBwY2llX2NhcF9oYXNfbG5rY3Rs
KCkgdG8gZm9sbG93IFBDSWUgc3BlY2lmaWNhdGlvbnMuDQo+ID4NCj4gPiBTaWduZWQtb2ZmLWJ5
OiBKaWFuZyBMaXUgPGppYW5nLmxpdUBodWF3ZWkuY29tPg0KPiA+IC0tLQ0KPiA+IEhpIEJqb3Ju
LA0KPiA+ICAgICAgICAgWXV2YWwgaGFzIGEgUENJZSAxLjAgc3dpdGhlciwgc28gdHJpZ2dlcnMg
dGhlIGJ1Zy4gIEknbSBub3Qgc3VyZQ0KPiA+IHdoeSBZdSBaaGFvIGhhcyBpbXBsZW1lbnRlZCBw
Y2llX2NhcF9oYXNfbGluY3RsKCkgaW4gdGhhdCB3YXksIGFueSBzcGVjaWFsDQo+ID4gY29uc2lk
ZXJhdGlvbiBoZXJlPw0KPiA+DQo+ID4gSGkgWXV2YWwsDQo+ID4gICAgICAgICBDb3VsZCB5b3Ug
cGxlYXNlIGhlbHAgdG8gdGVzdCB0aGlzIHBhdGNoPyBEdWUgdG8gaGFyZHdhcmUgcmVzb3VyY2UN
Cj4gPiBjb25zdHJhaW50cywgSSBoYXZlIGp1c3QgY29tcGlsZWQgdGhlIHBhdGNoIG9uIG15IGxh
cHRvcCB3aXRob3V0IHRlc3RpbmcuDQo+ID4NCj4gPiBUaGFua3MhDQo+ID4NCj4gPiAtLS0NCj4g
PiAgZHJpdmVycy9wY2kvYWNjZXNzLmMgfCAxMiArKysrKysrKy0tLS0NCj4gPiAgMSBmaWxlIGNo
YW5nZWQsIDggaW5zZXJ0aW9ucygrKSwgNCBkZWxldGlvbnMoLSkNCj4gPg0KPiA+IGRpZmYgLS1n
aXQgYS9kcml2ZXJzL3BjaS9hY2Nlc3MuYyBiL2RyaXZlcnMvcGNpL2FjY2Vzcy5jDQo+ID4gaW5k
ZXggMWNjMjM2Ni4uMjNmZjM2NiAxMDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJzL3BjaS9hY2Nlc3Mu
Yw0KPiA+ICsrKyBiL2RyaXZlcnMvcGNpL2FjY2Vzcy5jDQo+ID4gQEAgLTQ4NCwxMCArNDg0LDE0
IEBAIHN0YXRpYyBpbmxpbmUgYm9vbCBwY2llX2NhcF9oYXNfbG5rY3RsKGNvbnN0IHN0cnVjdCBw
Y2lfZGV2ICpkZXYpDQo+ID4gIHsNCj4gPiAgICAgICAgIGludCB0eXBlID0gcGNpX3BjaWVfdHlw
ZShkZXYpOw0KPiA+DQo+ID4gLSAgICAgICByZXR1cm4gcGNpZV9jYXBfdmVyc2lvbihkZXYpID4g
MSB8fA0KPiA+IC0gICAgICAgICAgICAgIHR5cGUgPT0gUENJX0VYUF9UWVBFX1JPT1RfUE9SVCB8
fA0KPiA+IC0gICAgICAgICAgICAgIHR5cGUgPT0gUENJX0VYUF9UWVBFX0VORFBPSU5UIHx8DQo+
ID4gLSAgICAgICAgICAgICAgdHlwZSA9PSBQQ0lfRVhQX1RZUEVfTEVHX0VORDsNCj4gPiArICAg
ICAgIHJldHVybiAgcGNpZV9jYXBfdmVyc2lvbihkZXYpID09IDEgfHwNCj4gPiArICAgICAgICAg
ICAgICAgdHlwZSA9PSBQQ0lfRVhQX1RZUEVfRU5EUE9JTlQgfHwNCj4gPiArICAgICAgICAgICAg
ICAgdHlwZSA9PSBQQ0lfRVhQX1RZUEVfTEVHX0VORCB8fA0KPiA+ICsgICAgICAgICAgICAgICB0
eXBlID09IFBDSV9FWFBfVFlQRV9ST09UX1BPUlQgfHwNCj4gPiArICAgICAgICAgICAgICAgdHlw
ZSA9PSBQQ0lfRVhQX1RZUEVfVVBTVFJFQU0gfHwNCj4gPiArICAgICAgICAgICAgICAgdHlwZSA9
PSBQQ0lfRVhQX1RZUEVfRE9XTlNUUkVBTSB8fA0KPiA+ICsgICAgICAgICAgICAgICB0eXBlID09
IFBDSV9FWFBfVFlQRV9QQ0lfQlJJREdFIHx8DQo+ID4gKyAgICAgICAgICAgICAgIHR5cGUgPT0g
UENJX0VYUF9UWVBFX1BDSUVfQlJJREdFOw0KPiA+ICB9DQo+ID4NCj4gPiAgc3RhdGljIGlubGlu
ZSBib29sIHBjaWVfY2FwX2hhc19zbHRjdGwoY29uc3Qgc3RydWN0IHBjaV9kZXYgKmRldikNCj4g
PiAtLQ0KPiA+IDEuOC4xLjINCj4gPg0KDQoNCg==

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] PCI: correctly check availability of PCIe Link Cap/Status/Control registers
  2013-08-27 18:02                 ` Keller, Jacob E
@ 2013-08-27 18:11                   ` Bjorn Helgaas
  2013-08-27 22:28                     ` Yuval Mintz
  0 siblings, 1 reply; 16+ messages in thread
From: Bjorn Helgaas @ 2013-08-27 18:11 UTC (permalink / raw)
  To: Keller, Jacob E
  Cc: Jiang Liu, Yuval Mintz, Zhao, Yu, Jiang Liu,
	linux-pci @ vger . kernel . org

On Tue, Aug 27, 2013 at 12:02 PM, Keller, Jacob E
<jacob.e.keller@intel.com> wrote:
> On Tue, 2013-08-27 at 11:07 -0600, Bjorn Helgaas wrote:
>> On Tue, Aug 27, 2013 at 10:44 AM, Jiang Liu <liuj97@gmail.com> wrote:
>> > From: Jiang Liu <jiang.liu@huawei.com>
>> >
>> > PCIe Base Spec 1.0 Section 5.8 states that "The PCI Express Capabilities,
>> > Device Capabilities, Device Status/Control, Link Capabilities and
>> > Link Status/Control registers are required for all PCI Express devices."
>>
>> And spec 1.1 amended that (in sec 7.8, not 5.8) to say that the link
>> registers are not required for Root Complex Integrated Endpoints.
>> Integrated endpoints and event collectors don't have links, so even if
>> some RC-integrated devices do implement link registers per spec 1.0, I
>> don't think there's any point in allowing access to them.
>>
>
> Does this mean that pcie_get_minimum_link should be modified to check to
> make sure the device has a linkstatus, and only keep track of values for
> which there is a minimum?

That doesn't seem necessary to me.  pcie_get_minimum_link() is only
looking at bridge devices (Downstream Ports, Upstream Ports, and Root
Ports).  All of those are required to implement the Link Status
register, so I think checking would be unnecessary complication.

The RC-integrated devices can't be bridges, so pcie_get_minimum_link()
should never even iterate over any of them.

>> > And PCIe Base Spec 3.0 Section 7.8 states that "The Link Capabilities,
>> > Link Status, and Link Control registers are required for all Root Ports,
>> > Switch Ports, Bridges, and Endpoints that are not Root Complex Integrated
>> > Endpoints."
>> >
>> > Changeset 1b6b8ce2ac372e "PCI: only save/restore existent registers in the
>> > PCIe capability" introduces pcie_cap_has_lnkctl(), which assumes:
>> > 1) PCIe root port, endpoint and legacy endpoint with 1.0 PCIe Capability
>> >    implement Link Cap/Status/Control registers.
>> > 2) All PCIe devices with 2.0 PCIe Capability implemnet Link Cap/Status/
>> >    Control registers.
>> >
>> > The above assumptions don't conform to PCIe base specifications, so change
>> > pcie_cap_has_lnkctl() to follow PCIe specifications.
>> >
>> > Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>> > ---
>> > Hi Bjorn,
>> >         Yuval has a PCIe 1.0 swither, so triggers the bug.  I'm not sure
>> > why Yu Zhao has implemented pcie_cap_has_linctl() in that way, any special
>> > consideration here?
>> >
>> > Hi Yuval,
>> >         Could you please help to test this patch? Due to hardware resource
>> > constraints, I have just compiled the patch on my laptop without testing.
>> >
>> > Thanks!
>> >
>> > ---
>> >  drivers/pci/access.c | 12 ++++++++----
>> >  1 file changed, 8 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
>> > index 1cc2366..23ff366 100644
>> > --- a/drivers/pci/access.c
>> > +++ b/drivers/pci/access.c
>> > @@ -484,10 +484,14 @@ static inline bool pcie_cap_has_lnkctl(const struct pci_dev *dev)
>> >  {
>> >         int type = pci_pcie_type(dev);
>> >
>> > -       return pcie_cap_version(dev) > 1 ||
>> > -              type == PCI_EXP_TYPE_ROOT_PORT ||
>> > -              type == PCI_EXP_TYPE_ENDPOINT ||
>> > -              type == PCI_EXP_TYPE_LEG_END;
>> > +       return  pcie_cap_version(dev) == 1 ||
>> > +               type == PCI_EXP_TYPE_ENDPOINT ||
>> > +               type == PCI_EXP_TYPE_LEG_END ||
>> > +               type == PCI_EXP_TYPE_ROOT_PORT ||
>> > +               type == PCI_EXP_TYPE_UPSTREAM ||
>> > +               type == PCI_EXP_TYPE_DOWNSTREAM ||
>> > +               type == PCI_EXP_TYPE_PCI_BRIDGE ||
>> > +               type == PCI_EXP_TYPE_PCIE_BRIDGE;
>> >  }
>> >
>> >  static inline bool pcie_cap_has_sltctl(const struct pci_dev *dev)
>> > --
>> > 1.8.1.2
>> >
>
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH] PCI: correctly check availability of PCIe Link Cap/Status/Control registers
  2013-08-27 18:11                   ` Bjorn Helgaas
@ 2013-08-27 22:28                     ` Yuval Mintz
  0 siblings, 0 replies; 16+ messages in thread
From: Yuval Mintz @ 2013-08-27 22:28 UTC (permalink / raw)
  To: Bjorn Helgaas, Keller, Jacob E
  Cc: Jiang Liu, Zhao, Yu, Jiang Liu, linux-pci @ vger . kernel . org

> On Tue, Aug 27, 2013 at 12:02 PM, Keller, Jacob E
> >> > From: Jiang Liu <jiang.liu@huawei.com>
> >> > ---
> >> > Hi Bjorn,
> >> >         Yuval has a PCIe 1.0 swither, so triggers the bug.  I'm not sure
> >> > why Yu Zhao has implemented pcie_cap_has_linctl() in that way, any
> special
> >> > consideration here?
> >> >
> >> > Hi Yuval,
> >> >         Could you please help to test this patch? Due to hardware resource
> >> > constraints, I have just compiled the patch on my laptop without testing.
> >> >

Hi,

Don't know whether your fix is correct or not (I'm leaving that to you PCI guys),
but it solved my issue - width now returns as x4. Thanks.

Tested-by: Yuval Mintz <yuvalmin@broadcom.com>

> >> > Thanks!
> >> >
> >> > ---
> >> >  drivers/pci/access.c | 12 ++++++++----
> >> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> >> > index 1cc2366..23ff366 100644
> >> > --- a/drivers/pci/access.c
> >> > +++ b/drivers/pci/access.c
> >> > @@ -484,10 +484,14 @@ static inline bool pcie_cap_has_lnkctl(const
> struct pci_dev *dev)
> >> >  {
> >> >         int type = pci_pcie_type(dev);
> >> >
> >> > -       return pcie_cap_version(dev) > 1 ||
> >> > -              type == PCI_EXP_TYPE_ROOT_PORT ||
> >> > -              type == PCI_EXP_TYPE_ENDPOINT ||
> >> > -              type == PCI_EXP_TYPE_LEG_END;
> >> > +       return  pcie_cap_version(dev) == 1 ||
> >> > +               type == PCI_EXP_TYPE_ENDPOINT ||
> >> > +               type == PCI_EXP_TYPE_LEG_END ||
> >> > +               type == PCI_EXP_TYPE_ROOT_PORT ||
> >> > +               type == PCI_EXP_TYPE_UPSTREAM ||
> >> > +               type == PCI_EXP_TYPE_DOWNSTREAM ||
> >> > +               type == PCI_EXP_TYPE_PCI_BRIDGE ||
> >> > +               type == PCI_EXP_TYPE_PCIE_BRIDGE;
> >> >  }
> >> >
> >> >  static inline bool pcie_cap_has_sltctl(const struct pci_dev *dev)
> >> > --
> >> > 1.8.1.2
> >> >
> >
> >



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] PCI: correctly check availability of PCIe Link Cap/Status/Control registers
  2013-08-27 17:07               ` Bjorn Helgaas
  2013-08-27 18:02                 ` Keller, Jacob E
@ 2013-08-28  0:08                 ` Jiang Liu
  2013-08-28 12:58                   ` Bjorn Helgaas
  1 sibling, 1 reply; 16+ messages in thread
From: Jiang Liu @ 2013-08-28  0:08 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Yuval Mintz, jacob . e . keller @ intel . com, Yu Zhao, Jiang Liu,
	linux-pci @ vger . kernel . org

On 08/28/2013 01:07 AM, Bjorn Helgaas wrote:
> On Tue, Aug 27, 2013 at 10:44 AM, Jiang Liu <liuj97@gmail.com> wrote:
>> From: Jiang Liu <jiang.liu@huawei.com>
>>
>> PCIe Base Spec 1.0 Section 5.8 states that "The PCI Express Capabilities,
>> Device Capabilities, Device Status/Control, Link Capabilities and
>> Link Status/Control registers are required for all PCI Express devices."
> 
> And spec 1.1 amended that (in sec 7.8, not 5.8) to say that the link
> registers are not required for Root Complex Integrated Endpoints.
> Integrated endpoints and event collectors don't have links, so even if
> some RC-integrated devices do implement link registers per spec 1.0, I
> don't think there's any point in allowing access to them.
Hi Bjorn,
	Sorry, I haven't noticed that PCIe Base spec 1.1 has modified
the definition without changing the PCIe capability version. So seems
we should remove the "pcie_cap_version(dev) == 1" check and just
verify PCIe express device types. Which form do you prefer?

!(type == PCI_EXP_TYPE_RC_END || type == PCI_EXP_TYPE_RC_EC)
or
type == PCI_EXP_TYPE_ENDPOINT ||
type == PCI_EXP_TYPE_LEG_END ||
type == PCI_EXP_TYPE_ROOT_PORT ||
type == PCI_EXP_TYPE_UPSTREAM ||
type == PCI_EXP_TYPE_DOWNSTREAM ||
type == PCI_EXP_TYPE_PCI_BRIDGE ||
type == PCI_EXP_TYPE_PCIE_BRIDGE;

Regards!
Gerry

> 
>> And PCIe Base Spec 3.0 Section 7.8 states that "The Link Capabilities,
>> Link Status, and Link Control registers are required for all Root Ports,
>> Switch Ports, Bridges, and Endpoints that are not Root Complex Integrated
>> Endpoints."
>>
>> Changeset 1b6b8ce2ac372e "PCI: only save/restore existent registers in the
>> PCIe capability" introduces pcie_cap_has_lnkctl(), which assumes:
>> 1) PCIe root port, endpoint and legacy endpoint with 1.0 PCIe Capability
>>    implement Link Cap/Status/Control registers.
>> 2) All PCIe devices with 2.0 PCIe Capability implemnet Link Cap/Status/
>>    Control registers.
>>
>> The above assumptions don't conform to PCIe base specifications, so change
>> pcie_cap_has_lnkctl() to follow PCIe specifications.
>>
>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>> ---
>> Hi Bjorn,
>>         Yuval has a PCIe 1.0 swither, so triggers the bug.  I'm not sure
>> why Yu Zhao has implemented pcie_cap_has_linctl() in that way, any special
>> consideration here?
>>
>> Hi Yuval,
>>         Could you please help to test this patch? Due to hardware resource
>> constraints, I have just compiled the patch on my laptop without testing.
>>
>> Thanks!
>>
>> ---
>>  drivers/pci/access.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
>> index 1cc2366..23ff366 100644
>> --- a/drivers/pci/access.c
>> +++ b/drivers/pci/access.c
>> @@ -484,10 +484,14 @@ static inline bool pcie_cap_has_lnkctl(const struct pci_dev *dev)
>>  {
>>         int type = pci_pcie_type(dev);
>>
>> -       return pcie_cap_version(dev) > 1 ||
>> -              type == PCI_EXP_TYPE_ROOT_PORT ||
>> -              type == PCI_EXP_TYPE_ENDPOINT ||
>> -              type == PCI_EXP_TYPE_LEG_END;
>> +       return  pcie_cap_version(dev) == 1 ||
>> +               type == PCI_EXP_TYPE_ENDPOINT ||
>> +               type == PCI_EXP_TYPE_LEG_END ||
>> +               type == PCI_EXP_TYPE_ROOT_PORT ||
>> +               type == PCI_EXP_TYPE_UPSTREAM ||
>> +               type == PCI_EXP_TYPE_DOWNSTREAM ||
>> +               type == PCI_EXP_TYPE_PCI_BRIDGE ||
>> +               type == PCI_EXP_TYPE_PCIE_BRIDGE;
>>  }
>>
>>  static inline bool pcie_cap_has_sltctl(const struct pci_dev *dev)
>> --
>> 1.8.1.2
>>


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] PCI: correctly check availability of PCIe Link Cap/Status/Control registers
  2013-08-28  0:08                 ` Jiang Liu
@ 2013-08-28 12:58                   ` Bjorn Helgaas
  2013-08-28 17:23                     ` [PATCH v2] " Jiang Liu
  0 siblings, 1 reply; 16+ messages in thread
From: Bjorn Helgaas @ 2013-08-28 12:58 UTC (permalink / raw)
  To: Jiang Liu
  Cc: Yuval Mintz, jacob . e . keller @ intel . com, Yu Zhao, Jiang Liu,
	linux-pci @ vger . kernel . org

On Tue, Aug 27, 2013 at 6:08 PM, Jiang Liu <liuj97@gmail.com> wrote:
> On 08/28/2013 01:07 AM, Bjorn Helgaas wrote:
>> On Tue, Aug 27, 2013 at 10:44 AM, Jiang Liu <liuj97@gmail.com> wrote:
>>> From: Jiang Liu <jiang.liu@huawei.com>
>>>
>>> PCIe Base Spec 1.0 Section 5.8 states that "The PCI Express Capabilities,
>>> Device Capabilities, Device Status/Control, Link Capabilities and
>>> Link Status/Control registers are required for all PCI Express devices."
>>
>> And spec 1.1 amended that (in sec 7.8, not 5.8) to say that the link
>> registers are not required for Root Complex Integrated Endpoints.
>> Integrated endpoints and event collectors don't have links, so even if
>> some RC-integrated devices do implement link registers per spec 1.0, I
>> don't think there's any point in allowing access to them.
> Hi Bjorn,
>         Sorry, I haven't noticed that PCIe Base spec 1.1 has modified
> the definition without changing the PCIe capability version. So seems
> we should remove the "pcie_cap_version(dev) == 1" check and just
> verify PCIe express device types. Which form do you prefer?

The "pcie_cap_version(dev) > 1" test is based on the spec r3.0
language to the effect that "For Functions that do not implement the
[Device, Link, Slot, Root] registers, these spaces must be hardwired
to 0b."  That means that for v2 capabilities, we don't need to check
the device type at all.  I think we should keep that check so the
structure of pcie_cap_has_lnkctl() remains similar to
pcie_cap_has_sltctl() and pcie_cap_has_rtctl().

It might be more obvious what we're doing if we restructured them all
in the form of:

    if (pcie_cap_version(dev) == 1)
        return type == PCI_EXP_TYPE_ROOT_PORT ||
                type == PCI_EXP_TYPE_ENDPOINT ||
                ...;
    return true;

That would make it more clear that for v1 capabilities, we have to
make extra checks, and for v2 capabilities, we rely on the r3.0 spec
language.

> !(type == PCI_EXP_TYPE_RC_END || type == PCI_EXP_TYPE_RC_EC)
> or
> type == PCI_EXP_TYPE_ENDPOINT ||
> type == PCI_EXP_TYPE_LEG_END ||
> type == PCI_EXP_TYPE_ROOT_PORT ||
> type == PCI_EXP_TYPE_UPSTREAM ||
> type == PCI_EXP_TYPE_DOWNSTREAM ||
> type == PCI_EXP_TYPE_PCI_BRIDGE ||
> type == PCI_EXP_TYPE_PCIE_BRIDGE;

I don't really care which of these styles we use.  Either way seems
future-proof, since no new device type should ever be added with a v1
capability.  Yours has the advantage of being stated in the positive,
which is a good aid to understanding.

>>> And PCIe Base Spec 3.0 Section 7.8 states that "The Link Capabilities,
>>> Link Status, and Link Control registers are required for all Root Ports,
>>> Switch Ports, Bridges, and Endpoints that are not Root Complex Integrated
>>> Endpoints."
>>>
>>> Changeset 1b6b8ce2ac372e "PCI: only save/restore existent registers in the
>>> PCIe capability" introduces pcie_cap_has_lnkctl(), which assumes:
>>> 1) PCIe root port, endpoint and legacy endpoint with 1.0 PCIe Capability
>>>    implement Link Cap/Status/Control registers.
>>> 2) All PCIe devices with 2.0 PCIe Capability implemnet Link Cap/Status/
>>>    Control registers.
>>>
>>> The above assumptions don't conform to PCIe base specifications, so change
>>> pcie_cap_has_lnkctl() to follow PCIe specifications.
>>>
>>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>>> ---
>>> Hi Bjorn,
>>>         Yuval has a PCIe 1.0 swither, so triggers the bug.  I'm not sure
>>> why Yu Zhao has implemented pcie_cap_has_linctl() in that way, any special
>>> consideration here?
>>>
>>> Hi Yuval,
>>>         Could you please help to test this patch? Due to hardware resource
>>> constraints, I have just compiled the patch on my laptop without testing.
>>>
>>> Thanks!
>>>
>>> ---
>>>  drivers/pci/access.c | 12 ++++++++----
>>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
>>> index 1cc2366..23ff366 100644
>>> --- a/drivers/pci/access.c
>>> +++ b/drivers/pci/access.c
>>> @@ -484,10 +484,14 @@ static inline bool pcie_cap_has_lnkctl(const struct pci_dev *dev)
>>>  {
>>>         int type = pci_pcie_type(dev);
>>>
>>> -       return pcie_cap_version(dev) > 1 ||
>>> -              type == PCI_EXP_TYPE_ROOT_PORT ||
>>> -              type == PCI_EXP_TYPE_ENDPOINT ||
>>> -              type == PCI_EXP_TYPE_LEG_END;
>>> +       return  pcie_cap_version(dev) == 1 ||
>>> +               type == PCI_EXP_TYPE_ENDPOINT ||
>>> +               type == PCI_EXP_TYPE_LEG_END ||
>>> +               type == PCI_EXP_TYPE_ROOT_PORT ||
>>> +               type == PCI_EXP_TYPE_UPSTREAM ||
>>> +               type == PCI_EXP_TYPE_DOWNSTREAM ||
>>> +               type == PCI_EXP_TYPE_PCI_BRIDGE ||
>>> +               type == PCI_EXP_TYPE_PCIE_BRIDGE;
>>>  }
>>>
>>>  static inline bool pcie_cap_has_sltctl(const struct pci_dev *dev)
>>> --
>>> 1.8.1.2
>>>
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2] PCI: correctly check availability of PCIe Link Cap/Status/Control registers
  2013-08-28 12:58                   ` Bjorn Helgaas
@ 2013-08-28 17:23                     ` Jiang Liu
  0 siblings, 0 replies; 16+ messages in thread
From: Jiang Liu @ 2013-08-28 17:23 UTC (permalink / raw)
  To: Bjorn Helgaas, Yuval Mintz, jacob . e . keller @ intel . com
  Cc: liuj97, Jiang Liu, linux-pci @ vger . kernel . org

From: Jiang Liu <jiang.liu@huawei.com>

According to PCIe Base Specification 3.0, devices are guarenteed to
return zero for unimplemented V2 PCI Express Capability registers.
But there's no such guarantee for unimplemented V1 PCI Express
Capability registers, so we need to explicitly check availability of
V1 PCI Express Capability registers and return zero for unimplemented
registers.

Changeset 1b6b8ce2ac372e "PCI: only save/restore existent registers in
the PCIe capability" introduces pcie_cap_has_lnkctl(), which assumes that
PCIe root port, endpoint and legacy endpoint with 1.0 PCIe Capability
implement Link Cap/Status/Control registers.

On the other hand, section 7.8 of PCIe Base Spec V1.1 and V3.0 states that
"The Link Capabilities, Link Status, and Link Control registers are
required for all Root Ports, Switch Ports, Bridges, and Endpoints that
are not Root Complex Integrated Endpoints."
That means Link Capability/Status/Control registers are also available
for PCIe Upstream Port, Downstream Port, PCIe-to-PCI bridge and PCI-to-PCIe
bridge. So change pcie_cap_has_lnkctl() to follow PCIe specifications.

Also refine pcie_cap_has_sltctl() and pcie_cap_has_rtctl() for readability.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Reported-by: Yuval Mintz <yuvalmin@broadcom.com>
---
 drivers/pci/access.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 1cc2366..b9af3d1 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -470,6 +470,13 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
 }
 EXPORT_SYMBOL_GPL(pci_cfg_access_unlock);
 
+/*
+ * According to PCIe base spec 3.0, devices are guarenteed to return zero for
+ * unimplemented V2 PCI Express Capability registers. But there's no such
+ * guarantee for unimplemented V1 PCI Express Capability registers, so
+ * explicitly check availability of V1 PCI Express Capability registers
+ * and return zero for unimplemented registers.
+ */
 static inline int pcie_cap_version(const struct pci_dev *dev)
 {
 	return pcie_caps_reg(dev) & PCI_EXP_FLAGS_VERS;
@@ -484,29 +491,39 @@ static inline bool pcie_cap_has_lnkctl(const struct pci_dev *dev)
 {
 	int type = pci_pcie_type(dev);
 
-	return pcie_cap_version(dev) > 1 ||
-	       type == PCI_EXP_TYPE_ROOT_PORT ||
-	       type == PCI_EXP_TYPE_ENDPOINT ||
-	       type == PCI_EXP_TYPE_LEG_END;
+	if (pcie_cap_version(dev) == 1)
+		return	type == PCI_EXP_TYPE_ENDPOINT ||
+			type == PCI_EXP_TYPE_LEG_END ||
+			type == PCI_EXP_TYPE_ROOT_PORT ||
+			type == PCI_EXP_TYPE_UPSTREAM ||
+			type == PCI_EXP_TYPE_DOWNSTREAM ||
+			type == PCI_EXP_TYPE_PCI_BRIDGE ||
+			type == PCI_EXP_TYPE_PCIE_BRIDGE;
+
+	return true;
 }
 
 static inline bool pcie_cap_has_sltctl(const struct pci_dev *dev)
 {
 	int type = pci_pcie_type(dev);
 
-	return pcie_cap_version(dev) > 1 ||
-	       type == PCI_EXP_TYPE_ROOT_PORT ||
-	       (type == PCI_EXP_TYPE_DOWNSTREAM &&
-		pcie_caps_reg(dev) & PCI_EXP_FLAGS_SLOT);
+	if (pcie_cap_version(dev) == 1)
+		return type == PCI_EXP_TYPE_ROOT_PORT ||
+		       (type == PCI_EXP_TYPE_DOWNSTREAM &&
+			pcie_caps_reg(dev) & PCI_EXP_FLAGS_SLOT);
+
+	return true;
 }
 
 static inline bool pcie_cap_has_rtctl(const struct pci_dev *dev)
 {
 	int type = pci_pcie_type(dev);
 
-	return pcie_cap_version(dev) > 1 ||
-	       type == PCI_EXP_TYPE_ROOT_PORT ||
-	       type == PCI_EXP_TYPE_RC_EC;
+	if (pcie_cap_version(dev) == 1)
+		return type == PCI_EXP_TYPE_ROOT_PORT ||
+		       type == PCI_EXP_TYPE_RC_EC;
+
+	return true;
 }
 
 static bool pcie_capability_reg_implemented(struct pci_dev *dev, int pos)
-- 
1.8.1.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2013-08-28 17:25 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20130825100157.GA16500@lb-tlvb-yuvalmin.il.broadcom.com>
2013-08-25 11:21 ` pcie_get_minimum_link returns 0 width Yuval Mintz
2013-08-26 18:27   ` Keller, Jacob E
2013-08-26 22:05   ` Bjorn Helgaas
2013-08-26 23:36     ` Yuval Mintz
2013-08-26 23:57       ` Bjorn Helgaas
2013-08-27  7:40         ` Yuval Mintz
2013-08-27 13:57           ` Bjorn Helgaas
2013-08-27 16:13             ` Bjorn Helgaas
2013-08-27 16:44             ` [PATCH] PCI: correctly check availability of PCIe Link Cap/Status/Control registers Jiang Liu
2013-08-27 17:07               ` Bjorn Helgaas
2013-08-27 18:02                 ` Keller, Jacob E
2013-08-27 18:11                   ` Bjorn Helgaas
2013-08-27 22:28                     ` Yuval Mintz
2013-08-28  0:08                 ` Jiang Liu
2013-08-28 12:58                   ` Bjorn Helgaas
2013-08-28 17:23                     ` [PATCH v2] " Jiang Liu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).