[PATCH v2] drm/privacy-screen: honor acpi=off in detect_thinkpad_privacy_screen

[PATCH v2] drm/privacy-screen: honor acpi=off in detect_thinkpad_privacy_screen

[Tong Zhang]

when acpi=off is provided in bootarg, kernel crash with

[    1.252739] BUG: kernel NULL pointer dereference, address: 0000000000000018
[    1.258308] Call Trace:
[    1.258490]  ? acpi_walk_namespace+0x147/0x147
[    1.258770]  acpi_get_devices+0xe4/0x137
[    1.258921]  ? drm_core_init+0xc0/0xc0 [drm]
[    1.259108]  detect_thinkpad_privacy_screen+0x5e/0xa8 [drm]
[    1.259337]  drm_privacy_screen_lookup_init+0xe/0xe85 [drm]

The reason is that acpi_walk_namespace expects acpi related stuff
initialized but in fact it wouldn't when acpi is set to off. In this case
we should honor acpi=off in detect_thinkpad_privacy_screen().

Signed-off-by: Tong Zhang <ztong0001@gmail.com>
---
v2: fix typo in previous commit -- my keyboard is eating letters 

 drivers/gpu/drm/drm_privacy_screen_x86.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c
index a2cafb294ca6..e7aa74ad0b24 100644
--- a/drivers/gpu/drm/drm_privacy_screen_x86.c
+++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
@@ -33,6 +33,9 @@ static bool __init detect_thinkpad_privacy_screen(void)
 	unsigned long long output;
 	acpi_status status;
 
+	if (acpi_disabled)
+		return false;
+
 	/* Get embedded-controller handle */
 	status = acpi_get_devices("PNP0C09", acpi_set_handle, NULL, &ec_handle);
 	if (ACPI_FAILURE(status) || !ec_handle)
-- 
2.25.1

acpi_get_devices() crash when acpi_disabled==true (was [PATCH v2] drm/privacy-screen: honor acpi=off in detect_thinkpad_privacy_screen)

[Hans de Goede]

Hi All,

On 1/23/22 10:10, Tong Zhang wrote:
> when acpi=off is provided in bootarg, kernel crash with
> 
> [    1.252739] BUG: kernel NULL pointer dereference, address: 0000000000000018
> [    1.258308] Call Trace:
> [    1.258490]  ? acpi_walk_namespace+0x147/0x147
> [    1.258770]  acpi_get_devices+0xe4/0x137
> [    1.258921]  ? drm_core_init+0xc0/0xc0 [drm]
> [    1.259108]  detect_thinkpad_privacy_screen+0x5e/0xa8 [drm]
> [    1.259337]  drm_privacy_screen_lookup_init+0xe/0xe85 [drm]
> 
> The reason is that acpi_walk_namespace expects acpi related stuff
> initialized but in fact it wouldn't when acpi is set to off. In this case
> we should honor acpi=off in detect_thinkpad_privacy_screen().
> 
> Signed-off-by: Tong Zhang <ztong0001@gmail.com>

Thank you for catching this and thank you for your patch. I was about to merge
this, but then I realized that this might not be the best way to fix this.

A quick grep shows 10 acpi_get_devices() calls outside of drivers/acpi,
and at a first glance about half of those are missing an acpi_disabled
check. IMHO it would be better to simply add an acpi_disabled check to
acpi_get_devices() itself.

Rafael, do you agree ?


Note the just added chrome privacy-screen check uses
acpi_dev_present(), this is also used in about 10 places outside
of drivers/acpi and AFAIK none of those do an acpi_disabled check.

acpi_dev_present() uses bus_find_device(&acpi_bus_type, ...)
but the acpi_bus_type does not get registered when acpi_disabled
is set. In the end this is fine though since bus_find_device
checks for the bus not being registered and then just returns
NULL.

Regards,

Hans




> ---
> v2: fix typo in previous commit -- my keyboard is eating letters 
> 
>  drivers/gpu/drm/drm_privacy_screen_x86.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c
> index a2cafb294ca6..e7aa74ad0b24 100644
> --- a/drivers/gpu/drm/drm_privacy_screen_x86.c
> +++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
> @@ -33,6 +33,9 @@ static bool __init detect_thinkpad_privacy_screen(void)
>  	unsigned long long output;
>  	acpi_status status;
>  
> +	if (acpi_disabled)
> +		return false;
> +
>  	/* Get embedded-controller handle */
>  	status = acpi_get_devices("PNP0C09", acpi_set_handle, NULL, &ec_handle);
>  	if (ACPI_FAILURE(status) || !ec_handle)

