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]

Message-ID: <CANg-bXAgiMB2Q_fHOc_BBm4pRXdVveu-Rs6uZrLCAjkzaBYm4w@mail.gmail.com>
Date: Fri, 22 Dec 2023 15:21:34 -0700
From: Mark Hasemeyer <markhas@...omium.org>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: LKML <linux-kernel@...r.kernel.org>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, 
	Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, Tzung-Bi Shih <tzungbi@...nel.org>, 
	Raul Rangel <rrangel@...omium.org>, Konrad Dybcio <konrad.dybcio@...aro.org>, 
	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

> > +     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())
> > +                     irq_wake = true;
> > +             else
> > +                     irq_wake = irqres.flags & IORESOURCE_IRQ_WAKECAPABLE;
> > +             dev_dbg(dev, "IRQ: %i, wake_capable: %i\n", irq, irq_wake);
> > +     } else if (irq != -ENXIO) {
> >               dev_err(dev, "couldn't retrieve IRQ number (%d)\n", irq);
> >               return irq;
> >       }
>
> Yeah, this is confusing now. Which one should I trust more: irq or irqres.start?
> What is the point to have irqres with this duplication?

irqres is needed to pull the wake capability of the irq. I agree tha
irq and irqres.start should have the same information. I chose irq
because it's more obvious what it represents over irqres.start. It's
also more concise.

>
> ...
>
> > -             dev_err(dev, "couldn't register ec_dev (%d)\n", ret);
> > +             dev_err_probe(dev, ret, "couldn't register ec_dev (%d)\n", ret);
> >               return ret;
>
>                 return dev_err_probe(...);
>
> ...
>
> > +                     dev_err_probe(dev, ret, "Failed to init device for wakeup");
> > +                     return ret;
>

Tzung-Bi pointed out there are other areas of the driver that just use
dev_err() in the probe path. My vote is to drop the use of
dev_err_probe() to stay consistent with what exists. A separate patch
can be sent to modify the statements to use the
dev_err_probe() variant.

>
> ...
>
> > +     if (!np)
> > +             return;
>
> Why do you need this now?

It felt intuitive to return early from cros_ec_spi_dt_probe() if no
device node exists. This does not fix any known bugs though. Dropping
as irrelevant.

> >       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")) {
> > +             ec_spi->irq_wake = true;
> > +             dev_dbg(&spi->dev, "IRQ: %i, wake_capable: %i\n", ec_dev->irq, ec_spi->irq_wake);
> > +     }
>
>         if (ret)
>                 return;
>
>         ec_spi->irq_wake = of_property_read_bool(np, "wakeup-source"));

The other interface drivers only analyze irq_wake if an irq exists. I
think that makes sense and would like to stay consistent.
Thinking:
ec_spi->irq_wake = spi->irq > 0 && of_property_read_bool(np, "wakeup-source"));

>         dev_dbg(&spi->dev, "IRQ: %i, wake_capable: %s\n", ec_dev->irq, str_yes_no(ec_spi->irq_wake));

> ...
>
> > @@ -78,6 +80,7 @@ struct cros_ec_uart {
> >       u32 baudrate;
> >       u8 flowcontrol;
> >       u32 irq;
> > +     bool irq_wake;
> >       struct response_info response;
> >  };
>
> Run `pahole` and amend respectively to avoid wasting memory.

No savings to be had, but we can defrag the holes from sizes 3 and 3,
to 2 and 4 by moving irq_wake above irq.

pahole --reorganize -C cros_ec_uart cros_ec_uart.o

struct cros_ec_uart {
        struct serdev_device *     serdev;               /*     0     8 */
        u32                        baudrate;             /*     8     4 */
        u8                         flowcontrol;          /*    12     1 */
        bool                       irq_wake;             /*    13     1 */

        /* XXX 2 bytes hole, try to pack */

        u32                        irq;                  /*    16     4 */

        /* XXX 4 bytes hole, try to pack */

        struct response_info       response;             /*    24    64 */

        /* size: 88, cachelines: 2, members: 6 */
        /* sum members: 82, holes: 2, sum holes: 6 */
        /* last cacheline: 24 bytes */
};

> > +     dev_dbg(dev, "IRQ: %i, wake_capable: %i\n", ec_uart->irq, ec_uart->irq_wake);
>
> str_yes_no() from string_choices.h?

SGTM