linux-kernel - RE: [PATCH v3 1/2] acpi/x86: Fix improper handling of SCI INT for platforms supporting only IOAPIC mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CS1PR8401MB0518E69621848A5BFB398B2CD92F0@CS1PR8401MB0518.NAMPRD84.PROD.OUTLOOK.COM>
Date:   Fri, 17 Nov 2017 14:00:14 +0000
From:   "Sajjan, Vikas C" <vikas.cha.sajjan@....com>
To:     Thomas Gleixner <tglx@...utronix.de>
CC:     "rjw@...ysocki.net" <rjw@...ysocki.net>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kkamagui@...il.com" <kkamagui@...il.com>,
        "mingo@...nel.org" <mingo@...nel.org>
Subject: RE: [PATCH v3 1/2] acpi/x86: Fix improper handling of SCI INT for
 platforms supporting only IOAPIC mode

On Thu, 16 Nov 2017, Vikas C Sajjan wrote:

> The platforms which support only IOAPIC mode, pass the SCI information 
> above the legacy space (0-15) via the FADT mechanism and not via MADT.
> In such cases the mp_override_legacy_irq() used by 
> acpi_sci_ioapic_setup() to register SCI interrupts fails for 
> interrupts >= 16, since it is meant to handle only legacy space and 
> throws error "Invalid bus_irq %u for legacy override". Hence add a new 
> function to handle SCI interrupts >= 16 and invoke it conditionally in 
> acpi_sci_ioapic_setup().The code duplication due to this new function will be cleaned up in a separate patch.

This reads way better, but I have a small nit pick. In the example I gave you there were multiple paragraphs on purpose to separate the different parts. So if I just split the above lump into separate paragraphs:

[1]
   The platforms which support only IOAPIC mode, pass the SCI information
   above the legacy space (0-15) via the FADT mechanism and not via MADT.

[2]
   In such cases the mp_override_legacy_irq() used by acpi_sci_ioapic_setup()
   to register SCI interrupts fails for interrupts >= 16, since it is meant to
   handle only legacy space and throws error "Invalid bus_irq %u for legacy
   override".

[3]
   Hence add a new function to handle SCI interrupts >= 16 and
   invoke it conditionally in acpi_sci_ioapic_setup().

[4]
   The code duplication due to this new function will be cleaned up in a
   separate patch.

then this is clearly structured:

[1] describes the context.

[2] describes the failure

[3] describes the solution

[4] is an extra note to tell the reviewer/reader that you are aware of the
    code duplication and this is addressed later. 

No need to resend. I can do that when picking it up.
Thanks.

> Co-developed-by: Sunil V L <sunil.vl@....com>

I had a discussion with Greg about this tag which resulted in a patch so it should be soon part of the official documentation:

 https://lkml.kernel.org/r/20171116132309.GA8449@kroah.com

Great. Good to know that.

We agreed that both authors should add their Signed-off-by to document that the work conforms with the Developer Certificate of Origin. I'll add that if that's ok for you.

I am OK with that. Please go ahead.

Thanks for following up!

Thank you for the review.

       tglx