[5.12 regression] DSDT overriding from initrd no longer works

[5.12 regression] DSDT overriding from initrd no longer works

[Hans de Goede]

Hi Rafael,

Sorry about the timing of reporting this regression.

I just noticed that overriding the DSDT (*) from the initrd will not work in 5.12,
this is caused by:

commit 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by ACPI tables")

This makes the initial acpi_locate_initial_tables() call happen earlier
then before, but the acpi_table_upgrade) call in arch/x86/kernel/setup.c is
not moved up, so the tables in the initrd are now only parsed and saved
after the initial ACPI table scanning has already been done.

I guess fixing this might be as easy as moving the acpi_table_upgrade) call
higher in arch/x86/kernel/setup.c but I'm not sure if that is save to do.

I've several devices with ACPI tables which are so broken that they need
an override. I've reverted the commit from my personal tree for now which
avoids the regression.

Regards,

Hans

Re: [5.12 regression] DSDT overriding from initrd no longer works

[Rafael J. Wysocki]

On Mon, Apr 12, 2021 at 7:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Rafael,
>
> Sorry about the timing of reporting this regression.

Oh well.

> I just noticed that overriding the DSDT (*) from the initrd will not work in 5.12,
> this is caused by:
>
> commit 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by ACPI tables")
>
> This makes the initial acpi_locate_initial_tables() call happen earlier
> then before, but the acpi_table_upgrade) call in arch/x86/kernel/setup.c is
> not moved up, so the tables in the initrd are now only parsed and saved
> after the initial ACPI table scanning has already been done.
>
> I guess fixing this might be as easy as moving the acpi_table_upgrade) call
> higher in arch/x86/kernel/setup.c but I'm not sure if that is save to do.

Why do you think it may not be safe?

Have you tried to do that?

Re: [5.12 regression] DSDT overriding from initrd no longer works

[Rafael J. Wysocki]

On Mon, Apr 12, 2021 at 8:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Apr 12, 2021 at 7:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Hi Rafael,
> >
> > Sorry about the timing of reporting this regression.
>
> Oh well.
>
> > I just noticed that overriding the DSDT (*) from the initrd will not work in 5.12,
> > this is caused by:
> >
> > commit 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by ACPI tables")
> >
> > This makes the initial acpi_locate_initial_tables() call happen earlier
> > then before, but the acpi_table_upgrade) call in arch/x86/kernel/setup.c is
> > not moved up, so the tables in the initrd are now only parsed and saved
> > after the initial ACPI table scanning has already been done.
> >
> > I guess fixing this might be as easy as moving the acpi_table_upgrade) call
> > higher in arch/x86/kernel/setup.c but I'm not sure if that is save to do.
>
> Why do you think it may not be safe?

OK, so it won't work in some cases, because acpi_table_upgrade() needs
to be called after reserve_initrd(), so I guess the commit above will
need to be reverted.

Re: [5.12 regression] DSDT overriding from initrd no longer works

[Hans de Goede]

Hi,

On 4/12/21 8:11 PM, Rafael J. Wysocki wrote:
> On Mon, Apr 12, 2021 at 8:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Mon, Apr 12, 2021 at 7:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi Rafael,
>>>
>>> Sorry about the timing of reporting this regression.
>>
>> Oh well.
>>
>>> I just noticed that overriding the DSDT (*) from the initrd will not work in 5.12,
>>> this is caused by:
>>>
>>> commit 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by ACPI tables")
>>>
>>> This makes the initial acpi_locate_initial_tables() call happen earlier
>>> then before, but the acpi_table_upgrade) call in arch/x86/kernel/setup.c is
>>> not moved up, so the tables in the initrd are now only parsed and saved
>>> after the initial ACPI table scanning has already been done.
>>>
>>> I guess fixing this might be as easy as moving the acpi_table_upgrade) call
>>> higher in arch/x86/kernel/setup.c but I'm not sure if that is save to do.
>>
>> Why do you think it may not be safe?
> 
> OK, so it won't work in some cases, because acpi_table_upgrade() needs
> to be called after reserve_initrd(),

Right I notice it was sitting right after reserve_initrd() which made me think
that it probably needed to be after that. Sorry I should have mentioned that
in my original email.

> so I guess the commit above will
> need to be reverted.

One possible solution which I was wondering about is to modify
acpi_table_initrd_scan() to have it call acpi_tb_override_table()
instead of acpi_install_table() for existing tables using the matching
logic from acpi_table_initrd_override(). But I'm not sure when the
parsing of the DSDT is done. If acpi_table_initrd_scan() runs before
the first parsing of the DSDT is done then I think that that should work.

This might be more 5.13 material though and for 5.12 a revert is
probably best.

I also just remembered that at least the Intel audio folks rely on
DSDT overrides to get some (prototype) boards in their CI to work.

Regards,

Hans

Re: [5.12 regression] DSDT overriding from initrd no longer works

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 2280 bytes --]