Re: acpi_get_devices() crash when acpi_disabled==true (was [PATCH v2] drm/privacy-screen: honor acpi=off in detect_thinkpad_privacy_screen)

[Hans de Goede]

Hi,

On 1/26/22 14:47, Hans de Goede wrote:
> Hi All,
> 
> On 1/23/22 10:10, Tong Zhang wrote:
>> when acpi=off is provided in bootarg, kernel crash with
>>
>> [    1.252739] BUG: kernel NULL pointer dereference, address: 0000000000000018
>> [    1.258308] Call Trace:
>> [    1.258490]  ? acpi_walk_namespace+0x147/0x147
>> [    1.258770]  acpi_get_devices+0xe4/0x137
>> [    1.258921]  ? drm_core_init+0xc0/0xc0 [drm]
>> [    1.259108]  detect_thinkpad_privacy_screen+0x5e/0xa8 [drm]
>> [    1.259337]  drm_privacy_screen_lookup_init+0xe/0xe85 [drm]
>>
>> The reason is that acpi_walk_namespace expects acpi related stuff
>> initialized but in fact it wouldn't when acpi is set to off. In this case
>> we should honor acpi=off in detect_thinkpad_privacy_screen().
>>
>> Signed-off-by: Tong Zhang <ztong0001@gmail.com>
> 
> Thank you for catching this and thank you for your patch. I was about to merge
> this, but then I realized that this might not be the best way to fix this.
> 
> A quick grep shows 10 acpi_get_devices() calls outside of drivers/acpi,
> and at a first glance about half of those are missing an acpi_disabled
> check. IMHO it would be better to simply add an acpi_disabled check to
> acpi_get_devices() itself.
> 
> Rafael, do you agree ?

Never mind I just saw that acpi_get_devices() is part of acpica, where
as the acpi_disabled flag is not. So callers need to check acpi_disabled
before calling acpi_get_devices().

I'll go and push this patch to drm-misc-fixes now.

Regards,

Hans


>> ---
>> v2: fix typo in previous commit -- my keyboard is eating letters 
>>
>>  drivers/gpu/drm/drm_privacy_screen_x86.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c
>> index a2cafb294ca6..e7aa74ad0b24 100644
>> --- a/drivers/gpu/drm/drm_privacy_screen_x86.c
>> +++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
>> @@ -33,6 +33,9 @@ static bool __init detect_thinkpad_privacy_screen(void)
>>  	unsigned long long output;
>>  	acpi_status status;
>>  
>> +	if (acpi_disabled)
>> +		return false;
>> +
>>  	/* Get embedded-controller handle */
>>  	status = acpi_get_devices("PNP0C09", acpi_set_handle, NULL, &ec_handle);
>>  	if (ACPI_FAILURE(status) || !ec_handle)

Re: acpi_get_devices() crash when acpi_disabled==true (was [PATCH v2] drm/privacy-screen: honor acpi=off in detect_thinkpad_privacy_screen)

[Rafael J. Wysocki]

On Wed, Jan 26, 2022 at 2:47 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi All,
>
> On 1/23/22 10:10, Tong Zhang wrote:
> > when acpi=off is provided in bootarg, kernel crash with
> >
> > [    1.252739] BUG: kernel NULL pointer dereference, address: 0000000000000018
> > [    1.258308] Call Trace:
> > [    1.258490]  ? acpi_walk_namespace+0x147/0x147
> > [    1.258770]  acpi_get_devices+0xe4/0x137
> > [    1.258921]  ? drm_core_init+0xc0/0xc0 [drm]
> > [    1.259108]  detect_thinkpad_privacy_screen+0x5e/0xa8 [drm]
> > [    1.259337]  drm_privacy_screen_lookup_init+0xe/0xe85 [drm]
> >
> > The reason is that acpi_walk_namespace expects acpi related stuff
> > initialized but in fact it wouldn't when acpi is set to off. In this case
> > we should honor acpi=off in detect_thinkpad_privacy_screen().
> >
> > Signed-off-by: Tong Zhang <ztong0001@gmail.com>
>
> Thank you for catching this and thank you for your patch. I was about to merge
> this, but then I realized that this might not be the best way to fix this.
>
> A quick grep shows 10 acpi_get_devices() calls outside of drivers/acpi,
> and at a first glance about half of those are missing an acpi_disabled
> check. IMHO it would be better to simply add an acpi_disabled check to
> acpi_get_devices() itself.
>
> Rafael, do you agree ?

