linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] PCI/DPC: Check host->native_dpc before enable dpc service
@ 2021-02-03 12:53 Yicong Yang
  2021-02-03 12:53 ` [PATCH] PCI/DPC: Disable ERR_COR explicitly for native " Yicong Yang
  2021-02-24  9:47 ` [PATCH v2] PCI/DPC: Check host->native_dpc before enable " Yicong Yang
  0 siblings, 2 replies; 12+ messages in thread
From: Yicong Yang @ 2021-02-03 12:53 UTC (permalink / raw)
  To: helgaas, linux-pci
  Cc: sathyanarayanan.kuppuswamy, kbusch, sean.v.kelley, qiuxu.zhuo,
	prime.zeng, yangyicong, linuxarm

Per Downstream Port Containment Related Enhancements ECN[1]
Table 4-6, Interpretation of _OSC Control Field Returned Value,
for bit 7 of _OSC control return value:

  "Firmware sets this bit to 1 to grant the OS control over PCI Express
  Downstream Port Containment configuration."
  "If control of this feature was requested and denied,
  or was not requested, the firmware returns this bit set to 0."

We store bit 7 of _OSC control return value in host->native_dpc,
check it before enable the dpc service as the firmware may not
grant the control.

[1] Downstream Port Containment Related Enhancements ECN,
    Jan 28, 2019, affecting PCI Firmware Specification, Rev. 3.2
    https://members.pcisig.com/wg/PCI-SIG/document/12888

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
Change since v1:
- use correct reference for _OSC control return value

 drivers/pci/pcie/portdrv_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index e1fed664..7445d03 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -253,7 +253,8 @@ static int get_port_device_capability(struct pci_dev *dev)
 	 */
 	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
 	    pci_aer_available() &&
-	    (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
+	    (pcie_ports_dpc_native ||
+	    ((services & PCIE_PORT_SERVICE_AER) && host->native_dpc)))
 		services |= PCIE_PORT_SERVICE_DPC;
 
 	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
-- 
2.8.1


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

* [PATCH] PCI/DPC: Disable ERR_COR explicitly for native dpc service
  2021-02-03 12:53 [PATCH v2] PCI/DPC: Check host->native_dpc before enable dpc service Yicong Yang
@ 2021-02-03 12:53 ` Yicong Yang
  2021-02-18 17:20   ` Bjorn Helgaas
  2021-04-10 15:21   ` Bjorn Helgaas
  2021-02-24  9:47 ` [PATCH v2] PCI/DPC: Check host->native_dpc before enable " Yicong Yang
  1 sibling, 2 replies; 12+ messages in thread
From: Yicong Yang @ 2021-02-03 12:53 UTC (permalink / raw)
  To: helgaas, linux-pci
  Cc: sathyanarayanan.kuppuswamy, kbusch, sean.v.kelley, qiuxu.zhuo,
	prime.zeng, yangyicong, linuxarm

Per Downstream Port Containment Related Enhancements ECN[1],
Table 4-6 Interpretation of _OSC Control Field Returned Value,
for bit 7 of _OSC control return value:

  "If firmware allows the OS control of this feature, then,
  in the context of the _OSC method the OS must ensure that
  Downstream Port Containment ERR_COR signaling is disabled
  as described in the PCI Express Base Specification."

and PCI Express Base Specification Revision 4.0 Version 1.0
section 6.2.10.2, Use of DPC ERR_COR Signaling:

  "...DPC ERR_COR signaling is primarily intended for use by
  platform firmware..."

Currently we don't set DPC ERR_COR enable bit, but explicitly
clear the bit to ensure it's disabled.

[1] Downstream Port Containment Related Enhancements ECN,
    Jan 28, 2019, affecting PCI Firmware Specification, Rev. 3.2
    https://members.pcisig.com/wg/PCI-SIG/document/12888

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/pci/pcie/dpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index e05aba8..5cc8ef3 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -302,7 +302,7 @@ static int dpc_probe(struct pcie_device *dev)
 	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap);
 	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
 
