diff mbox series

PCI: cadence: Set the AFS bit in Device Capabilities 2 Register

Message ID 20230802103059.3492181-1-a-verma1@ti.com (mailing list archive)
State New, archived
Headers show
Series PCI: cadence: Set the AFS bit in Device Capabilities 2 Register | expand

Commit Message

Achal Verma Aug. 2, 2023, 10:30 a.m. UTC
J7 PCIe Root Complex has ARI Forwarding Support, means supporting
forwarding of TLPs addressed to functions with function number greater than
7 but some PCIe instances on J7 have this bit cleared which results in
failure of forwarding of TLPs destined for function number > 7.
Setting the AFS bit in Device Capabilities 2 Register explicitly, resolves
the issue and leads to successful access to function number > 7.

Some observations:
1. J7200-EVB has single PCIe instance(PCIe1) for which ARIFwd bit is not
   set. Enumeration gracefully fails for funciton number greater than 7 but
   later read/write access to these funcitons results in a crash.

2. On J721E-EVB, PCIe1 instance has ARIFwd bit set while it is cleared for
   PCIe0 instance. This issue combined with errata i2086
   (Unsupported Request (UR) Response Results in External Abort) results in
   SERROR while scanning multi-function endpoint device.

Kernel panic - not syncing: Asynchronous SError Interrupt
Hardware name: Texas Instruments K3 J721E SoC (DT)
Workqueue: events deferred_probe_work_func
Call trace:
 dump_backtrace+0x0/0x1a0
 show_stack+0x18/0x68
 dump_stack+0xd0/0x12c
 panic+0x16c/0x334
 nmi_panic+0x8c/0x90
 arm64_serror_panic+0x78/0x84
 do_serror+0x38/0x98
 el1_error+0x90/0x110
 pci_generic_config_read+0x3c/0xe0
 cdns_ti_pcie_config_read+0x18/0x38
 pci_bus_read_config_dword+0x80/0xd8
 pci_bus_generic_read_dev_vendor_id+0x34/0x1b0
 pci_bus_read_dev_vendor_id+0x4c/0x70
 pci_scan_single_device+0x7c/0xf8
 pci_scan_slot+0x74/0x120
 pci_scan_child_bus_extend+0x54/0x298
 pci_scan_bridge_extend+0x29c/0x580
 pci_scan_child_bus_extend+0x1e4/0x298
 pci_scan_root_bus_bridge+0x64/0xd8
 pci_host_probe+0x18/0xc8
 cdns_pcie_host_setup+0x534/0x8f0
 j721e_pcie_probe+0x494/0x820

Signed-off-by: Achal Verma <a-verma1@ti.com>
---
 drivers/pci/controller/cadence/pci-j721e.c         |  4 ++++
 drivers/pci/controller/cadence/pcie-cadence-host.c |  7 +++++++
 drivers/pci/controller/cadence/pcie-cadence.h      | 12 ++++++++++++
 3 files changed, 23 insertions(+)

Comments

Bjorn Helgaas Aug. 2, 2023, 4:19 p.m. UTC | #1
In subject, "Advertise ARI Forwarding Supported".

It's not obvious that "AFS" refers to ARI Forwarding Supported, and
the bit name is enough; we don't need to know that it's in Dev Cap 2.
"Advertise" shows that we're just *advertising* the functionality, not
*enabling* it.

On Wed, Aug 02, 2023 at 04:00:59PM +0530, Achal Verma wrote:
> J7 PCIe Root Complex has ARI Forwarding Support, means supporting
> forwarding of TLPs addressed to functions with function number greater than
> 7 but some PCIe instances on J7 have this bit cleared which results in
> failure of forwarding of TLPs destined for function number > 7.
> Setting the AFS bit in Device Capabilities 2 Register explicitly, resolves
> the issue and leads to successful access to function number > 7.

s/AFS/ARI Forwarding Supported/

> Some observations:
> 1. J7200-EVB has single PCIe instance(PCIe1) for which ARIFwd bit is not
>    set. Enumeration gracefully fails for funciton number greater than 7 but
>    later read/write access to these funcitons results in a crash.

By "ARIFwd bit" here, I assume you mean PCI_EXP_DEVCAP2_ARI in the Root
Port?  Maybe you can use the #define to make this more greppable.

s/funciton/function/ (twice)

If we don't enumerate function numbers greater than 7, we shouldn't
have pci_dev structs for them, so why are there later read/write
config accesses to them?

If the Root Port doesn't advertise ARI Forwarding Supported,
bridge->ari_enabled will not be set, and we shouldn't even try to
enumerate functions greater than 7.  So it's not that enumeration
*fails*; it just doesn't happen at all.