Yes, I do.

> Note the just added chrome privacy-screen check uses
> acpi_dev_present(), this is also used in about 10 places outside
> of drivers/acpi and AFAIK none of those do an acpi_disabled check.
>
> acpi_dev_present() uses bus_find_device(&acpi_bus_type, ...)
> but the acpi_bus_type does not get registered when acpi_disabled
> is set. In the end this is fine though since bus_find_device
> checks for the bus not being registered and then just returns
> NULL.

Right.

> > ---
> > v2: fix typo in previous commit -- my keyboard is eating letters
> >
> >  drivers/gpu/drm/drm_privacy_screen_x86.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c
> > index a2cafb294ca6..e7aa74ad0b24 100644
> > --- a/drivers/gpu/drm/drm_privacy_screen_x86.c
> > +++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
> > @@ -33,6 +33,9 @@ static bool __init detect_thinkpad_privacy_screen(void)
> >       unsigned long long output;
> >       acpi_status status;
> >
> > +     if (acpi_disabled)
> > +             return false;
> > +
> >       /* Get embedded-controller handle */
> >       status = acpi_get_devices("PNP0C09", acpi_set_handle, NULL, &ec_handle);
> >       if (ACPI_FAILURE(status) || !ec_handle)
>

Re: acpi_get_devices() crash when acpi_disabled==true (was [PATCH v2] drm/privacy-screen: honor acpi=off in detect_thinkpad_privacy_screen)

[Hans de Goede]

Hi,

On 1/26/22 16:54, Rafael J. Wysocki wrote:
> On Wed, Jan 26, 2022 at 2:47 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi All,
>>
>> On 1/23/22 10:10, Tong Zhang wrote:
>>> when acpi=off is provided in bootarg, kernel crash with
>>>
>>> [    1.252739] BUG: kernel NULL pointer dereference, address: 0000000000000018
>>> [    1.258308] Call Trace:
>>> [    1.258490]  ? acpi_walk_namespace+0x147/0x147
>>> [    1.258770]  acpi_get_devices+0xe4/0x137
>>> [    1.258921]  ? drm_core_init+0xc0/0xc0 [drm]
>>> [    1.259108]  detect_thinkpad_privacy_screen+0x5e/0xa8 [drm]
>>> [    1.259337]  drm_privacy_screen_lookup_init+0xe/0xe85 [drm]
>>>
>>> The reason is that acpi_walk_namespace expects acpi related stuff
>>> initialized but in fact it wouldn't when acpi is set to off. In this case
>>> we should honor acpi=off in detect_thinkpad_privacy_screen().
>>>
>>> Signed-off-by: Tong Zhang <ztong0001@gmail.com>
>>
>> Thank you for catching this and thank you for your patch. I was about to merge
>> this, but then I realized that this might not be the best way to fix this.
>>
>> A quick grep shows 10 acpi_get_devices() calls outside of drivers/acpi,
>> and at a first glance about half of those are missing an acpi_disabled
>> check. IMHO it would be better to simply add an acpi_disabled check to
>> acpi_get_devices() itself.
>>
>> Rafael, do you agree ?
> 
> Yes, I do.

Did you see my follow-up that that is not going to work because
acpi_get_devices() is an acpica function ?

Regards,

Hans


 
>>> ---
>>> v2: fix typo in previous commit -- my keyboard is eating letters
>>>
>>>  drivers/gpu/drm/drm_privacy_screen_x86.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c
>>> index a2cafb294ca6..e7aa74ad0b24 100644
>>> --- a/drivers/gpu/drm/drm_privacy_screen_x86.c
>>> +++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
>>> @@ -33,6 +33,9 @@ static bool __init detect_thinkpad_privacy_screen(void)
>>>       unsigned long long output;
>>>       acpi_status status;
>>>
>>> +     if (acpi_disabled)
>>> +             return false;
>>> +
>>>       /* Get embedded-controller handle */
>>>       status = acpi_get_devices("PNP0C09", acpi_set_handle, NULL, &ec_handle);
>>>       if (ACPI_FAILURE(status) || !ec_handle)
>>
> 