-	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
+	ctl = (ctl & 0xffe4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
 	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
 	pci_info(pdev, "enabled with IRQ %d\n", dev->irq);
 
-- 
2.8.1


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

* Re: [PATCH] PCI/DPC: Disable ERR_COR explicitly for native dpc service
  2021-02-03 12:53 ` [PATCH] PCI/DPC: Disable ERR_COR explicitly for native " Yicong Yang
@ 2021-02-18 17:20   ` Bjorn Helgaas
  2021-02-24  8:56     ` Yicong Yang
  2021-04-10 15:21   ` Bjorn Helgaas
  1 sibling, 1 reply; 12+ messages in thread
From: Bjorn Helgaas @ 2021-02-18 17:20 UTC (permalink / raw)
  To: Yicong Yang
  Cc: linux-pci, sathyanarayanan.kuppuswamy, kbusch, sean.v.kelley,
	qiuxu.zhuo, prime.zeng, linuxarm

On Wed, Feb 03, 2021 at 08:53:15PM +0800, Yicong Yang wrote:
> Per Downstream Port Containment Related Enhancements ECN[1],
> Table 4-6 Interpretation of _OSC Control Field Returned Value,
> for bit 7 of _OSC control return value:
> 
>   "If firmware allows the OS control of this feature, then,
>   in the context of the _OSC method the OS must ensure that
>   Downstream Port Containment ERR_COR signaling is disabled
>   as described in the PCI Express Base Specification."

I think "the OS must ensure" is a typo in the spec.  In the new r3.3
of the spec, it has been corrected to:

  If firmware allows the operating system control of this feature,
  then, in the context of the _OSC method firmware must clear the DPC
  ERR_COR Enable bit in the DPC Control Register (refer to the PCI
  Express Base Specification) to 0.

> and PCI Express Base Specification Revision 4.0 Version 1.0
> section 6.2.10.2, Use of DPC ERR_COR Signaling:
> 
>   "...DPC ERR_COR signaling is primarily intended for use by
>   platform firmware..."
> 
> Currently we don't set DPC ERR_COR enable bit, but explicitly
> clear the bit to ensure it's disabled.

Does this fix a problem you observed?  If you're seeing a problem, and
this patch fixes it, we need to do something.  But if it's just to
line up with the language in the spec, I think we can rely on the
corrected spec language, which says the *firmware* is responsible for
doing this, and leave dpc_probe() alone.

> [1] Downstream Port Containment Related Enhancements ECN,
>     Jan 28, 2019, affecting PCI Firmware Specification, Rev. 3.2
>     https://members.pcisig.com/wg/PCI-SIG/document/12888
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/pci/pcie/dpc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index e05aba8..5cc8ef3 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -302,7 +302,7 @@ static int dpc_probe(struct pcie_device *dev)
>  	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap);
>  	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
>  
> -	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
> +	ctl = (ctl & 0xffe4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;

If we need to clear things here, I'd prefer to have names instead of
the 0xfff4 or 0xffe4 magic numbers.

>  	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
>  	pci_info(pdev, "enabled with IRQ %d\n", dev->irq);
>  
> -- 
> 2.8.1
> 

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

* Re: [PATCH] PCI/DPC: Disable ERR_COR explicitly for native dpc service
  2021-02-18 17:20   ` Bjorn Helgaas
@ 2021-02-24  8:56     ` Yicong Yang
  0 siblings, 0 replies; 12+ messages in thread
From: Yicong Yang @ 2021-02-24  8:56 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: linux-pci, sathyanarayanan.kuppuswamy, kbusch, sean.v.kelley,
	qiuxu.zhuo, prime.zeng, linuxarm

On 2021/2/19 1:20, Bjorn Helgaas wrote:
> On Wed, Feb 03, 2021 at 08:53:15PM +0800, Yicong Yang wrote:
>> Per Downstream Port Containment Related Enhancements ECN[1],
>> Table 4-6 Interpretation of _OSC Control Field Returned Value,
>> for bit 7 of _OSC control return value:
>>
>>   "If firmware allows the OS control of this feature, then,
>>   in the context of the _OSC method the OS must ensure that
>>   Downstream Port Containment ERR_COR signaling is disabled
>>   as described in the PCI Express Base Specification."
> 
> I think "the OS must ensure" is a typo in the spec.  In the new r3.3
> of the spec, it has been corrected to:
> 
>   If firmware allows the operating system control of this feature,
>   then, in the context of the _OSC method firmware must clear the DPC
>   ERR_COR Enable bit in the DPC Control Register (refer to the PCI
>   Express Base Specification) to 0.
> 

yes, it's probably a typo according to the latest spec.

>> and PCI Express Base Specification Revision 4.0 Version 1.0
>> section 6.2.10.2, Use of DPC ERR_COR Signaling:
>>
>>   "...DPC ERR_COR signaling is primarily intended for use by
>>   platform firmware..."
>>
>> Currently we don't set DPC ERR_COR enable bit, but explicitly
>> clear the bit to ensure it's disabled.
> 
> Does this fix a problem you observed?  If you're seeing a problem, and
> this patch fixes it, we need to do something.  But if it's just to
> line up with the language in the spec, I think we can rely on the
> corrected spec language, which says the *firmware* is responsible for
> doing this, and leave dpc_probe() alone.
> 

this patch comes when i was debugging the EDR and navigating the code and spec
(i cannot get the latest spec at that time). no problem was observed but
i have thought it might be sanity to ensure the ERR_COR was not set.

it's ok leave the code as is, as the latest spec exlicitly requires the
firmware to ensure this.

>> [1] Downstream Port Containment Related Enhancements ECN,
>>     Jan 28, 2019, affecting PCI Firmware Specification, Rev. 3.2
>>     https://members.pcisig.com/wg/PCI-SIG/document/12888
>>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> ---
>>  drivers/pci/pcie/dpc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
>> index e05aba8..5cc8ef3 100644
>> --- a/drivers/pci/pcie/dpc.c
>> +++ b/drivers/pci/pcie/dpc.c
>> @@ -302,7 +302,7 @@ static int dpc_probe(struct pcie_device *dev)
>>  	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap);
>>  	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
>>  
>> -	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
>> +	ctl = (ctl & 0xffe4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
> 
> If we need to clear things here, I'd prefer to have names instead of
> the 0xfff4 or 0xffe4 magic numbers.
> 

sure, that will be clearer. i just followed the previous implementation.

Thanks,
Yicong

>>  	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
>>  	pci_info(pdev, "enabled with IRQ %d\n", dev->irq);
>>  
>> -- 
>> 2.8.1
>>
> 
> .
> 


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

* Re: [PATCH v2] PCI/DPC: Check host->native_dpc before enable dpc service
  2021-02-03 12:53 [PATCH v2] PCI/DPC: Check host->native_dpc before enable dpc service Yicong Yang
  2021-02-03 12:53 ` [PATCH] PCI/DPC: Disable ERR_COR explicitly for native " Yicong Yang
@ 2021-02-24  9:47 ` Yicong Yang
  2021-07-26 22:05   ` Bjorn Helgaas
  1 sibling, 1 reply; 12+ messages in thread
From: Yicong Yang @ 2021-02-24  9:47 UTC (permalink / raw)
  To: helgaas, linux-pci
  Cc: sathyanarayanan.kuppuswamy, kbusch, sean.v.kelley, qiuxu.zhuo,
	prime.zeng, linuxarm

On 2021/2/3 20:53, Yicong Yang wrote:
> Per Downstream Port Containment Related Enhancements ECN[1]
> Table 4-6, Interpretation of _OSC Control Field Returned Value,
> for bit 7 of _OSC control return value:
> 
>   "Firmware sets this bit to 1 to grant the OS control over PCI Express
>   Downstream Port Containment configuration."
>   "If control of this feature was requested and denied,
>   or was not requested, the firmware returns this bit set to 0."
> 
> We store bit 7 of _OSC control return value in host->native_dpc,
> check it before enable the dpc service as the firmware may not
> grant the control.
> 
> [1] Downstream Port Containment Related Enhancements ECN,
>     Jan 28, 2019, affecting PCI Firmware Specification, Rev. 3.2
>     https://members.pcisig.com/wg/PCI-SIG/document/12888
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
> Change since v1:
> - use correct reference for _OSC control return value
> 
>  drivers/pci/pcie/portdrv_core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index e1fed664..7445d03 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -253,7 +253,8 @@ static int get_port_device_capability(struct pci_dev *dev)
>  	 */
>  	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
>  	    pci_aer_available() &&
> -	    (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
> +	    (pcie_ports_dpc_native ||
> +	    ((services & PCIE_PORT_SERVICE_AER) && host->native_dpc)))
>  		services |= PCIE_PORT_SERVICE_DPC;
>  

the check here maybe problematic. the bit 7 of _OSC return value is reserved
previously and the change here may break the backward compatibility.
currently we make dpc enabled along with aer, which can ensure the native
dpc won't be enabled if the edr is enabled.

i feel a bit confused as the bit 7 is not used.
does it provide a way to enable native dpc regardless of aer ownership?
just as pcie_ports=dpc-native does when i checked the discussion in [1].

[1] https://lore.kernel.org/linux-pci/20191023192205.97024-1-olof@lixom.net/

Thanks,
Yicong

>  	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
> 


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

* Re: [PATCH] PCI/DPC: Disable ERR_COR explicitly for native dpc service
  2021-02-03 12:53 ` [PATCH] PCI/DPC: Disable ERR_COR explicitly for native " Yicong Yang
  2021-02-18 17:20   ` Bjorn Helgaas
@ 2021-04-10 15:21   ` Bjorn Helgaas
  2021-04-10 19:17     ` Lukas Wunner
  2021-04-12  3:32     ` Kuppuswamy, Sathyanarayanan
  1 sibling, 2 replies; 12+ messages in thread
From: Bjorn Helgaas @ 2021-04-10 15:21 UTC (permalink / raw)
  To: Yicong Yang
  Cc: linux-pci, sathyanarayanan.kuppuswamy, kbusch, sean.v.kelley,
	qiuxu.zhuo, prime.zeng, linuxarm

On Wed, Feb 03, 2021 at 08:53:15PM +0800, Yicong Yang wrote:
> Per Downstream Port Containment Related Enhancements ECN[1],
> Table 4-6 Interpretation of _OSC Control Field Returned Value,
> for bit 7 of _OSC control return value:
> 
>   "If firmware allows the OS control of this feature, then,
>   in the context of the _OSC method the OS must ensure that
>   Downstream Port Containment ERR_COR signaling is disabled
>   as described in the PCI Express Base Specification."
> 
> and PCI Express Base Specification Revision 4.0 Version 1.0
> section 6.2.10.2, Use of DPC ERR_COR Signaling:
> 
>   "...DPC ERR_COR signaling is primarily intended for use by
>   platform firmware..."
> 
> Currently we don't set DPC ERR_COR enable bit, but explicitly
> clear the bit to ensure it's disabled.
> 
> [1] Downstream Port Containment Related Enhancements ECN,
>     Jan 28, 2019, affecting PCI Firmware Specification, Rev. 3.2
>     https://members.pcisig.com/wg/PCI-SIG/document/12888
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

Anybody want to chime in and review this?  Sometimes I feel like a
one-man band :)