> 2. On J721E-EVB, PCIe1 instance has ARIFwd bit set while it is cleared for
>    PCIe0 instance. This issue combined with errata i2086
>    (Unsupported Request (UR) Response Results in External Abort) results in
>    SERROR while scanning multi-function endpoint device.

Is the SERROR when scanning under PCIe0 or under PCIe1?

I'm not clear on what's happening here:

  1) Root Port advertises PCI_EXP_DEVCAP2_ARI, we set
     bridge->ari_enabled and scan functions > 7, we do a config read
     to function 8, get a UR response (as expected during enumeration)
     and that results in SERROR?

  2) Root Port *doesn't* advertise PCI_EXP_DEVCAP2_ARI, we don't set
     bridge->ari_enabled, so we don't try config read to function 8,
     and something blows up later?

  3) Something else?

> @@ -507,6 +507,7 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>  	struct cdns_pcie *pcie;
>  	struct resource *res;
>  	int ret;
> +	u32 pcie_cap2;
>  
>  	bridge = pci_host_bridge_from_priv(rc);
>  	if (!bridge)
> @@ -536,6 +537,12 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>  	if (rc->quirk_detect_quiet_flag)
>  		cdns_pcie_detect_quiet_min_delay_set(&rc->pcie);
>  
> +	if (rc->set_afs_bit) {
> +		pcie_cap2 = cdns_pcie_rp_readl(pcie, CDNS_PCIE_RP_CAP_OFFSET + PCI_EXP_DEVCAP2);
> +		pcie_cap2 |= PCI_EXP_DEVCAP2_ARI;
> +		cdns_pcie_rp_writel(pcie, CDNS_PCIE_RP_CAP_OFFSET + PCI_EXP_DEVCAP2, pcie_cap2);
> +	}

This seems like a j721e defect; why does the workaround need to be in
the generic cadence code?

Bjorn
Achal Verma Aug. 4, 2023, 7:52 a.m. UTC | #2
On 8/2/2023 9:49 PM, Bjorn Helgaas wrote:
> In subject, "Advertise ARI Forwarding Supported".
Ok
> 
> It's not obvious that "AFS" refers to ARI Forwarding Supported, and
> the bit name is enough; we don't need to know that it's in Dev Cap 2.
> "Advertise" shows that we're just *advertising* the functionality, not
> *enabling* it.
> 
> On Wed, Aug 02, 2023 at 04:00:59PM +0530, Achal Verma wrote:
>> J7 PCIe Root Complex has ARI Forwarding Support, means supporting
>> forwarding of TLPs addressed to functions with function number greater than
>> 7 but some PCIe instances on J7 have this bit cleared which results in
>> failure of forwarding of TLPs destined for function number > 7.
>> Setting the AFS bit in Device Capabilities 2 Register explicitly, resolves
>> the issue and leads to successful access to function number > 7.
> 
> s/AFS/ARI Forwarding Supported/
> 
>> Some observations:
>> 1. J7200-EVB has single PCIe instance(PCIe1) for which ARIFwd bit is not
>>     set. Enumeration gracefully fails for funciton number greater than 7 but
>>     later read/write access to these funcitons results in a crash.
> 
> By "ARIFwd bit" here, I assume you mean PCI_EXP_DEVCAP2_ARI in the Root
> Port?  Maybe you can use the #define to make this more greppable.
> 
will replace with PCI_EXP_DEVCAP2_ARI
> s/funciton/function/ (twice)
> 
> If we don't enumerate function numbers greater than 7, we shouldn't
> have pci_dev structs for them, so why are there later read/write
> config accesses to them?
> 
> If the Root Port doesn't advertise ARI Forwarding Supported,
> bridge->ari_enabled will not be set, and we shouldn't even try to
> enumerate functions greater than 7.  So it's not that enumeration
> *fails*; it just doesn't happen at all.
> 
>> 2. On J721E-EVB, PCIe1 instance has ARIFwd bit set while it is cleared for
>>     PCIe0 instance. This issue combined with errata i2086
>>     (Unsupported Request (UR) Response Results in External Abort) results in
>>     SERROR while scanning multi-function endpoint device.
> 
> Is the SERROR when scanning under PCIe0 or under PCIe1?
> 
> I'm not clear on what's happening here:
> 
>    1) Root Port advertises PCI_EXP_DEVCAP2_ARI, we set
>       bridge->ari_enabled and scan functions > 7, we do a config read
>       to function 8, get a UR response (as expected during enumeration)
>       and that results in SERROR?
> 
>    2) Root Port *doesn't* advertise PCI_EXP_DEVCAP2_ARI, we don't set
>       bridge->ari_enabled, so we don't try config read to function 8,
>       and something blows up later?
> 
>    3) Something else?
> 
Hello Bjorn,

