linux-kernel - Re: [PATCH v2 22/22] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

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]

Date: Fri, 22 Dec 2023 15:19:56 -0700
From: Mark Hasemeyer <markhas@...omium.org>
To: Tzung-Bi Shih <tzungbi@...nel.org>
Cc: LKML <linux-kernel@...r.kernel.org>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, 
	Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, Raul Rangel <rrangel@...omium.org>, 
	Konrad Dybcio <konrad.dybcio@...aro.org>, Andy Shevchenko <andriy.shevchenko@...el.com>, 
	Rob Herring <robh@...nel.org>, Sudeep Holla <sudeep.holla@....com>, 
	Benson Leung <bleung@...omium.org>, Bhanu Prakash Maiya <bhanumaiya@...omium.org>, 
	Chen-Yu Tsai <wenst@...omium.org>, Guenter Roeck <groeck@...omium.org>, Lee Jones <lee@...nel.org>, 
	Prashant Malani <pmalani@...omium.org>, Rob Barnes <robbarnes@...gle.com>, 
	Stephen Boyd <swboyd@...omium.org>, chrome-platform@...ts.linux.dev
Subject: Re: [PATCH v2 22/22] platform/chrome: cros_ec: Use PM subsystem to
 manage wakeirq

On Thu, Dec 21, 2023 at 1:58 AM Tzung-Bi Shih <tzungbi@...nel.org> wrote:
>
> On Wed, Dec 20, 2023 at 04:54:36PM -0700, Mark Hasemeyer wrote:
> > The IRQ wake logic was added on an interface basis (lpc, spi, uart) as
> > opposed to adding it to cros_ec.c because the i2c subsystem already
> > enables the wakirq (if applicable) on our behalf.
>
> The setting flow are all the same.  I think helper functions in cros_ec.c help
> to deduplicate the code.

I'll see what I can do.

> > +MODULE_DEVICE_TABLE(dmi, untrusted_fw_irq_wake_capable);
>
> Does it really need `MODULE_DEVICE_TABLE`?

Nope. Will drop.

> >       ret = cros_ec_register(ec_dev);
> >       if (ret) {
> > -             dev_err(dev, "couldn't register ec_dev (%d)\n", ret);
> > +             dev_err_probe(dev, ret, "couldn't register ec_dev (%d)\n", ret);
>
> The change is irrelevant to the series.

I'll drop the use of dev_err_probe() to stay consistent with current
conventions. Perhaps it can be added in a follow-up patch.

> > @@ -470,6 +512,8 @@ static void cros_ec_lpc_remove(struct platform_device *pdev)
> >               acpi_remove_notify_handler(adev->handle, ACPI_ALL_NOTIFY,
> >                                          cros_ec_lpc_acpi_notify);
> >
> > +     dev_pm_clear_wake_irq(dev);
> > +     device_init_wakeup(dev, false);
>
> Is it safe to call them anyway regardless of `irq_wake` in cros_ec_lpc_probe()?

According to bench tests, it's not a problem. That said, I am
refactoring the code to move the logic into cros_ec.c and will
conditionally call the cleanup functions.

> > +     if (!np)
> > +             return;
> > +
>
> The change is an improvement (or rather say it could change behavior).  But
> strictly speaking, the change is irrelevant to the series.

Will drop.

>
> > @@ -702,6 +710,11 @@ static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
> >       ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val);
> >       if (!ret)
> >               ec_spi->end_of_msg_delay = val;
> > +
> > +     if (ec_dev->irq > 0 && of_property_read_bool(np, "wakeup-source")) {
>
> Or just use `spi->irq`[2].
>
> [2]: https://elixir.bootlin.com/linux/v6.6/source/drivers/platform/chrome/cros_ec_spi.c#L762
>
> They are the same, but does of_property_present() make more sense?

Yes it does. I'll use it.

> > @@ -768,6 +778,9 @@ static int cros_ec_spi_probe(struct spi_device *spi)
> >                          sizeof(struct ec_response_get_protocol_info);
> >       ec_dev->dout_size = sizeof(struct ec_host_request);
> >
> > +     /* Check for any DT properties */
> > +     cros_ec_spi_dt_probe(ec_spi, spi);
>
> `spi` is possibly not needed.  See comment above.

Agreed.