mbox series

[v3,0/2] PCI MCFG consolidation and APEI resource filterin

Message ID YW5OTMz+x8zrsqkF@Dennis-MBP.local (mailing list archive)
Headers show
Series PCI MCFG consolidation and APEI resource filterin | expand

Message

Xuesong Chen Oct. 19, 2021, 4:49 a.m. UTC
Hello All,

The idea of this patch set is very strainforward, it's somehow a refactor
of the original codes to share some ones that they should do. Based on that,
we can resolve the MCFG address access issue in APEI module on x86 in a 
command way instead of the current arch-dependent one, while this issue also
does happen on ARM64 platform.

The logic of the series is very clear(IMO it's even time-wasting to explain that):

Patch #1: Escalating the 'pci_mmcfg_list' and 'pci_mmcfg_region' to the
pci.[c,h] which will shared by all the arches. A common sense, in some degree.

Patch #2: Since the 'pci_mmcfg_list' now can be shared across all arches,
the arch-specific fix method can be replaced by the new solution naturally.

Now the v3 patch has been finalized, can we move forward to the next step? -
either give the concerns/objections or pick it up.

Xuesong Chen (2):
  PCI: MCFG: Consolidate the separate PCI MCFG table entry list
  ACPI: APEI: Filter the PCI MCFG address with an arch-agnostic method

 arch/x86/include/asm/pci_x86.h | 17 +---------------
 arch/x86/pci/mmconfig-shared.c | 30 ----------------------------
 drivers/acpi/apei/apei-base.c  | 45 ++++++++++++++++++++++++++++--------------
 drivers/acpi/pci_mcfg.c        | 34 ++++++++++++-------------------
 drivers/pci/pci.c              |  2 ++
 include/linux/pci.h            | 17 ++++++++++++++++
 6 files changed, 63 insertions(+), 82 deletions(-)

Comments

Bjorn Helgaas Oct. 19, 2021, 3:12 p.m. UTC | #1
On Tue, Oct 19, 2021 at 12:49:16PM +0800, Xuesong Chen wrote:
> Hello All,
> 
> The idea of this patch set is very strainforward, it's somehow a refactor
> of the original codes to share some ones that they should do. Based on that,
> we can resolve the MCFG address access issue in APEI module on x86 in a 
> command way instead of the current arch-dependent one, while this issue also
> does happen on ARM64 platform.
> 
> The logic of the series is very clear(IMO it's even time-wasting to explain that):

If you want people to look at and care about your changes, it is never
a waste of time to explain them.

> Patch #1: Escalating the 'pci_mmcfg_list' and 'pci_mmcfg_region' to the
> pci.[c,h] which will shared by all the arches. A common sense, in some degree.
> 
> Patch #2: Since the 'pci_mmcfg_list' now can be shared across all arches,
> the arch-specific fix method can be replaced by the new solution naturally.
> 
> Now the v3 patch has been finalized, can we move forward to the next step? -
> either give the concerns/objections or pick it up.

It's helpful to your reviewers if you include a note about changes
between v2 and v3, as you did in your v2 0/2 cover letter.

It's also helpful if you thread the series with patches 1 and 2 as
responses to the cover letter.  That makes it easy to download the
patches using b4.  Here's a little more background:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/5.Posting.rst?id=v5.14#n320

> Xuesong Chen (2):
>   PCI: MCFG: Consolidate the separate PCI MCFG table entry list
>   ACPI: APEI: Filter the PCI MCFG address with an arch-agnostic method
> 
>  arch/x86/include/asm/pci_x86.h | 17 +---------------
>  arch/x86/pci/mmconfig-shared.c | 30 ----------------------------
>  drivers/acpi/apei/apei-base.c  | 45 ++++++++++++++++++++++++++++--------------
>  drivers/acpi/pci_mcfg.c        | 34 ++++++++++++-------------------
>  drivers/pci/pci.c              |  2 ++
>  include/linux/pci.h            | 17 ++++++++++++++++
>  6 files changed, 63 insertions(+), 82 deletions(-)
> 
> -- 
> 1.8.3.1
>
Xuesong Chen Oct. 20, 2021, 2:28 a.m. UTC | #2
On 19/10/2021 23:12, Bjorn Helgaas wrote:
> On Tue, Oct 19, 2021 at 12:49:16PM +0800, Xuesong Chen wrote:
>> Hello All,
>>
>> The idea of this patch set is very strainforward, it's somehow a refactor
>> of the original codes to share some ones that they should do. Based on that,
>> we can resolve the MCFG address access issue in APEI module on x86 in a 
>> command way instead of the current arch-dependent one, while this issue also
>> does happen on ARM64 platform.
>>
>> The logic of the series is very clear(IMO it's even time-wasting to explain that):
> 
> If you want people to look at and care about your changes, it is never
> a waste of time to explain them.

En, very good point and professional, I'll keep in mind ;-)
> 
>> Patch #1: Escalating the 'pci_mmcfg_list' and 'pci_mmcfg_region' to the
>> pci.[c,h] which will shared by all the arches. A common sense, in some degree.
>>
>> Patch #2: Since the 'pci_mmcfg_list' now can be shared across all arches,
>> the arch-specific fix method can be replaced by the new solution naturally.
>>
>> Now the v3 patch has been finalized, can we move forward to the next step? -
>> either give the concerns/objections or pick it up.
> 
> It's helpful to your reviewers if you include a note about changes
> between v2 and v3, as you did in your v2 0/2 cover letter.
> 
> It's also helpful if you thread the series with patches 1 and 2 as
> responses to the cover letter.  That makes it easy to download the
> patches using b4.  Here's a little more background:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/5.Posting.rst?id=v5.14#n320

OK, I will rewrite it in the next version...
> 
>> Xuesong Chen (2):
>>   PCI: MCFG: Consolidate the separate PCI MCFG table entry list
>>   ACPI: APEI: Filter the PCI MCFG address with an arch-agnostic method
>>
>>  arch/x86/include/asm/pci_x86.h | 17 +---------------
>>  arch/x86/pci/mmconfig-shared.c | 30 ----------------------------
>>  drivers/acpi/apei/apei-base.c  | 45 ++++++++++++++++++++++++++++--------------
>>  drivers/acpi/pci_mcfg.c        | 34 ++++++++++++-------------------
>>  drivers/pci/pci.c              |  2 ++
>>  include/linux/pci.h            | 17 ++++++++++++++++
>>  6 files changed, 63 insertions(+), 82 deletions(-)
>>
>> -- 
>> 1.8.3.1
>>
Xuesong Chen Oct. 27, 2021, 8:10 a.m. UTC | #3
The issue of commit d91525eb8ee6 ("ACPI, EINJ: Enhance error injection tolerance
level") on x86 is also happened on our own ARM64 platform. We sent a patch[1]
trying to fix this issue in an arch-specific way as x86 does at first, but
according to the suggestion from Lorenzo Pieralisi and Catalin Marinas, we can
consolidate the PCI MCFG part then fix it in a more common way, that's why this
patch series comes.

[1] https://marc.info/?l=linux-arm-kernel&m=163108478627166&w=2

---
Change from v3 to v4:
  - Add a new patch (patch #3) to address the quirk ECAM access issue. Because
    the normal ECAM config space can be accessed in a lockless way, so we don't
    need the mutual exclusion with the EINJ action. But those quirks maybe break
    this rule and corrupt the configuration access, reserve its MCFG address
    regions in this case to avoid that happens. 
  
  - Add another patch (patch #4) to log the PCI MCFG entry parse message per
    the suggestion from Bjorn Helgaas. The output on ARM64 as:
    ACPI: MCFG entry for domain 0000 [bus 00-0f] at [mem 0x50000000-0x50ffffff] (base 0x50000000)
  
  - Commit message updated with more details of patch #2

Change from v2 to v3:
  - Address the comments of Lorenzo Pieralisi about the CONFIG_PCI
    dependence issue in APEI module (patch #2)

Change from v1 to v2:
  - Fix the "undefined reference to `pci_mmcfg_list'" build error in case
    of PCI_CONFIG=n, reported by the kernel test robot

Xuesong Chen (4):
  PCI: MCFG: Consolidate the separate PCI MCFG table entry list
  ACPI: APEI: Filter the PCI MCFG address with an arch-agnostic method
  ACPI: APEI: Reserve the MCFG address for quirk ECAM implementation
  PCI: MCFG: Add the MCFG entry parse log message

 arch/x86/include/asm/pci_x86.h | 17 +----------
 arch/x86/pci/mmconfig-shared.c | 30 -------------------
 drivers/acpi/apei/apei-base.c  | 68 ++++++++++++++++++++++++++++++++----------
 drivers/acpi/pci_mcfg.c        | 46 +++++++++++++++-------------
 drivers/pci/pci.c              |  2 ++
 drivers/pci/quirks.c           |  2 ++
 include/linux/pci.h            | 18 +++++++++++
 7 files changed, 101 insertions(+), 82 deletions(-)
Xuesong Chen Nov. 1, 2021, 2:18 a.m. UTC | #4
How about the status of this series, it's really bad, bad and still bad... to wait long
time for the final judgement, especially you take extremely serious to rework it round
by round, finaly you receive nothing. Everyone's work should be repected!

Technically, I don't think it's very hard to say yes or no (what's your concerns) for
the patch set. If you give your objections and convince me, then I will drop it, that's
nothing. Hopefully our maintainers can take the responsibility that they should take,
I totally understand that our maintainers are very busy and will face tens of thousands
of mails. But, YOU ARE THE MAINTAINER! 

Responsiblity!Responsiblity!! Still TMD f*cking FucResponsiblity!!!

On 27/10/2021 16:10, Xuesong Chen wrote:
> The issue of commit d91525eb8ee6 ("ACPI, EINJ: Enhance error injection tolerance
> level") on x86 is also happened on our own ARM64 platform. We sent a patch[1]
> trying to fix this issue in an arch-specific way as x86 does at first, but
> according to the suggestion from Lorenzo Pieralisi and Catalin Marinas, we can
> consolidate the PCI MCFG part then fix it in a more common way, that's why this
> patch series comes.
> 
> [1] https://marc.info/?l=linux-arm-kernel&m=163108478627166&w=2
> 
> ---
> Change from v3 to v4:
>   - Add a new patch (patch #3) to address the quirk ECAM access issue. Because
>     the normal ECAM config space can be accessed in a lockless way, so we don't
>     need the mutual exclusion with the EINJ action. But those quirks maybe break
>     this rule and corrupt the configuration access, reserve its MCFG address
>     regions in this case to avoid that happens. 
>   
>   - Add another patch (patch #4) to log the PCI MCFG entry parse message per
>     the suggestion from Bjorn Helgaas. The output on ARM64 as:
>     ACPI: MCFG entry for domain 0000 [bus 00-0f] at [mem 0x50000000-0x50ffffff] (base 0x50000000)
>   
>   - Commit message updated with more details of patch #2
> 
> Change from v2 to v3:
>   - Address the comments of Lorenzo Pieralisi about the CONFIG_PCI
>     dependence issue in APEI module (patch #2)
> 
> Change from v1 to v2:
>   - Fix the "undefined reference to `pci_mmcfg_list'" build error in case
>     of PCI_CONFIG=n, reported by the kernel test robot
> 
> Xuesong Chen (4):
>   PCI: MCFG: Consolidate the separate PCI MCFG table entry list
>   ACPI: APEI: Filter the PCI MCFG address with an arch-agnostic method
>   ACPI: APEI: Reserve the MCFG address for quirk ECAM implementation
>   PCI: MCFG: Add the MCFG entry parse log message
> 
>  arch/x86/include/asm/pci_x86.h | 17 +----------
>  arch/x86/pci/mmconfig-shared.c | 30 -------------------
>  drivers/acpi/apei/apei-base.c  | 68 ++++++++++++++++++++++++++++++++----------
>  drivers/acpi/pci_mcfg.c        | 46 +++++++++++++++-------------
>  drivers/pci/pci.c              |  2 ++
>  drivers/pci/quirks.c           |  2 ++
>  include/linux/pci.h            | 18 +++++++++++
>  7 files changed, 101 insertions(+), 82 deletions(-)
>
Will Deacon Nov. 1, 2021, 9:36 a.m. UTC | #5
Hi,

On Mon, Nov 01, 2021 at 10:18:35AM +0800, Xuesong Chen wrote:
> How about the status of this series, it's really bad, bad and still bad... to wait long
> time for the final judgement, especially you take extremely serious to rework it round
> by round, finaly you receive nothing. Everyone's work should be repected!

I've trimmed the rest of your response as it wasn't especially constructive.
Please can you try to keep things civil, even when you're frustrated? It's
not very pleasant being on the end of a rant.

One likely explanation for you not getting a reply on your patches is that
I've discovered many of your emails have ended up in my spam, for some
reason. I'm using gmail for my inbox so, if Bjorn is doing that as well,
then there's a good chance he hasn't seen them either.

The other thing to keep in mind is that the 5.16 merge window opened today
and you posted the latest version of your patches on Wednesday. That doesn't
really leave enough time for the patches to be reviewed (noting that patch 3
is new in this version and the kernel build robot was still complaining on
Friday), queued and put into linux-next, so I would suspect that this series
is looking more like 5.17 material and therefore not a priority for
maintainers at the moment.

Your best is probably to post a v5, with the kbuild warnings addressed,
when -rc1 is released in a couple of weeks. I'm not sure how to fix the
spam issue though :(

Will
Xuesong Chen Nov. 1, 2021, 12:12 p.m. UTC | #6
Hi Will,

Thanks for the feedback!

On 01/11/2021 17:36, Will Deacon wrote:
> Hi,
> 
> On Mon, Nov 01, 2021 at 10:18:35AM +0800, Xuesong Chen wrote:
>> How about the status of this series, it's really bad, bad and still bad... to wait long
>> time for the final judgement, especially you take extremely serious to rework it round
>> by round, finaly you receive nothing. Everyone's work should be repected!
> 
> I've trimmed the rest of your response as it wasn't especially constructive.
> Please can you try to keep things civil, even when you're frustrated? It's
> not very pleasant being on the end of a rant.

I'm very sorry about the non-constructived response, and I'd like to take this chance to
withdraw them entirely... personally this is not a good example in terms of the mood or
the way of expression.

> 
> One likely explanation for you not getting a reply on your patches is that
> I've discovered many of your emails have ended up in my spam, for some
> reason. I'm using gmail for my inbox so, if Bjorn is doing that as well,
> then there's a good chance he hasn't seen them either.
> 
> The other thing to keep in mind is that the 5.16 merge window opened today
> and you posted the latest version of your patches on Wednesday. That doesn't
> really leave enough time for the patches to be reviewed (noting that patch 3
> is new in this version and the kernel build robot was still complaining on
> Friday), queued and put into linux-next, so I would suspect that this series
> is looking more like 5.17 material and therefore not a priority for
> maintainers at the moment.
> 
> Your best is probably to post a v5, with the kbuild warnings addressed,
> when -rc1 is released in a couple of weeks. I'm not sure how to fix the
> spam issue though :(

I've noticed the kbuild warning by the robot, so I plan to fix it and post the v5 soon.

Thanks,
Xuesong

> 
> Will
>
Borislav Petkov Nov. 1, 2021, 12:22 p.m. UTC | #7
On Mon, Nov 01, 2021 at 08:12:15PM +0800, Xuesong Chen wrote:
> I'm very sorry about the non-constructived response, and I'd like to
> take this chance to withdraw them entirely... personally this is not a
> good example in terms of the mood or the way of expression.

Good idea. There are other maintainers who would ignore you indefinitely
for uncalled for explosions like that. And then you would have achieved
the opposite of what you were aiming for, with that rant.

To Will's point, you can always read Documentation/process/ while
waiting for your patches to get reviewed - there the whole process is
explained and what the best ways and times are to send a patchset.

HTH.
Xuesong Chen Nov. 1, 2021, 1:32 p.m. UTC | #8
On 01/11/2021 20:22, Borislav Petkov wrote:
> On Mon, Nov 01, 2021 at 08:12:15PM +0800, Xuesong Chen wrote:
>> I'm very sorry about the non-constructived response, and I'd like to
>> take this chance to withdraw them entirely... personally this is not a
>> good example in terms of the mood or the way of expression.
> 
> Good idea. There are other maintainers who would ignore you indefinitely
> for uncalled for explosions like that. And then you would have achieved
> the opposite of what you were aiming for, with that rant.

Actually that's my original intention, especially when you take lots of serious
effors to rework it round by round, but no one say YES or NO, which is really
frustrating. Hopefully the newbies can also be treated fairly in the community.
During my working experience in the community before, lot's of nice maintainers
give me very deep impression. What a splendid memory!

> 
> To Will's point, you can always read Documentation/process/ while
> waiting for your patches to get reviewed - there the whole process is
> explained and what the best ways and times are to send a patchset.

Good suggestion, learnt and thanks!

Thanks,
Xuesong
> 
> HTH.
>
Borislav Petkov Nov. 1, 2021, 1:55 p.m. UTC | #9
On Mon, Nov 01, 2021 at 09:32:31PM +0800, Xuesong Chen wrote:
> Actually that's my original intention

There's a misunderstanding here - I don't think your original intention
is to get ignored indefinitely.

> especially when you take lots of serious effors to rework it round by
> round, but no one say YES or NO, which is really frustrating.

Well, try to put yourself in the maintainer's shoes, maybe that would
answer some of that frustration:

- Most of the maintainers are overworked and backlogged until forever.

- If you rework something and you don't get an answer, maybe the
maintainer is not sure yet and is thinking about the pros and cons of
taking that patch.

Greg has formulated this particular issue of the maintainers very
nicely:

"Seriously. It's easier for the maintainer to not accept your code at
all. To accept it, it takes time to review it, apply it, send it on up
the development chain, handle any problems that might happen with the
patch, accept responsibility for the patch, possibly fix any problems
that happen later on when you disappear, and maintain it for the next 20
years.

That's a lot of work that you are asking someone else to do on your
behalf…

So your goal is, when sending a patch, to give me no excuse to not
accept it. To make it such that if I ignore it, or reject it, I am the
one that is the problem here, not you."

And this thing is not really clear to all submitters - once their
patch(es) is applied, they're done. But maintainers have to deal with
that code forever.

So before you send your patchset, try to think as a maintainer and
think whether your change makes sense for the *whole* tree and whether
maintaining it forward would be easy.

- Did I say that maintainers are overworked?

Submitters don't see the amount of work maintainers do in the
background, testing everything and fixing build issues and bugs. Because
most of the time, submitters submit and the cleanups and bugs get mopped
after them by the maintainers - not the submitters.

Look at how some trees resort to maintainer *groups* because a single
maintainer simply doesn't scale, at the risk of a burnout or whatever
nasty.

And those maintainer groups have *all* their hands full.

> Hopefully the newbies can also be treated fairly in the community.

Newbies are treated fairly in the community - especially those who come
prepared and try to understand why the maintainers say things they way
they do and listen to feedback.

If there are examples against that, we would all like to know about
them.

I sincerely hope that explains the situation and hope that it'll help
you see it from the maintainers' POV too and maybe help you deal with
future submissions a lot better.

Thx.