| 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
| ||
|
Message-ID: <ZYP-H27vsJvMTupH@google.com>
Date: Thu, 21 Dec 2023 16:58:07 +0800
From: Tzung-Bi Shih <tzungbi@...nel.org>
To: Mark Hasemeyer <markhas@...omium.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 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.
> diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
[...]
> +static const struct dmi_system_id untrusted_fw_irq_wake_capable[] = {
> + {
> + .ident = "Brya",
> + .matches = {
> + DMI_MATCH(DMI_PRODUCT_FAMILY, "Google_Brya")
> + }
> + },
> + {
> + .ident = "Brask",
> + .matches = {
> + DMI_MATCH(DMI_PRODUCT_FAMILY, "Google_Brask")
> + }
> + },
> + { }
> +}
> +MODULE_DEVICE_TABLE(dmi, untrusted_fw_irq_wake_capable);
Does it really need `MODULE_DEVICE_TABLE`?
> +static bool cros_ec_should_force_irq_wake_capable(void)
Suggestion: either drop "cros_ec_" prefix or use "cros_ec_lpc_" prefix.
> @@ -428,20 +453,36 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
> * Some boards do not have an IRQ allotted for cros_ec_lpc,
> * which makes ENXIO an expected (and safe) scenario.
> */
> - irq = platform_get_irq_optional(pdev, 0);
> - if (irq > 0)
> + irq = platform_get_irq_resource_optional(pdev, 0, &irqres);
> + if (irq > 0) {
> ec_dev->irq = irq;
> - else if (irq != -ENXIO) {
> + if (cros_ec_should_force_irq_wake_capable())
Please see suggestion above.
> 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.
> + if (irq_wake) {
> + ret = device_init_wakeup(dev, true);
> + if (ret) {
> + dev_err_probe(dev, ret, "Failed to init device for wakeup");
> + return ret;
> + }
> + ret = dev_pm_set_wake_irq(dev, irq);
> + if (ret)
> + return ret;
> + }
[...]
> @@ -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()?
> diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
[...]
> -static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
> +static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct spi_device *spi)
> {
> - struct device_node *np = dev->of_node;
> + struct cros_ec_device *ec_dev = spi_get_drvdata(spi);
> + struct device_node *np = spi->dev.of_node;
struct spi_device *spi = ec_spi->spi; [1]
[1]: https://elixir.bootlin.com/linux/v6.6/source/drivers/platform/chrome/cros_ec_spi.c#L751
> + 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.
> @@ -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?
> @@ -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.
> @@ -776,19 +789,31 @@ static int cros_ec_spi_probe(struct spi_device *spi)
>
> err = cros_ec_register(ec_dev);
> if (err) {
> - dev_err(dev, "cannot register EC\n");
> + dev_err_probe(dev, err, "cannot register EC\n");
The change is irrelevant to the series.
> - device_init_wakeup(&spi->dev, true);
> + if (ec_spi->irq_wake) {
> + err = device_init_wakeup(dev, true);
> + if (err) {
> + dev_err_probe(dev, err, "Failed to init device for wakeup\n");
> + return err;
> + }
> + err = dev_pm_set_wake_irq(dev, ec_dev->irq);
> + if (err)
> + dev_err_probe(dev, err, "Failed to set irq(%d) for wake\n", ec_dev->irq);
The part is different from what the patch changed in cros_ec_lpc.c. Better to
be consistent.
- Just return vs. dev_err_probe().
- %i vs. %d.
> static void cros_ec_spi_remove(struct spi_device *spi)
> {
> struct cros_ec_device *ec_dev = spi_get_drvdata(spi);
> + struct device *dev = ec_dev->dev;
>
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
Ditto, is it safe to just call them regardless of `ec_spi->irq_wake`?
> diff --git a/drivers/platform/chrome/cros_ec_uart.c b/drivers/platform/chrome/cros_ec_uart.c
[...]
> @@ -301,13 +307,31 @@ static int cros_ec_uart_probe(struct serdev_device *serdev)
>
> serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops);
>
> - return cros_ec_register(ec_dev);
> + /* Register a new cros_ec device */
> + ret = cros_ec_register(ec_dev);
> + if (ret) {
> + dev_err(dev, "Couldn't register ec_dev (%d)\n", ret);
>From reading the changes above, I thought it would use dev_err_probe().
> + if (ec_uart->irq_wake) {
> + ret = device_init_wakeup(dev, true);
> + if (ret) {
> + dev_err_probe(dev, ret, "Failed to init device for wakeup");
> + return ret;
> + }
> + ret = dev_pm_set_wake_irq(dev, ec_uart->irq);
Ditto, better to be consistent.
> static void cros_ec_uart_remove(struct serdev_device *serdev)
> {
> struct cros_ec_device *ec_dev = serdev_device_get_drvdata(serdev);
> + struct device *dev = ec_dev->dev;
>
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
Ditto, is it safe to just call them regardless of `ec_uart->irq_wake`?
Powered by blists - more mailing lists