Re: acpi_get_devices() crash when acpi_disabled==true (was [PATCH v2] drm/privacy-screen: honor acpi=off in detect_thinkpad_privacy_screen)

[Rafael J. Wysocki]

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

On Wed, Jan 26, 2022 at 5:41 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 1/26/22 16:54, Rafael J. Wysocki wrote:
> > On Wed, Jan 26, 2022 at 2:47 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi All,
> >>
> >> On 1/23/22 10:10, Tong Zhang wrote:
> >>> when acpi=off is provided in bootarg, kernel crash with
> >>>
> >>> [    1.252739] BUG: kernel NULL pointer dereference, address: 0000000000000018
> >>> [    1.258308] Call Trace:
> >>> [    1.258490]  ? acpi_walk_namespace+0x147/0x147
> >>> [    1.258770]  acpi_get_devices+0xe4/0x137
> >>> [    1.258921]  ? drm_core_init+0xc0/0xc0 [drm]
> >>> [    1.259108]  detect_thinkpad_privacy_screen+0x5e/0xa8 [drm]
> >>> [    1.259337]  drm_privacy_screen_lookup_init+0xe/0xe85 [drm]
> >>>
> >>> The reason is that acpi_walk_namespace expects acpi related stuff
> >>> initialized but in fact it wouldn't when acpi is set to off. In this case
> >>> we should honor acpi=off in detect_thinkpad_privacy_screen().
> >>>
> >>> Signed-off-by: Tong Zhang <ztong0001@gmail.com>
> >>
> >> Thank you for catching this and thank you for your patch. I was about to merge
> >> this, but then I realized that this might not be the best way to fix this.
> >>
> >> A quick grep shows 10 acpi_get_devices() calls outside of drivers/acpi,
> >> and at a first glance about half of those are missing an acpi_disabled
> >> check. IMHO it would be better to simply add an acpi_disabled check to
> >> acpi_get_devices() itself.
> >>
> >> Rafael, do you agree ?
> >
> > Yes, I do.
>
> Did you see my follow-up that that is not going to work because
> acpi_get_devices() is an acpica function ?

No, I didn't, but it is possible to add a wrapper doing the check
around it and convert all of the users.

Alternatively, the ACPICA function can check acpi_gbl_root_node
against NULL, like in the attached (untested) patch.

