[SRU][J][PATCH v2 3/3] Enable config option CONFIG_PCIE_EDR

[Stefan Bader]

On 13.06.22 23:49, Michael Reed wrote:
> Hi All,
> 
> I am not sure if it is too late to respond to this thread, if so I can create a 
> V3 of the patch and resubmit it.  Answers are below.

It is never too late to respond. Not sure how quickly this can land if clarified 
but that is due to other reasons.
> 
> What does this option do?
> 
> PCI Express Error Disconnect Recover support
> 
> This option adds Error Disconnect Recover support as specified in the Downstream 
> Port Containment Related Enhancements ECN to the PCI Firmware Specification 
> r3.2. Enable this if you want to support hybrid DPC model which uses both 
> firmware and OS to implement DPC.

Bingo! Sorry, yes, this is an explanation but one which leaves a lot of 
questions marks if one is not following hardware design. I think the simple 
summary, which I would want to add, is that this enables support for the (not 
sure this is on cards or refers to the BIOS) firmware to handle errors on the 
PCI bus in addition to software handling in the Linux driver(s). This either got 
added recently or was not enabled for stability reasons.

Which probably is something that the "regression potential" of the SRU 
justification should say. That OS and firmware might battle over the handling 
and maybe leading to errors no longer be handled correctly.

-Stefan

> 
> Reference:
> https://cateee.net/lkddb/web-lkddb/PCIE_EDR.html 
> <https://cateee.net/lkddb/web-lkddb/PCIE_EDR.html>
> 
> Why is it needed now?
> This option is needed to enable DPC fixes which help in handling some of the 
> failure cases of DownPort Containment events.
> 
>   Why was it not turned on before?
> I do not know why it was not turned on before, I suspect it was disabled by 
> default. I cannot find anywhere in the git logs where it was previously 
> enabled.  I can follow up with Dell for more information if needed.
> 
> 
> Regards,
> Michael
> 
> 
> On Wed, May 18, 2022 at 9:20 AM Stefan Bader <stefan.bader at canonical.com 
> <mailto:stefan.bader at canonical.com>> wrote:
> 
>     On 18.05.22 14:00, Tim Gardner wrote:
>      >
>      >
>      > On 5/17/22 15:08, Michael Reed wrote:
>      >> From: Michael Reed <Michael.Reed at canonical.com
>     <mailto:Michael.Reed at canonical.com>>
>      >>
>      >>
>      >> BugLink: https://bugs.launchpad.net/bugs/1965241
>     <https://bugs.launchpad.net/bugs/1965241>
>      >>
>      >> ---
>      >>   debian.master/config/annotations          | 2 +-
>      >>   debian.master/config/config.common.ubuntu | 2 +-
>      >>   2 files changed, 2 insertions(+), 2 deletions(-)
>      >>
>      >> diff --git a/debian.master/config/annotations
>     b/debian.master/config/annotations
>      >> index 15759fa435bd..1a014729b79d 100644
>      >> --- a/debian.master/config/annotations
>      >> +++ b/debian.master/config/annotations
>      >> @@ -7091,7 +7091,7 @@ CONFIG_PCIEAER
>      >> policy<{'amd64': 'y', 'arm64': '
>      >>   CONFIG_PCIEAER_INJECT                           policy<{'amd64': 'n',
>      >> 'arm64': 'n', 'armhf': 'n', 's390x': 'n'}>
>      >>   CONFIG_PCIE_ECRC                                policy<{'amd64': 'n',
>      >> 'arm64': 'n', 'armhf': 'n', 's390x': 'n'}>
>      >>   CONFIG_PCIE_DPC                                 policy<{'amd64': 'y',
>      >> 'arm64': 'y', 'armhf': 'y', 's390x': 'y'}>
>      >> -CONFIG_PCIE_EDR                                 policy<{'amd64': 'n',
>      >> 'arm64': 'n'}>
>      >> +CONFIG_PCIE_EDR                                 policy<{'amd64': 'y',
>      >> 'arm64': 'n'}>
>      >
>      > We generally note why a config change is made by adding a line like this:
>      >
>      > CONFIG_PCIE_EDR mark<ENFORCED> note<LP: #1965241>
>      >
>      > Granted, one could go back in git history to figure it out, but when
>     you're as
>      > lazy as I am this is much quicker.
> 
>     What I also would like to see added (and both the annotation that addition
>     could
>     be done while applying to avoid a full v3) is some additional explanations in
>     the commit message what this option does and why its needed now and if anyone
>     remembers why it was not turned on before (mabye default was no because not
>     stable but it is considered better now. or so...)
> 
>     -Stefan
>      >
>      > rtg
>      >
>      >>   #
>      >>   CONFIG_PCIEAER_INJECT                           flag<TESTING>
>      >> diff --git a/debian.master/config/config.common.ubuntu
>      >> b/debian.master/config/config.common.ubuntu
>      >> index 0fffe06795c0..624831a93860 100644
>      >> --- a/debian.master/config/config.common.ubuntu
>      >> +++ b/debian.master/config/config.common.ubuntu
>      >> @@ -7603,7 +7603,7 @@ CONFIG_PCIE_DW_PLAT=y
>      >>   CONFIG_PCIE_DW_PLAT_EP=y
>      >>   CONFIG_PCIE_DW_PLAT_HOST=y
>      >>   # CONFIG_PCIE_ECRC is not set
>      >> -# CONFIG_PCIE_EDR is not set
>      >> +CONFIG_PCIE_EDR=y
>      >>   CONFIG_PCIE_HISI_ERR=y
>      >>   CONFIG_PCIE_HISI_STB=y
>      >>   CONFIG_PCIE_IPROC=m
>      >
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20220614/9076a9f2/attachment.sig>