| Message ID | 20220517210819.24114-4-michael.reed@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | DPC Fixes for Failure Cases of DownPort Containment events | expand |
On 5/17/22 15:08, Michael Reed wrote: > From: Michael Reed <Michael.Reed@canonical.com> > > > BugLink: 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. 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
On 18.05.22 14:00, Tim Gardner wrote: > > > On 5/17/22 15:08, Michael Reed wrote: >> From: Michael Reed <Michael.Reed@canonical.com> >> >> >> BugLink: 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 >
On 18.05.22 16:20, Stefan Bader wrote: > On 18.05.22 14:00, Tim Gardner wrote: >> >> >> On 5/17/22 15:08, Michael Reed wrote: >>> From: Michael Reed <Michael.Reed@canonical.com> >>> >>> >>> BugLink: 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...) Hi Michael, Could you please provide the additional information requested above? As mentioned, we can add such info when applying so you don't need to send another version of the patchset. Thanks, Kleber > > -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 >> > >
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. 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. Reference: 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@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@canonical.com> > >> > >> > >> BugLink: 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 > > > >
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@canonical.com > <mailto:stefan.bader@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@canonical.com > <mailto:Michael.Reed@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 > > >
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'}> # 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
From: Michael Reed <Michael.Reed@canonical.com> BugLink: 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(-)