> ---
>  drivers/pci/pcie/dpc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index e05aba8..5cc8ef3 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -302,7 +302,7 @@ static int dpc_probe(struct pcie_device *dev)
>  	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap);
>  	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
>  
> -	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
> +	ctl = (ctl & 0xffe4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
>  	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
>  	pci_info(pdev, "enabled with IRQ %d\n", dev->irq);
>  
> -- 
> 2.8.1
> 

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

* Re: [PATCH] PCI/DPC: Disable ERR_COR explicitly for native dpc service
  2021-04-10 15:21   ` Bjorn Helgaas
@ 2021-04-10 19:17     ` Lukas Wunner
  2021-04-12  9:32       ` Yicong Yang
  2021-04-12  3:32     ` Kuppuswamy, Sathyanarayanan
  1 sibling, 1 reply; 12+ messages in thread
From: Lukas Wunner @ 2021-04-10 19:17 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Yicong Yang, linux-pci, sathyanarayanan.kuppuswamy, kbusch,
	sean.v.kelley, qiuxu.zhuo, prime.zeng, linuxarm

On Sat, Apr 10, 2021 at 10:21:03AM -0500, Bjorn Helgaas wrote:
> Anybody want to chime in and review this?  Sometimes I feel like a
> one-man band :)

Can't say anything about the object of the patch, but style-wise
this looks cryptic:

> > --- a/drivers/pci/pcie/dpc.c
> > +++ b/drivers/pci/pcie/dpc.c
> > @@ -302,7 +302,7 @@ static int dpc_probe(struct pcie_device *dev)
> >  	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
> >  
> > -	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
> > +	ctl = (ctl & 0xffe4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
> >  	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);

Instead of writing "ctl & 0xfff4", I'd prefer defining macros for the
register bits of interest, then use "ctl &= ~(u16)(bits to clear)"
and in a separate line use "ctl |= (bits to set)".

Obviously, clearing bits that are unconditionally set afterwards is
unnecessary (as is done here).


> >  	pci_info(pdev, "enabled with IRQ %d\n", dev->irq);

This looks superfluous since the IRQ can be found out in /proc/interrupts.

Thanks,

Lukas

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

* Re: [PATCH] PCI/DPC: Disable ERR_COR explicitly for native dpc service
  2021-04-10 15:21   ` Bjorn Helgaas
  2021-04-10 19:17     ` Lukas Wunner
@ 2021-04-12  3:32     ` Kuppuswamy, Sathyanarayanan
  2021-04-12  9:46       ` Yicong Yang
  1 sibling, 1 reply; 12+ messages in thread
From: Kuppuswamy, Sathyanarayanan @ 2021-04-12  3:32 UTC (permalink / raw)
  To: Bjorn Helgaas, Yicong Yang
  Cc: linux-pci, kbusch, sean.v.kelley, qiuxu.zhuo, prime.zeng, linuxarm



