Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220216062601.779690-1-ztong0001@gmail.com>
In-Reply-To: <20220216062601.779690-1-ztong0001@gmail.com>
From:   Guenter Roeck <groeck@google.com>
Date:   Wed, 16 Feb 2022 08:12:42 -0800
Message-ID: <CABXOdTcZoGy3w_aVPju0DLr+Ab0C8XUsMs0+XsTW0QacR53iug@mail.gmail.com>
Subject: Re: [PATCH] platform/chrome: cros_ec: honor acpi=off
To:     Tong Zhang <ztong0001@gmail.com>
Cc:     Benson Leung <bleung@chromium.org>,
        Guenter Roeck <groeck@chromium.org>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Feb 15, 2022 at 10:26 PM Tong Zhang <ztong0001@gmail.com> wrote:
>
> when acpi=off is provided in bootarg, kernel crash with
>
>  BUG: kernel NULL pointer dereference, address: 0000000000000018
>  RIP: 0010:acpi_ns_walk_namespace+0x57/0x280
>   <TASK>
>  ? acpi_get_devices+0x140/0x140
>  cros_ec_lpc_init+0x25/0x100
>
> Driver should check if ACPI is disabled before calling acpi_get_devices(),
> otherwise acpi_walk_namespace() will dereference null pointer since the
> acpi_gbl_root_node is not initialized.
> This is a common pattern and should be fixed in ACPI framework to prevent
> such crash in the future, but since many drivers are already doing explicit
> check(acpi_disable) we do the same thing here.
>
> Signed-off-by: Tong Zhang <ztong0001@gmail.com>
> ---
>  drivers/platform/chrome/cros_ec_lpc.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
> index d6306d2a096f..95412a55ed8d 100644
> --- a/drivers/platform/chrome/cros_ec_lpc.c
> +++ b/drivers/platform/chrome/cros_ec_lpc.c
> @@ -550,6 +550,9 @@ static int __init cros_ec_lpc_init(void)
>         int ret;
>         acpi_status status;
>
> +       if (acpi_disable)

acpi_disabled ?

One does wonder why anyone would disable ACPI on any of the supported
systems, but in either case this is wrong. The driver should not abort
if acpi is disabled but just not call acpi_get_devices().

Guenter

> +               return -ENODEV;
> +
>         status = acpi_get_devices(ACPI_DRV_NAME, cros_ec_lpc_parse_device,
>                                   &cros_ec_lpc_acpi_device_found, NULL);
>         if (ACPI_FAILURE(status))
> --
> 2.25.1
>