diff mbox

[Linaro-acpi,v5,0/5] Provide better MADT subtable sanity checks

Message ID 561EB923.6010109@redhat.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Al Stone Oct. 14, 2015, 8:20 p.m. UTC
On 10/12/2015 10:06 PM, Pat Erley wrote:
> On 10/12/2015 01:52 PM, Al Stone wrote:
>> On 10/11/2015 09:58 PM, Pat Erley wrote:
>>> On 10/11/2015 08:49 PM, Hanjun Guo wrote:
>>>> On 10/12/2015 11:08 AM, Pat Erley wrote:
>>>>> On 10/05/2015 10:12 AM, Al Stone wrote:
>>>>>> On 10/05/2015 07:39 AM, Rafael J. Wysocki wrote:
>>>>>>> On Wednesday, September 30, 2015 10:10:16 AM Al Stone wrote:
>>>>>>>> On 09/30/2015 03:00 AM, Hanjun Guo wrote:
>>>>>>>>> On 2015/9/30 7:45, Al Stone wrote:
>>>>>>>>>> NB: this patch set is for use against the linux-pm bleeding edge
>>>>>>>>>> branch.
>>>>>>>>>>
>>>>>>>>
>>>>>>>> [snip...]
>>>>>>>>
>>>>>>>>>
>>>>>>>>> For this patch set,
>>>>>>>>>
>>>>>>>>> Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> Hanjun
>>>>>>>>
>>>>>>>> Thanks, Hanjun!
>>>>>>>
>>>>>>> Series applied, thanks!
>>>>>>>
>>>>>>> Rafael
>>>>>>>
>>>>>>
>>>>>> Thanks, Rafael!
>>>>>>
>>>>>
>>>>> Just decided to test out linux-next (to see the new nouveau cleanups).
>>>>> This change set prevents my Lenovo W510 from booting properly.
>>>>>
>>>>> Reverting: 7494b0 "ACPI: add in a bad_madt_entry() function to
>>>>> eventually replace the macro"
>>>>>
>>>>> Gets the system booting again.  I'm attaching my dmesg from the failed
>>>>> boot, who wants the acpidump?
>>>>
>>>> [    0.000000] ACPI: undefined version for either FADT 4.0 or MADT 1
>>>> [    0.000000] ACPI: Error parsing LAPIC address override entry
>>>> [    0.000000] ACPI: Invalid BIOS MADT, disabling ACPI
>>>>
>>>> Seems the MADT revision is not right, could you dump the ACPI MADT
>>>> (APIC) table and send it out? I will take a look :)
>>>>
>>>> Thanks
>>>> Hanjun
>>>
>>> Here ya go, enjoy.  Feel free to CC me on any patches that might fix it.
>>
>> Pat,
>>
>> Would you mind sending a copy of the FADT, also, please?  The first of the
>> ACPI messages is a check of version correspondence between the FADT and MADT,
>> while the second message is from looking at just an MADT subtable.  Thanks
>> for sending the MADT out -- that helps me quite a lot in thinking this through.
>>
>> BTW, whoever is providing the BIOS (Lenovo, I assume) may want to have a look
>> at these, also:
>>
>> [    0.000000] ACPI BIOS Warning (bug): 32/64X length mismatch in
>> FADT/Pm1aControlBlock: 16/32 (20150818/tbfadt-623)
>> [    0.000000] ACPI BIOS Warning (bug): Invalid length for
>> FADT/Pm1aControlBlock: 32, using default 16 (20150818/tbfadt-704)
>>
>> Not inherently dangerous, but definitely sloppy and mind-numbingly easy to
>> avoid, IIRC.
>>
> 
> Here ya go.

Okay.  There's just a lot of weird stuff out there in ACPI-land.  I've
attached four minor fixes for the special cases that have been reported
(well, the last one is actually a fix for a typo in the spec, but just
the same...).