There are multiple issues which are leading to different behavior on 
different platforms.

1. PCI_EXP_DEVCAP2_ARI is not set.

Consider scenario:
J7200 (RC) --- J721E (EP with 1 PF and 4 VFs)

PF enumerates successfully on J7200 but bringing up 4 associated VFs 
(echo 4 > /sys/bus/pci/devices/<device>/sriov_numvfs) doesn't workout. 
First VF gets devfn = 6 and then +1 for next one on wards. 6 and 7 are 
setup fine but 8 and 9 fails and UR is received while accessing them. 
Accessing VFs > 7 doesn't go through the ARI support check and directly 
calls pci_setup_device(). So, since PCI_EXP_DEVCAP2_ARI is not set, 
unable to access VFs > 7.

do_serror+0x34/0x88
el1_error+0x8c/0x10c
pci_generic_config_read+0x90/0xd0
cdns_ti_pcie_config_read+0x14/0x28
pci_bus_read_config_word+0x78/0xd0
__pci_bus_find_cap_start+0x2c/0x78
pci_find_capability+0x38/0x90
set_pcie_port_type+0x2c/0x150
pci_setup_device+0x90/0x728
pci_iov_add_virtfn+0xe4/0x2e0
sriov_enable+0x1f0/0x440
pci_sriov_configure_simple+0x34/0x80
sriov_numvfs_store+0xa4/0x190


Same issue happens when J7200 RC is replaced by J721E PCIe0 in above 
setup but because of errata i2086 in this case, UR response results in 
SERROR too.

2. PCI_ARI_CAP_NFN field in PCI_ARI_CAP for last function is not set to 
0 which marks current function as last but instead set to current_fn+1, 
which leads to reading of vendor ID for current_fn+1 which doesn't 
exists, giving UR response.

Consider scenario:
J721E(PCIe1 as RC) ----- J721E (EP with 1PF)
PCIe1 has PCI_EXP_DEVCAP2_ARI set , so it reads the PCI_ARI_CAP_NFN 
field of EP to get the next function number, now since at last function 
(PF=0) PCI_ARI_CAP_NFN field is set to 1 and not to 0, RC tries to 
access devfn=1 , which results in UR along with SERROR because of errata 
i2086.
For this I had pushed the workaround
https://lore.kernel.org/all/20230316065455.191785-1-a-verma1@ti.com/

do_serror+0x34/0x88
el1_error+0x8c/0x10c
pci_generic_config_read+0x38/0xd0
cdns_ti_pcie_config_read+0x14/0x28
pci_bus_read_config_dword+0x7c/0xd0
pci_bus_generic_read_dev_vendor_id+0x30/0x1a8
pci_bus_read_dev_vendor_id+0x48/0x68
pci_scan_single_device+0x74/0xf0
pci_scan_slot+0x70/0x118
pci_scan_child_bus_extend+0x50/0x290
pci_scan_bridge_extend+0x294/0x578
pci_scan_child_bus_extend+0x1dc/0x290
pci_scan_root_bus_bridge+0x60/0xd0
pci_host_probe+0x14/0xc0
cdns_pcie_host_setup+0x52c/0x8e0
j721e_pcie_probe+0x48c/0x818
platform_drv_probe+0x50/0xa0


>> @@ -507,6 +507,7 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>   	struct cdns_pcie *pcie;
>>   	struct resource *res;
>>   	int ret;
>> +	u32 pcie_cap2;
>>   
>>   	bridge = pci_host_bridge_from_priv(rc);
>>   	if (!bridge)
>> @@ -536,6 +537,12 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>   	if (rc->quirk_detect_quiet_flag)
>>   		cdns_pcie_detect_quiet_min_delay_set(&rc->pcie);
>>   
>> +	if (rc->set_afs_bit) {
>> +		pcie_cap2 = cdns_pcie_rp_readl(pcie, CDNS_PCIE_RP_CAP_OFFSET + PCI_EXP_DEVCAP2);
>> +		pcie_cap2 |= PCI_EXP_DEVCAP2_ARI;
>> +		cdns_pcie_rp_writel(pcie, CDNS_PCIE_RP_CAP_OFFSET + PCI_EXP_DEVCAP2, pcie_cap2);
>> +	}
> 
> This seems like a j721e defect; why does the workaround need to be in
> the generic cadence code?

