linux-kernel - Re: [PATCH 1/2] pinctrl: renesas: rzg2l: Add suspend/resume support

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: <cdaffd20-1df4-4f73-ba96-9625e0749269@tuxon.dev>
Date: Thu, 15 Feb 2024 11:14:57 +0200
From: claudiu beznea <claudiu.beznea@...on.dev>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: magnus.damm@...il.com, robh@...nel.org,
 krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
 linus.walleij@...aro.org, linux-renesas-soc@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-gpio@...r.kernel.org, Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH 1/2] pinctrl: renesas: rzg2l: Add suspend/resume support

Hi, Geert,

On 12.02.2024 17:35, claudiu beznea wrote:
>>>  static const u16 available_ps[] = { 1800, 2500, 3300 };
>>> @@ -1880,6 +1938,19 @@ static void rzg2l_gpio_irq_print_chip(struct irq_data *data, struct seq_file *p)
>>>         seq_printf(p, dev_name(gc->parent));
>>>  }
>>>
>>> +static int rzg2l_gpio_irq_set_wake(struct irq_data *data, unsigned int on)
>>> +{
>>> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
>>> +       struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip);
>>> +
>> I think you also have to call irq_set_irq_wake(pctrl->hwirq[...]) here.
>> Cfr. drivers/gpio/gpio-rcar.c (which is simpler, as it has a single interrupt
>> parent, instead of a parent irq_domain with multiple interrupts).
> I had it in my initial implementation (done long time ago) but I don't
> remember why I removed it. I'll re-add it anyway.

I did some investigation on this. It seems adding irq_set_irq_wake() is not
necessary as the pinctrl has no virq requested on behalf of itself.

With this irqchip hierarchy (pinctrl-rzg2l -> irq-renesas-rzg2l -> gic) if
an IRQ consumer, e.g., the gpio-keys, request an interrupt then it may call
irq_set_irq_wake(virq) (gpio-keys does that).

irq_set_irq_wake(virq) is forwarded to pinctrl as follows:

irq_set_irq_wake(virq, on) ->
    set_irq_wake_real(virq, ono) ->
        rzg2l_gpio_irq_set_wake(irq, on)

As the irq_set_irq_wake() gets a virq as argument and as we have no virq
requested by pinctrl driver there is no need to call irq_set_irq_wake(), as
of my investigation. Calling it with hwirq will return with -22 and calling
it with virq received as argument leads to deadlock (as it's the same virq
that consumer already is configuring with irq_set_irq_wake()) due the
following line from irq_set_irq_wake():

struct irq_desc *desc = irq_get_desc_buslock(irq, &flags,
IRQ_GET_DESC_CHECK_GLOBAL);

What we can do is to forward irq_set_wake() to the parent IRQ chip
(irq-renesas-rzg2l) with irq_chip_set_wake_parent() to let him set its
wakeup_path, if any. But, at the moment the irq-renesas-rzg2l has
IRQCHIP_SKIP_SET_WAKE thus the irq_chip_set_wake_parent() does nothing (but
it can be updated for that). Now I remember that irq_chip_set_wake_parent()
is what I've called in my initial implementation and removed it due to
IRQCHIP_SKIP_SET_WAKE.

Please let me know if you are OK to add irq_chip_set_wake_parent() and
update the irq-renesas-rzg2l driver.

Thank you,
Claudiu Beznea