These should apply on top of linux-next; would you mind trying them out
to make sure I didn't break anything else on your laptop?  If they behave
as I hope they will, I think I'll have covered all the places where the
checking of MADT subtables needs to be be relaxed a bit.  These work for
me on arm64, but if they work for you and a couple of other testers, then
I'll send them to Rafael properly.

Many thanks!

Comments

Rafael J. Wysocki Oct. 14, 2015, 9:25 p.m. UTC | #1
On Wednesday, October 14, 2015 02:20:51 PM Al Stone wrote:
> This is a multi-part message in MIME format.
> --------------020400080004050109020606
> Content-Type: text/plain; charset=utf-8
> Content-Transfer-Encoding: 7bit
> 
> On 10/12/2015 10:06 PM, Pat Erley wrote:
> > On 10/12/2015 01:52 PM, Al Stone wrote:
> >> On 10/11/2015 09:58 PM, Pat Erley wrote:
> >>> On 10/11/2015 08:49 PM, Hanjun Guo wrote:
> >>>> On 10/12/2015 11:08 AM, Pat Erley wrote:
> >>>>> On 10/05/2015 10:12 AM, Al Stone wrote:
> >>>>>> On 10/05/2015 07:39 AM, Rafael J. Wysocki wrote:
> >>>>>>> On Wednesday, September 30, 2015 10:10:16 AM Al Stone wrote:
> >>>>>>>> On 09/30/2015 03:00 AM, Hanjun Guo wrote:
> >>>>>>>>> On 2015/9/30 7:45, Al Stone wrote:
> >>>>>>>>>> NB: this patch set is for use against the linux-pm bleeding edge
> >>>>>>>>>> branch.
> >>>>>>>>>>
> >>>>>>>>
> >>>>>>>> [snip...]
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>> For this patch set,
> >>>>>>>>>
> >>>>>>>>> Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org>
> >>>>>>>>>
> >>>>>>>>> Thanks
> >>>>>>>>> Hanjun
> >>>>>>>>
> >>>>>>>> Thanks, Hanjun!
> >>>>>>>
> >>>>>>> Series applied, thanks!
> >>>>>>>
> >>>>>>> Rafael
> >>>>>>>
> >>>>>>
> >>>>>> Thanks, Rafael!
> >>>>>>
> >>>>>
> >>>>> Just decided to test out linux-next (to see the new nouveau cleanups).
> >>>>> This change set prevents my Lenovo W510 from booting properly.
> >>>>>
> >>>>> Reverting: 7494b0 "ACPI: add in a bad_madt_entry() function to
> >>>>> eventually replace the macro"
> >>>>>
> >>>>> Gets the system booting again.  I'm attaching my dmesg from the failed
> >>>>> boot, who wants the acpidump?
> >>>>
> >>>> [    0.000000] ACPI: undefined version for either FADT 4.0 or MADT 1
> >>>> [    0.000000] ACPI: Error parsing LAPIC address override entry
> >>>> [    0.000000] ACPI: Invalid BIOS MADT, disabling ACPI
> >>>>
> >>>> Seems the MADT revision is not right, could you dump the ACPI MADT
> >>>> (APIC) table and send it out? I will take a look :)
> >>>>
> >>>> Thanks
> >>>> Hanjun
> >>>
> >>> Here ya go, enjoy.  Feel free to CC me on any patches that might fix it.
> >>
> >> Pat,
> >>
> >> Would you mind sending a copy of the FADT, also, please?  The first of the
> >> ACPI messages is a check of version correspondence between the FADT and MADT,
> >> while the second message is from looking at just an MADT subtable.  Thanks
> >> for sending the MADT out -- that helps me quite a lot in thinking this through.
> >>
> >> BTW, whoever is providing the BIOS (Lenovo, I assume) may want to have a look
> >> at these, also:
> >>
> >> [    0.000000] ACPI BIOS Warning (bug): 32/64X length mismatch in
> >> FADT/Pm1aControlBlock: 16/32 (20150818/tbfadt-623)
> >> [    0.000000] ACPI BIOS Warning (bug): Invalid length for
> >> FADT/Pm1aControlBlock: 32, using default 16 (20150818/tbfadt-704)
> >>
> >> Not inherently dangerous, but definitely sloppy and mind-numbingly easy to
> >> avoid, IIRC.
> >>
> > 
> > Here ya go.
> 
> Okay.  There's just a lot of weird stuff out there in ACPI-land.  I've
> attached four minor fixes for the special cases that have been reported
> (well, the last one is actually a fix for a typo in the spec, but just
> the same...).
> 
> These should apply on top of linux-next; would you mind trying them out
> to make sure I didn't break anything else on your laptop?  If they behave
> as I hope they will, I think I'll have covered all the places where the
> checking of MADT subtables needs to be be relaxed a bit.  These work for
> me on arm64, but if they work for you and a couple of other testers, then
> I'll send them to Rafael properly.