On 4/10/21 8:21 AM, Bjorn Helgaas wrote:
> On Wed, Feb 03, 2021 at 08:53:15PM +0800, Yicong Yang wrote:
>> Per Downstream Port Containment Related Enhancements ECN[1],
>> Table 4-6 Interpretation of _OSC Control Field Returned Value,
>> for bit 7 of _OSC control return value:
>>
>>    "If firmware allows the OS control of this feature, then,
>>    in the context of the _OSC method the OS must ensure that
>>    Downstream Port Containment ERR_COR signaling is disabled
>>    as described in the PCI Express Base Specification."
>>
>> and PCI Express Base Specification Revision 4.0 Version 1.0
>> section 6.2.10.2, Use of DPC ERR_COR Signaling:
>>
>>    "...DPC ERR_COR signaling is primarily intended for use by
>>    platform firmware..."
>>
>> Currently we don't set DPC ERR_COR enable bit, but explicitly
>> clear the bit to ensure it's disabled.
Instead of spec reference, can you explain what error you are
fixing? without this fix what will be the impact and explain
how you mitigating it with your fix.
>>
>> [1] Downstream Port Containment Related Enhancements ECN,
>>      Jan 28, 2019, affecting PCI Firmware Specification, Rev. 3.2
>>      https://members.pcisig.com/wg/PCI-SIG/document/12888
>>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> 
> Anybody want to chime in and review this?  Sometimes I feel like a
> one-man band :)
> 
>> ---
>>   drivers/pci/pcie/dpc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
>> index e05aba8..5cc8ef3 100644
>> --- a/drivers/pci/pcie/dpc.c
>> +++ b/drivers/pci/pcie/dpc.c
>> @@ -302,7 +302,7 @@ static int dpc_probe(struct pcie_device *dev)
>>   	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap);
>>   	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
>>   
>> -	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
>> +	ctl = (ctl & 0xffe4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
>>   	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
>>   	pci_info(pdev, "enabled with IRQ %d\n", dev->irq);
>>   
>> -- 
>> 2.8.1
>>

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

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

* Re: [PATCH] PCI/DPC: Disable ERR_COR explicitly for native dpc service
  2021-04-10 19:17     ` Lukas Wunner
@ 2021-04-12  9:32       ` Yicong Yang
  0 siblings, 0 replies; 12+ messages in thread
From: Yicong Yang @ 2021-04-12  9:32 UTC (permalink / raw)
  To: Lukas Wunner, Bjorn Helgaas
  Cc: linux-pci, sathyanarayanan.kuppuswamy, kbusch, sean.v.kelley,
	qiuxu.zhuo, prime.zeng, linuxarm

On 2021/4/11 3:17, Lukas Wunner wrote:
> On Sat, Apr 10, 2021 at 10:21:03AM -0500, Bjorn Helgaas wrote:
>> Anybody want to chime in and review this?  Sometimes I feel like a
>> one-man band :)
> 
> Can't say anything about the object of the patch, but style-wise
> this looks cryptic:
> 
>>> --- a/drivers/pci/pcie/dpc.c
>>> +++ b/drivers/pci/pcie/dpc.c
>>> @@ -302,7 +302,7 @@ static int dpc_probe(struct pcie_device *dev)
>>>  	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
>>>  
>>> -	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
>>> +	ctl = (ctl & 0xffe4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
>>>  	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
> 
> Instead of writing "ctl & 0xfff4", I'd prefer defining macros for the
> register bits of interest, then use "ctl &= ~(u16)(bits to clear)"
> and in a separate line use "ctl |= (bits to set)".
> 

yes. that's clearer. I'll use macros if we're going to have this patch.

> Obviously, clearing bits that are unconditionally set afterwards is
> unnecessary (as is done here).
> 

ok. I found this when I read the Spec and thought it might be sanity
to ensure the bit is not set.

> 
>>>  	pci_info(pdev, "enabled with IRQ %d\n", dev->irq);
> 
> This looks superfluous since the IRQ can be found out in /proc/interrupts.

this can help us track dpc interrupts on certain ports through /proc/interrupts,
as all the dpc interrupts are named 'pcie-dpc' and its hard to distinguish.

Thanks,
Yicong

> 
> Thanks,
> 
> Lukas
> 
> .
> 


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

* Re: [PATCH] PCI/DPC: Disable ERR_COR explicitly for native dpc service
  2021-04-12  3:32     ` Kuppuswamy, Sathyanarayanan
@ 2021-04-12  9:46       ` Yicong Yang
  0 siblings, 0 replies; 12+ messages in thread
From: Yicong Yang @ 2021-04-12  9:46 UTC (permalink / raw)
  To: Kuppuswamy, Sathyanarayanan, Bjorn Helgaas
  Cc: linux-pci, kbusch, sean.v.kelley, qiuxu.zhuo, prime.zeng,
	linuxarm, Yicong Yang

On 2021/4/12 11:32, Kuppuswamy, Sathyanarayanan wrote:
> 
> 
> On 4/10/21 8:21 AM, Bjorn Helgaas wrote:
>> On Wed, Feb 03, 2021 at 08:53:15PM +0800, Yicong Yang wrote:
>>> Per Downstream Port Containment Related Enhancements ECN[1],
>>> Table 4-6 Interpretation of _OSC Control Field Returned Value,
>>> for bit 7 of _OSC control return value:
>>>
>>>    "If firmware allows the OS control of this feature, then,
>>>    in the context of the _OSC method the OS must ensure that
>>>    Downstream Port Containment ERR_COR signaling is disabled
>>>    as described in the PCI Express Base Specification."
>>>
>>> and PCI Express Base Specification Revision 4.0 Version 1.0
>>> section 6.2.10.2, Use of DPC ERR_COR Signaling:
>>>
>>>    "...DPC ERR_COR signaling is primarily intended for use by
>>>    platform firmware..."
>>>
>>> Currently we don't set DPC ERR_COR enable bit, but explicitly
>>> clear the bit to ensure it's disabled.
> Instead of spec reference, can you explain what error you are
> fixing? without this fix what will be the impact and explain
> how you mitigating it with your fix.

I found this when I read the code and spec. No actual problem.
I have thought it might be sanity to ensure the ERR_COR are not set
if the firmware doesn't ensure this.

In the  PCI Firmware Specification, Rev. 3.3, it mentioned that
firmware must ensure this bit to be cleared if DPC goes native,
so not sure this patch is necessary now.

Thanks,
Yicong

>>>
>>> [1] Downstream Port Containment Related Enhancements ECN,
>>>      Jan 28, 2019, affecting PCI Firmware Specification, Rev. 3.2
>>>      https://members.pcisig.com/wg/PCI-SIG/document/12888
>>>
>>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>>
>> Anybody want to chime in and review this?  Sometimes I feel like a
>> one-man band :)
>>
>>> ---
>>>   drivers/pci/pcie/dpc.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
>>> index e05aba8..5cc8ef3 100644
>>> --- a/drivers/pci/pcie/dpc.c
>>> +++ b/drivers/pci/pcie/dpc.c
>>> @@ -302,7 +302,7 @@ static int dpc_probe(struct pcie_device *dev)
>>>       pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap);
>>>       pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
>>>   -    ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
>>> +    ctl = (ctl & 0xffe4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
>>>       pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
>>>       pci_info(pdev, "enabled with IRQ %d\n", dev->irq);
>>>   -- 
>>> 2.8.1
>>>
> 


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

* Re: [PATCH v2] PCI/DPC: Check host->native_dpc before enable dpc service
  2021-02-24  9:47 ` [PATCH v2] PCI/DPC: Check host->native_dpc before enable " Yicong Yang
@ 2021-07-26 22:05   ` Bjorn Helgaas
  2021-07-28  9:22     ` Yicong Yang
  0 siblings, 1 reply; 12+ messages in thread
From: Bjorn Helgaas @ 2021-07-26 22:05 UTC (permalink / raw)
  To: Yicong Yang
  Cc: linux-pci, sathyanarayanan.kuppuswamy, kbusch, sean.v.kelley,
	qiuxu.zhuo, prime.zeng, linuxarm

On Wed, Feb 24, 2021 at 05:47:58PM +0800, Yicong Yang wrote:
> On 2021/2/3 20:53, Yicong Yang wrote:
> > Per Downstream Port Containment Related Enhancements ECN[1]
> > Table 4-6, Interpretation of _OSC Control Field Returned Value,
> > for bit 7 of _OSC control return value:
> > 
> >   "Firmware sets this bit to 1 to grant the OS control over PCI Express
> >   Downstream Port Containment configuration."
> >   "If control of this feature was requested and denied,
> >   or was not requested, the firmware returns this bit set to 0."
> > 
> > We store bit 7 of _OSC control return value in host->native_dpc,
> > check it before enable the dpc service as the firmware may not
> > grant the control.
> > 
> > [1] Downstream Port Containment Related Enhancements ECN,
> >     Jan 28, 2019, affecting PCI Firmware Specification, Rev. 3.2
> >     https://members.pcisig.com/wg/PCI-SIG/document/12888
> > 
> > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> > ---
> > Change since v1:
> > - use correct reference for _OSC control return value
> > 
> >  drivers/pci/pcie/portdrv_core.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> > index e1fed664..7445d03 100644
> > --- a/drivers/pci/pcie/portdrv_core.c
> > +++ b/drivers/pci/pcie/portdrv_core.c
> > @@ -253,7 +253,8 @@ static int get_port_device_capability(struct pci_dev *dev)
> >  	 */
> >  	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
> >  	    pci_aer_available() &&
> > -	    (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
> > +	    (pcie_ports_dpc_native ||
> > +	    ((services & PCIE_PORT_SERVICE_AER) && host->native_dpc)))
> >  		services |= PCIE_PORT_SERVICE_DPC;
> >  
> 
> the check here maybe problematic. the bit 7 of _OSC return value is reserved
> previously and the change here may break the backward compatibility.
> currently we make dpc enabled along with aer, which can ensure the native
> dpc won't be enabled if the edr is enabled.

Since you think this is problematic, I'll drop it for now.  If you
decide it's the right thing to do, please post it again.

> i feel a bit confused as the bit 7 is not used.
> does it provide a way to enable native dpc regardless of aer ownership?
> just as pcie_ports=dpc-native does when i checked the discussion in [1].
> 
> [1] https://lore.kernel.org/linux-pci/20191023192205.97024-1-olof@lixom.net/
> 
> Thanks,
> Yicong
> 
> >  	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
> > 
> 

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

* Re: [PATCH v2] PCI/DPC: Check host->native_dpc before enable dpc service
  2021-07-26 22:05   ` Bjorn Helgaas
@ 2021-07-28  9:22     ` Yicong Yang
  0 siblings, 0 replies; 12+ messages in thread
From: Yicong Yang @ 2021-07-28  9:22 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: linux-pci, sathyanarayanan.kuppuswamy, kbusch, sean.v.kelley,
	qiuxu.zhuo, prime.zeng, linuxarm

On 2021/7/27 6:05, Bjorn Helgaas wrote:
> On Wed, Feb 24, 2021 at 05:47:58PM +0800, Yicong Yang wrote:
>> On 2021/2/3 20:53, Yicong Yang wrote:
>>> Per Downstream Port Containment Related Enhancements ECN[1]
>>> Table 4-6, Interpretation of _OSC Control Field Returned Value,
>>> for bit 7 of _OSC control return value:
>>>
>>>   "Firmware sets this bit to 1 to grant the OS control over PCI Express
>>>   Downstream Port Containment configuration."
>>>   "If control of this feature was requested and denied,
>>>   or was not requested, the firmware returns this bit set to 0."
>>>
>>> We store bit 7 of _OSC control return value in host->native_dpc,
>>> check it before enable the dpc service as the firmware may not
>>> grant the control.
>>>
>>> [1] Downstream Port Containment Related Enhancements ECN,
>>>     Jan 28, 2019, affecting PCI Firmware Specification, Rev. 3.2
>>>     https://members.pcisig.com/wg/PCI-SIG/document/12888
>>>
>>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>>> ---
>>> Change since v1:
>>> - use correct reference for _OSC control return value
>>>
>>>  drivers/pci/pcie/portdrv_core.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
>>> index e1fed664..7445d03 100644
>>> --- a/drivers/pci/pcie/portdrv_core.c
>>> +++ b/drivers/pci/pcie/portdrv_core.c
>>> @@ -253,7 +253,8 @@ static int get_port_device_capability(struct pci_dev *dev)
>>>  	 */
>>>  	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
>>>  	    pci_aer_available() &&
>>> -	    (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
>>> +	    (pcie_ports_dpc_native ||
>>> +	    ((services & PCIE_PORT_SERVICE_AER) && host->native_dpc)))
>>>  		services |= PCIE_PORT_SERVICE_DPC;
>>>  
>>
>> the check here maybe problematic. the bit 7 of _OSC return value is reserved
>> previously and the change here may break the backward compatibility.
>> currently we make dpc enabled along with aer, which can ensure the native
>> dpc won't be enabled if the edr is enabled.
> 
> Since you think this is problematic, I'll drop it for now.  If you
> decide it's the right thing to do, please post it again.
> 

yes, I still think this has problem. sorry for the noise.

>> i feel a bit confused as the bit 7 is not used.
>> does it provide a way to enable native dpc regardless of aer ownership?
>> just as pcie_ports=dpc-native does when i checked the discussion in [1].
>>
>> [1] https://lore.kernel.org/linux-pci/20191023192205.97024-1-olof@lixom.net/
>>
>> Thanks,
>> Yicong
>>
>>>  	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
>>>
>>
> 
> .
> 


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

end of thread, other threads:[~2021-07-28  9:22 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-03 12:53 [PATCH v2] PCI/DPC: Check host->native_dpc before enable dpc service Yicong Yang
2021-02-03 12:53 ` [PATCH] PCI/DPC: Disable ERR_COR explicitly for native " Yicong Yang
2021-02-18 17:20   ` Bjorn Helgaas
2021-02-24  8:56     ` Yicong Yang
2021-04-10 15:21   ` Bjorn Helgaas
2021-04-10 19:17     ` Lukas Wunner
2021-04-12  9:32       ` Yicong Yang
2021-04-12  3:32     ` Kuppuswamy, Sathyanarayanan
2021-04-12  9:46       ` Yicong Yang
2021-02-24  9:47 ` [PATCH v2] PCI/DPC: Check host->native_dpc before enable " Yicong Yang
2021-07-26 22:05   ` Bjorn Helgaas
2021-07-28  9:22     ` Yicong Yang

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).