This register setting has to go between 
devm_platform_ioremap_resource_byname(pdev, "reg") and 
cdns_pcie_start_link() which is in pcie-cadence-host.c

And this will be done only if private data field set_afs_bit is true, 
other quirks are also done in similar way.

Regards,
Achal Verma

> 
> Bjorn
Bjorn Helgaas Aug. 8, 2023, 10:49 p.m. UTC | #3
On Fri, Aug 04, 2023 at 01:22:56PM +0530, Verma, Achal wrote:
> On 8/2/2023 9:49 PM, Bjorn Helgaas wrote:
> > In subject, "Advertise ARI Forwarding Supported".
> Ok
> > 
> > It's not obvious that "AFS" refers to ARI Forwarding Supported, and
> > the bit name is enough; we don't need to know that it's in Dev Cap 2.
> > "Advertise" shows that we're just *advertising* the functionality, not
> > *enabling* it.
> > 
> > On Wed, Aug 02, 2023 at 04:00:59PM +0530, Achal Verma wrote:
> > > J7 PCIe Root Complex has ARI Forwarding Support, means supporting
> > > forwarding of TLPs addressed to functions with function number greater than
> > > 7 but some PCIe instances on J7 have this bit cleared which results in
> > > failure of forwarding of TLPs destined for function number > 7.
> > > Setting the AFS bit in Device Capabilities 2 Register explicitly, resolves
> > > the issue and leads to successful access to function number > 7.
> > 
> > s/AFS/ARI Forwarding Supported/
> > 
> > > Some observations:
> > > 1. J7200-EVB has single PCIe instance(PCIe1) for which ARIFwd bit is not
> > >     set. Enumeration gracefully fails for funciton number greater than 7 but
> > >     later read/write access to these funcitons results in a crash.
> > 
> > By "ARIFwd bit" here, I assume you mean PCI_EXP_DEVCAP2_ARI in the Root
> > Port?  Maybe you can use the #define to make this more greppable.
> > 
> will replace with PCI_EXP_DEVCAP2_ARI
> > s/funciton/function/ (twice)
> > 
> > If we don't enumerate function numbers greater than 7, we shouldn't
> > have pci_dev structs for them, so why are there later read/write
> > config accesses to them?
> > 
> > If the Root Port doesn't advertise ARI Forwarding Supported,
> > bridge->ari_enabled will not be set, and we shouldn't even try to
> > enumerate functions greater than 7.  So it's not that enumeration
> > *fails*; it just doesn't happen at all.
> > 
> > > 2. On J721E-EVB, PCIe1 instance has ARIFwd bit set while it is cleared for
> > >     PCIe0 instance. This issue combined with errata i2086
> > >     (Unsupported Request (UR) Response Results in External Abort) results in
> > >     SERROR while scanning multi-function endpoint device.
> > 
> > Is the SERROR when scanning under PCIe0 or under PCIe1?
> > 
> > I'm not clear on what's happening here:
> > 
> >    1) Root Port advertises PCI_EXP_DEVCAP2_ARI, we set
> >       bridge->ari_enabled and scan functions > 7, we do a config read
> >       to function 8, get a UR response (as expected during enumeration)
> >       and that results in SERROR?
> > 
> >    2) Root Port *doesn't* advertise PCI_EXP_DEVCAP2_ARI, we don't set
> >       bridge->ari_enabled, so we don't try config read to function 8,
> >       and something blows up later?
> > 
> >    3) Something else?
> > 
> Hello Bjorn,
> 
> There are multiple issues which are leading to different behavior on
> different platforms.
> 
> 1. PCI_EXP_DEVCAP2_ARI is not set.
> 
> Consider scenario:
> J7200 (RC) --- J721E (EP with 1 PF and 4 VFs)
> 
> PF enumerates successfully on J7200 but bringing up 4 associated VFs (echo 4
> > /sys/bus/pci/devices/<device>/sriov_numvfs) doesn't workout. First VF gets
> devfn = 6 and then +1 for next one on wards. 6 and 7 are setup fine but 8
> and 9 fails and UR is received while accessing them. Accessing VFs > 7
> doesn't go through the ARI support check and directly calls
> pci_setup_device(). So, since PCI_EXP_DEVCAP2_ARI is not set, unable to
> access VFs > 7.
> 
> do_serror+0x34/0x88
> el1_error+0x8c/0x10c
> pci_generic_config_read+0x90/0xd0
> cdns_ti_pcie_config_read+0x14/0x28
> pci_bus_read_config_word+0x78/0xd0
> __pci_bus_find_cap_start+0x2c/0x78
> pci_find_capability+0x38/0x90
> set_pcie_port_type+0x2c/0x150
> pci_setup_device+0x90/0x728
> pci_iov_add_virtfn+0xe4/0x2e0
> sriov_enable+0x1f0/0x440
> pci_sriov_configure_simple+0x34/0x80
> sriov_numvfs_store+0xa4/0x190