On Mon, Apr 12, 2021 at 8:53 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 4/12/21 8:11 PM, Rafael J. Wysocki wrote:
> > On Mon, Apr 12, 2021 at 8:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >>
> >> On Mon, Apr 12, 2021 at 7:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>>
> >>> Hi Rafael,
> >>>
> >>> Sorry about the timing of reporting this regression.
> >>
> >> Oh well.
> >>
> >>> I just noticed that overriding the DSDT (*) from the initrd will not work in 5.12,
> >>> this is caused by:
> >>>
> >>> commit 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by ACPI tables")
> >>>
> >>> This makes the initial acpi_locate_initial_tables() call happen earlier
> >>> then before, but the acpi_table_upgrade) call in arch/x86/kernel/setup.c is
> >>> not moved up, so the tables in the initrd are now only parsed and saved
> >>> after the initial ACPI table scanning has already been done.
> >>>
> >>> I guess fixing this might be as easy as moving the acpi_table_upgrade) call
> >>> higher in arch/x86/kernel/setup.c but I'm not sure if that is save to do.
> >>
> >> Why do you think it may not be safe?
> >
> > OK, so it won't work in some cases, because acpi_table_upgrade() needs
> > to be called after reserve_initrd(),
>
> Right I notice it was sitting right after reserve_initrd() which made me think
> that it probably needed to be after that. Sorry I should have mentioned that
> in my original email.
>
> > so I guess the commit above will
> > need to be reverted.
>
> One possible solution which I was wondering about is to modify
> acpi_table_initrd_scan() to have it call acpi_tb_override_table()
> instead of acpi_install_table() for existing tables using the matching
> logic from acpi_table_initrd_override(). But I'm not sure when the
> parsing of the DSDT is done. If acpi_table_initrd_scan() runs before
> the first parsing of the DSDT is done then I think that that should work.
>
> This might be more 5.13 material though and for 5.12 a revert is
> probably best.

The attached change should make it work again, though.  Can you please verify?

> I also just remembered that at least the Intel audio folks rely on
> DSDT overrides to get some (prototype) boards in their CI to work.

But they haven't complained so far.

[-- Attachment #2: acpi-x86-ordering.patch --]
[-- Type: text/x-patch, Size: 756 bytes --]

---
 arch/x86/kernel/setup.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Index: linux-pm/arch/x86/kernel/setup.c
===================================================================
--- linux-pm.orig/arch/x86/kernel/setup.c
+++ linux-pm/arch/x86/kernel/setup.c
@@ -1045,9 +1045,6 @@ void __init setup_arch(char **cmdline_p)
 
 	cleanup_highmap();
 
-	/* Look for ACPI tables and reserve memory occupied by them. */
-	acpi_boot_table_init();
-
 	memblock_set_current_limit(ISA_END_ADDRESS);
 	e820__memblock_setup();
 
@@ -1132,6 +1129,8 @@ void __init setup_arch(char **cmdline_p)
 	reserve_initrd();
 
 	acpi_table_upgrade();
+	/* Look for ACPI tables and reserve memory occupied by them. */
+	acpi_boot_table_init();
 
 	vsmp_init();
 

Re: [5.12 regression] DSDT overriding from initrd no longer works

[Hans de Goede]

Hi Rafael,

On 4/13/21 11:35 AM, Rafael J. Wysocki wrote:
> On Mon, Apr 12, 2021 at 8:53 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 4/12/21 8:11 PM, Rafael J. Wysocki wrote:
>>> On Mon, Apr 12, 2021 at 8:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>>>
>>>> On Mon, Apr 12, 2021 at 7:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>
>>>>> Hi Rafael,
>>>>>
>>>>> Sorry about the timing of reporting this regression.
>>>>
>>>> Oh well.
>>>>
>>>>> I just noticed that overriding the DSDT (*) from the initrd will not work in 5.12,
>>>>> this is caused by:
>>>>>
>>>>> commit 1a1c130ab757 ("ACPI: tables: x86: Reserve memory occupied by ACPI tables")
>>>>>
>>>>> This makes the initial acpi_locate_initial_tables() call happen earlier
>>>>> then before, but the acpi_table_upgrade) call in arch/x86/kernel/setup.c is
>>>>> not moved up, so the tables in the initrd are now only parsed and saved
>>>>> after the initial ACPI table scanning has already been done.
>>>>>
>>>>> I guess fixing this might be as easy as moving the acpi_table_upgrade) call
>>>>> higher in arch/x86/kernel/setup.c but I'm not sure if that is save to do.
>>>>
>>>> Why do you think it may not be safe?
>>>
>>> OK, so it won't work in some cases, because acpi_table_upgrade() needs
>>> to be called after reserve_initrd(),
>>
>> Right I notice it was sitting right after reserve_initrd() which made me think
>> that it probably needed to be after that. Sorry I should have mentioned that
>> in my original email.
>>
>>> so I guess the commit above will
>>> need to be reverted.
>>
>> One possible solution which I was wondering about is to modify
>> acpi_table_initrd_scan() to have it call acpi_tb_override_table()
>> instead of acpi_install_table() for existing tables using the matching
>> logic from acpi_table_initrd_override(). But I'm not sure when the
>> parsing of the DSDT is done. If acpi_table_initrd_scan() runs before
>> the first parsing of the DSDT is done then I think that that should work.
>>
>> This might be more 5.13 material though and for 5.12 a revert is
>> probably best.
> 
> The attached change should make it work again, though.  Can you please verify?

I can confirm that the attached change fixes things, thank you.

>> I also just remembered that at least the Intel audio folks rely on
>> DSDT overrides to get some (prototype) boards in their CI to work.
> 
> But they haven't complained so far.

Weird, I'll drop them an email with you in the Cc.

Regards,

Hans