Well, you might as well submit them properly right away, so I could pick
them up and put them into my linux-next branch, which then might make it
easier for some people to test them.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Al Stone Oct. 14, 2015, 9:27 p.m. UTC | #2
On 10/14/2015 03:25 PM, Rafael J. Wysocki wrote:
> On Wednesday, October 14, 2015 02:20:51 PM Al Stone wrote:
>> This is a multi-part message in MIME format.
>> --------------020400080004050109020606
>> Content-Type: text/plain; charset=utf-8
>> Content-Transfer-Encoding: 7bit
>>
>> On 10/12/2015 10:06 PM, Pat Erley wrote:
>>> On 10/12/2015 01:52 PM, Al Stone wrote:
>>>> On 10/11/2015 09:58 PM, Pat Erley wrote:
>>>>> On 10/11/2015 08:49 PM, Hanjun Guo wrote:
>>>>>> On 10/12/2015 11:08 AM, Pat Erley wrote:
>>>>>>> On 10/05/2015 10:12 AM, Al Stone wrote:
>>>>>>>> On 10/05/2015 07:39 AM, Rafael J. Wysocki wrote:
>>>>>>>>> On Wednesday, September 30, 2015 10:10:16 AM Al Stone wrote:
>>>>>>>>>> On 09/30/2015 03:00 AM, Hanjun Guo wrote:
>>>>>>>>>>> On 2015/9/30 7:45, Al Stone wrote:
>>>>>>>>>>>> NB: this patch set is for use against the linux-pm bleeding edge
>>>>>>>>>>>> branch.
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> [snip...]
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> For this patch set,
>>>>>>>>>>>
>>>>>>>>>>> Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>>>>>>>>>
>>>>>>>>>>> Thanks
>>>>>>>>>>> Hanjun
>>>>>>>>>>
>>>>>>>>>> Thanks, Hanjun!
>>>>>>>>>
>>>>>>>>> Series applied, thanks!
>>>>>>>>>
>>>>>>>>> Rafael
>>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks, Rafael!
>>>>>>>>
>>>>>>>
>>>>>>> Just decided to test out linux-next (to see the new nouveau cleanups).
>>>>>>> This change set prevents my Lenovo W510 from booting properly.
>>>>>>>
>>>>>>> Reverting: 7494b0 "ACPI: add in a bad_madt_entry() function to
>>>>>>> eventually replace the macro"
>>>>>>>
>>>>>>> Gets the system booting again.  I'm attaching my dmesg from the failed
>>>>>>> boot, who wants the acpidump?
>>>>>>
>>>>>> [    0.000000] ACPI: undefined version for either FADT 4.0 or MADT 1
>>>>>> [    0.000000] ACPI: Error parsing LAPIC address override entry
>>>>>> [    0.000000] ACPI: Invalid BIOS MADT, disabling ACPI
>>>>>>
>>>>>> Seems the MADT revision is not right, could you dump the ACPI MADT
>>>>>> (APIC) table and send it out? I will take a look :)
>>>>>>
>>>>>> Thanks
>>>>>> Hanjun
>>>>>
>>>>> Here ya go, enjoy.  Feel free to CC me on any patches that might fix it.
>>>>
>>>> Pat,
>>>>
>>>> Would you mind sending a copy of the FADT, also, please?  The first of the
>>>> ACPI messages is a check of version correspondence between the FADT and MADT,
>>>> while the second message is from looking at just an MADT subtable.  Thanks
>>>> for sending the MADT out -- that helps me quite a lot in thinking this through.
>>>>
>>>> BTW, whoever is providing the BIOS (Lenovo, I assume) may want to have a look
>>>> at these, also:
>>>>
>>>> [    0.000000] ACPI BIOS Warning (bug): 32/64X length mismatch in
>>>> FADT/Pm1aControlBlock: 16/32 (20150818/tbfadt-623)
>>>> [    0.000000] ACPI BIOS Warning (bug): Invalid length for
>>>> FADT/Pm1aControlBlock: 32, using default 16 (20150818/tbfadt-704)
>>>>
>>>> Not inherently dangerous, but definitely sloppy and mind-numbingly easy to
>>>> avoid, IIRC.
>>>>
>>>
>>> Here ya go.
>>
>> Okay.  There's just a lot of weird stuff out there in ACPI-land.  I've
>> attached four minor fixes for the special cases that have been reported
>> (well, the last one is actually a fix for a typo in the spec, but just
>> the same...).
>>
>> These should apply on top of linux-next; would you mind trying them out
>> to make sure I didn't break anything else on your laptop?  If they behave
>> as I hope they will, I think I'll have covered all the places where the
>> checking of MADT subtables needs to be be relaxed a bit.  These work for
>> me on arm64, but if they work for you and a couple of other testers, then
>> I'll send them to Rafael properly.
> 
> Well, you might as well submit them properly right away, so I could pick
> them up and put them into my linux-next branch, which then might make it
> easier for some people to test them.
> 
> Thanks,
> Rafael
> 