Thanks!  Obviously you should make the Root Port advertise
PCI_EXP_DEVCAP2_ARI if you want to use that functionality.

But I think the fact that we add a device with fn > 7 when ARI is not
enabled is also an underlying defect in iov.c.  

sriov_init() already checks whether ARI is enabled, and I think we
should probably remember that somewhere and use it in
pci_iov_add_virtfn() to avoid adding VFs with fn > 7.

Bjorn
diff mbox series

Patch

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 32b6a7dc3cff..e890fe9ca89d 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -71,6 +71,7 @@  struct j721e_pcie_data {
 	unsigned int		quirk_disable_flr:1;
 	u32			linkdown_irq_regfield;
 	unsigned int		byte_access_allowed:1;
+	unsigned int		set_afs_bit:1;
 };
 
 static inline u32 j721e_pcie_user_readl(struct j721e_pcie *pcie, u32 offset)
@@ -290,6 +291,7 @@  static const struct j721e_pcie_data j721e_pcie_rc_data = {
 	.quirk_retrain_flag = true,
 	.byte_access_allowed = false,
 	.linkdown_irq_regfield = LINK_DOWN,
+	.set_afs_bit = true,
 };
 
 static const struct j721e_pcie_data j721e_pcie_ep_data = {
@@ -302,6 +304,7 @@  static const struct j721e_pcie_data j7200_pcie_rc_data = {
 	.quirk_detect_quiet_flag = true,
 	.linkdown_irq_regfield = J7200_LINK_DOWN,
 	.byte_access_allowed = true,
+	.set_afs_bit = true,
 };
 
 static const struct j721e_pcie_data j7200_pcie_ep_data = {
@@ -391,6 +394,7 @@  static int j721e_pcie_probe(struct platform_device *pdev)
 		rc = pci_host_bridge_priv(bridge);
 		rc->quirk_retrain_flag = data->quirk_retrain_flag;
 		rc->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag;
+		rc->set_afs_bit = data->set_afs_bit;
 
 		cdns_pcie = &rc->pcie;
 		cdns_pcie->dev = dev;
diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 5b14f7ee3c79..fc696d94f7a2 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -507,6 +507,7 @@  int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 	struct cdns_pcie *pcie;
 	struct resource *res;
 	int ret;
+	u32 pcie_cap2;
 
 	bridge = pci_host_bridge_from_priv(rc);
 	if (!bridge)
@@ -536,6 +537,12 @@  int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 	if (rc->quirk_detect_quiet_flag)
 		cdns_pcie_detect_quiet_min_delay_set(&rc->pcie);
 
+	if (rc->set_afs_bit) {
+		pcie_cap2 = cdns_pcie_rp_readl(pcie, CDNS_PCIE_RP_CAP_OFFSET + PCI_EXP_DEVCAP2);
+		pcie_cap2 |= PCI_EXP_DEVCAP2_ARI;
+		cdns_pcie_rp_writel(pcie, CDNS_PCIE_RP_CAP_OFFSET + PCI_EXP_DEVCAP2, pcie_cap2);
+	}
+
 	cdns_pcie_host_enable_ptm_response(pcie);
 
 	ret = cdns_pcie_start_link(pcie);
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index 190786e47df9..7a5d05f3febc 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -329,6 +329,7 @@  struct cdns_pcie_rc {
 	bool			avail_ib_bar[CDNS_PCIE_RP_MAX_IB];
 	unsigned int		quirk_retrain_flag:1;
 	unsigned int		quirk_detect_quiet_flag:1;
+	unsigned int		set_afs_bit:1;
 };
 
 /**
@@ -457,6 +458,17 @@  static inline u16 cdns_pcie_rp_readw(struct cdns_pcie *pcie, u32 reg)
 	return cdns_pcie_read_sz(addr, 0x2);
 }
 
+static inline void cdns_pcie_rp_writel(struct cdns_pcie *pcie,
+				       u32 reg, u32 value)
+{
+	writel(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg);
+}
+
+static inline u32 cdns_pcie_rp_readl(struct cdns_pcie *pcie, u32 reg)
+{
+	return readl(pcie->reg_base + CDNS_PCIE_RP_BASE + reg);
+}
+
 /* Endpoint Function register access */
 static inline void cdns_pcie_ep_fn_writeb(struct cdns_pcie *pcie, u8 fn,
 					  u32 reg, u8 value)