[-- Attachment #2: acpica-check-root-in-walks.patch --]
[-- Type: text/x-patch, Size: 535 bytes --]

---
 drivers/acpi/acpica/nswalk.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-pm/drivers/acpi/acpica/nswalk.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/nswalk.c
+++ linux-pm/drivers/acpi/acpica/nswalk.c
@@ -169,6 +169,9 @@ acpi_ns_walk_namespace(acpi_object_type
 
 	if (start_node == ACPI_ROOT_OBJECT) {
 		start_node = acpi_gbl_root_node;
+		if (!start_node) {
+			return_ACPI_STATUS(AE_NO_NAMESPACE);
+		}
 	}
 
 	/* Null child means "get first node" */

Re: acpi_get_devices() crash when acpi_disabled==true (was [PATCH v2] drm/privacy-screen: honor acpi=off in detect_thinkpad_privacy_screen)

[Hans de Goede]

Hi,

On 1/26/22 18:11, Rafael J. Wysocki wrote:
> On Wed, Jan 26, 2022 at 5:41 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 1/26/22 16:54, Rafael J. Wysocki wrote:
>>> On Wed, Jan 26, 2022 at 2:47 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Hi All,
>>>>
>>>> On 1/23/22 10:10, Tong Zhang wrote:
>>>>> when acpi=off is provided in bootarg, kernel crash with
>>>>>
>>>>> [    1.252739] BUG: kernel NULL pointer dereference, address: 0000000000000018
>>>>> [    1.258308] Call Trace:
>>>>> [    1.258490]  ? acpi_walk_namespace+0x147/0x147
>>>>> [    1.258770]  acpi_get_devices+0xe4/0x137
>>>>> [    1.258921]  ? drm_core_init+0xc0/0xc0 [drm]
>>>>> [    1.259108]  detect_thinkpad_privacy_screen+0x5e/0xa8 [drm]
>>>>> [    1.259337]  drm_privacy_screen_lookup_init+0xe/0xe85 [drm]
>>>>>
>>>>> The reason is that acpi_walk_namespace expects acpi related stuff
>>>>> initialized but in fact it wouldn't when acpi is set to off. In this case
>>>>> we should honor acpi=off in detect_thinkpad_privacy_screen().
>>>>>
>>>>> Signed-off-by: Tong Zhang <ztong0001@gmail.com>
>>>>
>>>> Thank you for catching this and thank you for your patch. I was about to merge
>>>> this, but then I realized that this might not be the best way to fix this.
>>>>
>>>> A quick grep shows 10 acpi_get_devices() calls outside of drivers/acpi,
>>>> and at a first glance about half of those are missing an acpi_disabled
>>>> check. IMHO it would be better to simply add an acpi_disabled check to
>>>> acpi_get_devices() itself.
>>>>
>>>> Rafael, do you agree ?
>>>
>>> Yes, I do.
>>
>> Did you see my follow-up that that is not going to work because
>> acpi_get_devices() is an acpica function ?
> 
> No, I didn't, but it is possible to add a wrapper doing the check
> around it and convert all of the users.

Yes I did think about that. Note that I've gone ahead and pushed
the fix which started this to drm-misc-fixes, to resolve the crash
for now.

If we add such a wrapper we can remove a bunch of acpi_disabled checks
from various callers.

> Alternatively, the ACPICA function can check acpi_gbl_root_node
> against NULL, like in the attached (untested) patch.

That is probably an even better idea, as that avoids the need
for a wrapper altogether. So I believe that that is the best
solution.

Regards,

Hans

Re: acpi_get_devices() crash when acpi_disabled==true (was [PATCH v2] drm/privacy-screen: honor acpi=off in detect_thinkpad_privacy_screen)

[Rafael J. Wysocki]

On Thu, Jan 27, 2022 at 2:05 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 1/26/22 18:11, Rafael J. Wysocki wrote:
> > On Wed, Jan 26, 2022 at 5:41 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi,
> >>
> >> On 1/26/22 16:54, Rafael J. Wysocki wrote:
> >>> On Wed, Jan 26, 2022 at 2:47 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>>>
> >>>> Hi All,
> >>>>
> >>>> On 1/23/22 10:10, Tong Zhang wrote:
> >>>>> when acpi=off is provided in bootarg, kernel crash with
> >>>>>
> >>>>> [    1.252739] BUG: kernel NULL pointer dereference, address: 0000000000000018
> >>>>> [    1.258308] Call Trace:
> >>>>> [    1.258490]  ? acpi_walk_namespace+0x147/0x147
> >>>>> [    1.258770]  acpi_get_devices+0xe4/0x137
> >>>>> [    1.258921]  ? drm_core_init+0xc0/0xc0 [drm]
> >>>>> [    1.259108]  detect_thinkpad_privacy_screen+0x5e/0xa8 [drm]
> >>>>> [    1.259337]  drm_privacy_screen_lookup_init+0xe/0xe85 [drm]
> >>>>>
> >>>>> The reason is that acpi_walk_namespace expects acpi related stuff
> >>>>> initialized but in fact it wouldn't when acpi is set to off. In this case
> >>>>> we should honor acpi=off in detect_thinkpad_privacy_screen().
> >>>>>
> >>>>> Signed-off-by: Tong Zhang <ztong0001@gmail.com>
> >>>>
> >>>> Thank you for catching this and thank you for your patch. I was about to merge
> >>>> this, but then I realized that this might not be the best way to fix this.
> >>>>
> >>>> A quick grep shows 10 acpi_get_devices() calls outside of drivers/acpi,
> >>>> and at a first glance about half of those are missing an acpi_disabled
> >>>> check. IMHO it would be better to simply add an acpi_disabled check to
> >>>> acpi_get_devices() itself.
> >>>>
> >>>> Rafael, do you agree ?
> >>>
> >>> Yes, I do.
> >>
> >> Did you see my follow-up that that is not going to work because
> >> acpi_get_devices() is an acpica function ?
> >
> > No, I didn't, but it is possible to add a wrapper doing the check
> > around it and convert all of the users.
>
> Yes I did think about that. Note that I've gone ahead and pushed
> the fix which started this to drm-misc-fixes, to resolve the crash
> for now.

OK

> If we add such a wrapper we can remove a bunch of acpi_disabled checks
> from various callers.
>
> > Alternatively, the ACPICA function can check acpi_gbl_root_node
> > against NULL, like in the attached (untested) patch.
>
> That is probably an even better idea, as that avoids the need
> for a wrapper altogether. So I believe that that is the best
> solution.

Allright, let me cut an analogous patch for the upstream ACPICA, then.

Re: acpi_get_devices() crash when acpi_disabled==true (was [PATCH v2] drm/privacy-screen: honor acpi=off in detect_thinkpad_privacy_screen)

[Hans de Goede]

Hi,

On 1/27/22 14:33, Rafael J. Wysocki wrote:
> On Thu, Jan 27, 2022 at 2:05 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 1/26/22 18:11, Rafael J. Wysocki wrote:
>>> On Wed, Jan 26, 2022 at 5:41 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 1/26/22 16:54, Rafael J. Wysocki wrote:
>>>>> On Wed, Jan 26, 2022 at 2:47 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>
>>>>>> Hi All,
>>>>>>
>>>>>> On 1/23/22 10:10, Tong Zhang wrote:
>>>>>>> when acpi=off is provided in bootarg, kernel crash with
>>>>>>>
>>>>>>> [    1.252739] BUG: kernel NULL pointer dereference, address: 0000000000000018
>>>>>>> [    1.258308] Call Trace:
>>>>>>> [    1.258490]  ? acpi_walk_namespace+0x147/0x147
>>>>>>> [    1.258770]  acpi_get_devices+0xe4/0x137
>>>>>>> [    1.258921]  ? drm_core_init+0xc0/0xc0 [drm]
>>>>>>> [    1.259108]  detect_thinkpad_privacy_screen+0x5e/0xa8 [drm]
>>>>>>> [    1.259337]  drm_privacy_screen_lookup_init+0xe/0xe85 [drm]
>>>>>>>
>>>>>>> The reason is that acpi_walk_namespace expects acpi related stuff
>>>>>>> initialized but in fact it wouldn't when acpi is set to off. In this case
>>>>>>> we should honor acpi=off in detect_thinkpad_privacy_screen().
>>>>>>>
>>>>>>> Signed-off-by: Tong Zhang <ztong0001@gmail.com>
>>>>>>
>>>>>> Thank you for catching this and thank you for your patch. I was about to merge
>>>>>> this, but then I realized that this might not be the best way to fix this.
>>>>>>
>>>>>> A quick grep shows 10 acpi_get_devices() calls outside of drivers/acpi,
>>>>>> and at a first glance about half of those are missing an acpi_disabled
>>>>>> check. IMHO it would be better to simply add an acpi_disabled check to
>>>>>> acpi_get_devices() itself.
>>>>>>
>>>>>> Rafael, do you agree ?
>>>>>
>>>>> Yes, I do.
>>>>
>>>> Did you see my follow-up that that is not going to work because
>>>> acpi_get_devices() is an acpica function ?
>>>
>>> No, I didn't, but it is possible to add a wrapper doing the check
>>> around it and convert all of the users.
>>
>> Yes I did think about that. Note that I've gone ahead and pushed
>> the fix which started this to drm-misc-fixes, to resolve the crash
>> for now.
> 
> OK
> 
>> If we add such a wrapper we can remove a bunch of acpi_disabled checks
>> from various callers.
>>
>>> Alternatively, the ACPICA function can check acpi_gbl_root_node
>>> against NULL, like in the attached (untested) patch.
>>
>> That is probably an even better idea, as that avoids the need
>> for a wrapper altogether. So I believe that that is the best
>> solution.
> 
> Allright, let me cut an analogous patch for the upstream ACPICA, then.

Great, thank you.

I have added a note about checking for when this has found its way
into Linus' tree to my own TODO list, with the goal of doing
a cleanup series removing the then no longer needed acpi_disabled
checks in a bunch of places.

Regards,

Hans