Fair enough.  I was just being overly cautious about possible further breakage.

Done.  Patch series sent to the list.
diff mbox

Patch

From 6d3189ccfcbac9a61e6502df70499bd2ee808509 Mon Sep 17 00:00:00 2001
From: Al Stone <ahs3@redhat.com>
Date: Tue, 13 Oct 2015 16:31:50 -0600
Subject: [PATCH 4/4] ACPI: for bad_madt_entry(), the GIC ITS table is 20 bytes
 long, not 16

Correct a typo where bad_madt_entry() expected the MADT GIC ITS subtable
to be 16 bytes long, but it is actually 20 bytes.  This is a cut'n'paste
error picked up from the spec and inadvertently replicated in code.

Signed-off-by: Al Stone <al.stone@linaro.org>
---
 drivers/acpi/tables.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 790d4b0..1b7c13e 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -292,7 +292,7 @@  void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
  * GICD                  0xc                         24    24    24
  * GICv2m MSI            0xd                               24    24
  * GICR                  0xe                               16    16
- * GIC ITS               0xf                                     16
+ * GIC ITS               0xf                                     20
  *
  * In the table, each length entry is what should be in the length
  * field of the subtable, and -- in general -- it should match the
@@ -366,7 +366,7 @@  static struct acpi_madt_subtable_lengths spec_info[] = {
 		.madt_version = 3,
 		.num_types = 16,
 		.lengths = { 8, 12, 10, 8, 6, 12, 16, SUBTABLE_VARIABLE,
-			     16, 16, 12, 80, 24, 24, 16, 16 }
+			     16, 16, 12, 80, 24, 24, 16, 20 }
 	},
 	{ /* terminator */
 		.major_version = 0,
-- 
2.4.3