All of lore.kernel.org
 help / color / mirror / Atom feed
* gpiochip_lock_as_irq on pins without FLAG_REQUESTED: bug or feature ?
@ 2021-06-28  3:36 Vincent Pelletier
  2021-06-28 13:40 ` Andy Shevchenko
  2021-07-02  0:09 ` Linus Walleij
  0 siblings, 2 replies; 10+ messages in thread
From: Vincent Pelletier @ 2021-06-28  3:36 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, linux-gpio,
	Linux Kernel Mailing List

Hello,

While trying to debug an IRQ handling issue on a sifive-unmatched board
(which is a very recent board on a recent architecture, so I would not
be overly surprised if there were bugs in hiding), I realised that I was able
to claim via sysfs GPIO pins which are being actively used as IRQ sources.

Checking drivers/gpio/gpiolib.c and kernel/irq/chip.c, I believe this is because
gpiolib (gpiochip_irq_reqres, gpiochip_reqres_irq, gpiochip_lock_as_irq)
does not call gpiod_request_{,commit}, resulting in a pin which is available
for use. I could confirm this by adding (just as a debugging aid):
  WARN_ON(!test_bit(FLAG_REQUESTED, &desc->flags));
early in gpiochip_lock_as_irq, and this statement gets triggered.

Is this intentional ?
Does this requesting belong to something else in the codepath from
request_threaded_irq (and similar) ?
Could it be something missing in the devicetree for this board ?

Also, I notice that both gpiochip_hierarchy_add_domain and
gpiochip_reqres_irq call gpiochip_lock_as_irq, and I am surprised I do not
get any error about this: in my understanding only the first call on a given pin
should succeed, but with my WARN_ON I am seeing both stack traces and
no other warning.

FWIW, my builds are based on vanilla 5.13-rc6.

Regards,
-- 
Vincent Pelletier

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gpiochip_lock_as_irq on pins without FLAG_REQUESTED: bug or feature ?
  2021-06-28  3:36 gpiochip_lock_as_irq on pins without FLAG_REQUESTED: bug or feature ? Vincent Pelletier
@ 2021-06-28 13:40 ` Andy Shevchenko
  2021-06-28 13:42   ` Andy Shevchenko
  2021-07-02  0:09 ` Linus Walleij
  1 sibling, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2021-06-28 13:40 UTC (permalink / raw)
  To: Vincent Pelletier
  Cc: Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
	Linux Kernel Mailing List

On Mon, Jun 28, 2021 at 6:37 AM Vincent Pelletier <plr.vincent@gmail.com> wrote:
>
> Hello,
>
> While trying to debug an IRQ handling issue on a sifive-unmatched board
> (which is a very recent board on a recent architecture, so I would not
> be overly surprised if there were bugs in hiding), I realised that I was able
> to claim via sysfs GPIO pins which are being actively used as IRQ sources.
>
> Checking drivers/gpio/gpiolib.c and kernel/irq/chip.c, I believe this is because
> gpiolib (gpiochip_irq_reqres, gpiochip_reqres_irq, gpiochip_lock_as_irq)
> does not call gpiod_request_{,commit}, resulting in a pin which is available
> for use. I could confirm this by adding (just as a debugging aid):
>   WARN_ON(!test_bit(FLAG_REQUESTED, &desc->flags));
> early in gpiochip_lock_as_irq, and this statement gets triggered.
>
> Is this intentional ?

IIRC the GPIO can be locked as IRQ without being requested (perhaps
for legacy/historical reasons). But I forgot all code paths anyway, so
I'm expecting that Linus and  or Bart can elaborate this better.

> Does this requesting belong to something else in the codepath from
> request_threaded_irq (and similar) ?
> Could it be something missing in the devicetree for this board ?
>
> Also, I notice that both gpiochip_hierarchy_add_domain and
> gpiochip_reqres_irq call gpiochip_lock_as_irq, and I am surprised I do not
> get any error about this: in my understanding only the first call on a given pin
> should succeed, but with my WARN_ON I am seeing both stack traces and
> no other warning.


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gpiochip_lock_as_irq on pins without FLAG_REQUESTED: bug or feature ?
  2021-06-28 13:40 ` Andy Shevchenko
@ 2021-06-28 13:42   ` Andy Shevchenko
  2021-06-28 22:52     ` Vincent Pelletier
  0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2021-06-28 13:42 UTC (permalink / raw)
  To: Vincent Pelletier
  Cc: Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
	Linux Kernel Mailing List

On Mon, Jun 28, 2021 at 4:40 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Jun 28, 2021 at 6:37 AM Vincent Pelletier <plr.vincent@gmail.com> wrote:

> > While trying to debug an IRQ handling issue on a sifive-unmatched board
> > (which is a very recent board on a recent architecture, so I would not
> > be overly surprised if there were bugs in hiding), I realised that I was able
> > to claim via sysfs GPIO pins which are being actively used as IRQ sources.

And one important note: do NOT use sysfs GPIO interface. Use a GPIO
character device instead.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gpiochip_lock_as_irq on pins without FLAG_REQUESTED: bug or feature ?
  2021-06-28 13:42   ` Andy Shevchenko
@ 2021-06-28 22:52     ` Vincent Pelletier
  2021-06-29 10:19       ` Andy Shevchenko
  2021-07-02  0:24       ` Linus Walleij
  0 siblings, 2 replies; 10+ messages in thread
From: Vincent Pelletier @ 2021-06-28 22:52 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
	Linux Kernel Mailing List

On Mon, Jun 28, 2021 at 10:42 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> And one important note: do NOT use sysfs GPIO interface. Use a GPIO
> character device instead.

I am indeed aware of this. My IRQ issue is unrelated to the gpios
being claimed by anything, and I was doing it while trying to get
more information about the current state of the gpio driver.

For more background, the context of my IRQ issue is:
  PMIC (da9063) /irq -> GPIO pin 1 -> PLIC irq 24
The PMIC has several internal interrupt sources, like the power
button being pressed or the ADC conversion completion signal.
The first time after a boot that I press the power button, I do
get an interrupt and the da9063-onkey driver produces a keypress
input event.
But any further button press does not produce an IRQ. So something
is going wrong in the IRQ acknowledgement.
AFAIK the PLIC (platform-level interrupt controller) works: it is
used for PCIe interrupts, and those work.
The PMIC driver exists since 2013, so I assume any bug would have
been identified long ago.
But I believe the GPIO level has not handled any interrupt until I
enabled the power button event source, and this one is a lot more
recent: gpio-sifive.c from late 2019. So this is where I turned my
attention. Discovering that the pin is somehow only half-claimed
made me wonder if there was some important initialisation step
missing, which could maybe be related to these IRQ issues.

While on this topic, there is a bullet point in
Documentation/driver-api/gpio/driver.rst which I fail to understand:

| - Nominally set all handlers to handle_bad_irq() in the setup call and pass
|   handle_bad_irq() as flow handler parameter in
gpiochip_irqchip_add() if it is
|   expected for GPIO driver that irqchip .set_type() callback will be called
|   before using/enabling each GPIO IRQ. Then set the handler to
|   handle_level_irq() and/or handle_edge_irq() in the irqchip .set_type()
|   callback depending on what your controller supports and what is requested
|   by the consumer.

- why the plural in "set all handlers to handle_bad_irq()" ? Isn't
  there only a single handler in struct gpio_irq_chip ?
- I do not find a function named gpiochip_irqchip_add(), only
  gpiochip_irqchip_add_domain()
- "Then set the handler to [...] in the irqchip .set_type() callback"
  Isn't set_type per-pin, and isn't the interrupt handler chip-level ?

Regards,
-- 
Vincent Pelletier

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gpiochip_lock_as_irq on pins without FLAG_REQUESTED: bug or feature ?
  2021-06-28 22:52     ` Vincent Pelletier
@ 2021-06-29 10:19       ` Andy Shevchenko
  2021-07-01 13:36         ` Vincent Pelletier
  2021-07-02  0:24       ` Linus Walleij
  1 sibling, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2021-06-29 10:19 UTC (permalink / raw)
  To: Vincent Pelletier
  Cc: Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
	Linux Kernel Mailing List

On Tue, Jun 29, 2021 at 1:52 AM Vincent Pelletier <plr.vincent@gmail.com> wrote:
>
> On Mon, Jun 28, 2021 at 10:42 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > And one important note: do NOT use sysfs GPIO interface. Use a GPIO
> > character device instead.
>
> I am indeed aware of this. My IRQ issue is unrelated to the gpios
> being claimed by anything, and I was doing it while trying to get
> more information about the current state of the gpio driver.
>
> For more background, the context of my IRQ issue is:
>   PMIC (da9063) /irq -> GPIO pin 1 -> PLIC irq 24
> The PMIC has several internal interrupt sources, like the power
> button being pressed or the ADC conversion completion signal.

This is the usual case with PMIC. We have similar on x86 machines
(PMIC is represented by an MFD driver with regmap IRQ being involved).

> The first time after a boot that I press the power button, I do
> get an interrupt and the da9063-onkey driver produces a keypress
> input event.
> But any further button press does not produce an IRQ. So something
> is going wrong in the IRQ acknowledgement.
> AFAIK the PLIC (platform-level interrupt controller) works: it is
> used for PCIe interrupts, and those work.
> The PMIC driver exists since 2013, so I assume any bug would have
> been identified long ago.
> But I believe the GPIO level has not handled any interrupt until I
> enabled the power button event source, and this one is a lot more
> recent: gpio-sifive.c from late 2019. So this is where I turned my
> attention. Discovering that the pin is somehow only half-claimed
> made me wonder if there was some important initialisation step
> missing, which could maybe be related to these IRQ issues.
>
> While on this topic, there is a bullet point in
> Documentation/driver-api/gpio/driver.rst which I fail to understand:
>
> | - Nominally set all handlers to handle_bad_irq() in the setup call and pass
> |   handle_bad_irq() as flow handler parameter in
> gpiochip_irqchip_add() if it is
> |   expected for GPIO driver that irqchip .set_type() callback will be called
> |   before using/enabling each GPIO IRQ. Then set the handler to
> |   handle_level_irq() and/or handle_edge_irq() in the irqchip .set_type()
> |   callback depending on what your controller supports and what is requested
> |   by the consumer.
>
> - why the plural in "set all handlers to handle_bad_irq()" ? Isn't
>   there only a single handler in struct gpio_irq_chip ?

Each GPIO line may have its own handler (usually level or edge). I
guess it's written from the GPIO point of view.

> - I do not find a function named gpiochip_irqchip_add(), only
>   gpiochip_irqchip_add_domain()

Missed during update I suppose.
https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git/commit/?h=gpio/for-next&id=f1f37abbe6fc2b1242f78157db76e48dbf9518ee
Feel free to submit a patch!

> - "Then set the handler to [...] in the irqchip .set_type() callback"
>   Isn't set_type per-pin, and isn't the interrupt handler chip-level ?

The idea behind that initially the chip-level IRQ handler is set to
BAD. It means any (spurious) IRQ will be served by it. Now, when one
requests IRQ the framework will call ->irq_set_type() of corresponding
IRQ chip and change the handler for the certain pin (pin-level). So,
the main handler is basically for spurious interrupts only.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gpiochip_lock_as_irq on pins without FLAG_REQUESTED: bug or feature ?
  2021-06-29 10:19       ` Andy Shevchenko
@ 2021-07-01 13:36         ` Vincent Pelletier
  0 siblings, 0 replies; 10+ messages in thread
From: Vincent Pelletier @ 2021-07-01 13:36 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
	Linux Kernel Mailing List

Hello,

On Tue, Jun 29, 2021 at 7:19 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> > - why the plural in "set all handlers to handle_bad_irq()" ? Isn't
> >   there only a single handler in struct gpio_irq_chip ?
>
> Each GPIO line may have its own handler (usually level or edge). I
> guess it's written from the GPIO point of view.
[...]
> > - "Then set the handler to [...] in the irqchip .set_type() callback"
> >   Isn't set_type per-pin, and isn't the interrupt handler chip-level ?
>
> The idea behind that initially the chip-level IRQ handler is set to
> BAD. It means any (spurious) IRQ will be served by it. Now, when one
> requests IRQ the framework will call ->irq_set_type() of corresponding
> IRQ chip and change the handler for the certain pin (pin-level). So,
> the main handler is basically for spurious interrupts only.

I think I found what I was missing: I was only seeing
  (struct gpio_irq_chip *)->handler = handle_bad_irq;
and was completely missing
  irq_set_handler_locked((struct irq_data *), handle_..._irq);
hence my confusion about these points.
Thanks for this extra push which led me to these.
Maybe the doc should mention this function ?

> > - I do not find a function named gpiochip_irqchip_add(), only
> >   gpiochip_irqchip_add_domain()
>
> Missed during update I suppose.
> https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git/commit/?h=gpio/for-next&id=f1f37abbe6fc2b1242f78157db76e48dbf9518ee
> Feel free to submit a patch!

Makes sense. Before trying to fix the documentation I intended
to get to the end of my IRQ issues though, as I feel I am still
lacking a lot of understanding which will be needed to produce a
decent documentation patch (or three).

But I feel I am as far as I can go, as I cannot even tell what is normal
and what is not, for lack of what I would identify as a reference setup
in other machines. I have no idea what to try next, and I am ashamed to
say I am working on this issue for over a week (outside of work hours,
but still).

So here goes all I spotted, if anyone reading can tell the good from
the bad:

The high-level issue: I added a devicetree entry to enable the power-button
function of a DA9063 PMIC. This is AFAICT the first subsystem of this chip
on this board (hifive unmatched) which produces IRQs in normal use (meaning
outside of catastrophic failures on the board, like the PMIC complaining
about an overcurrent condition).

I initially thought it worked fine: short push of the button, system shuts
down cleanly.

Then I disabled power button handling in systemd-logind, and only the first
press causes a key press event, all further presses do nothing.

I hacked a script to peek at IRQ registers behind the kernel, and here is
what I see:

vincent@riscv:~$ sudo ./unmatched_gpio_irq_debug.py 1
GPIO 1: dir=in  in=1 out=0 irq_en=low irq_pending=high
PLIC 24: irq=False hart=4
vincent@riscv:~$ sudo ./unmatched_gpio_irq_debug.py 1
GPIO 1: dir=in  in=1 out=0 irq_en=low irq_pending=rise,high,low
PLIC 24: irq=False hart=4
vincent@riscv:~$ sudo ./unmatched_gpio_irq_debug.py 1
GPIO 1: dir=in  in=0 out=0 irq_en=low irq_pending=rise,fall,high,low
PLIC 24: irq=False hart=4

Note:
- GPIO pin 1 is active low, consistently with the da9063 code and the gpio
  irq_en line.
- after the first press, the gpio pin reads as "high", so the irs is ack'ed
  at the da9063 level
- after the first press, the pending irq events are all but "falling", which
  to me indicate the GPIO-level IRQ was ack'ed while the pin was still low,
  so it immediately became pending again. Knowing the GPIO driver clears
  *all* pending interrupts, I understand "rise,high,low" as meaning the GPIO
  controller saw the pin go from low to high after it was cleared, which also
  hints that the da9063's IRQ was cleared after the GPIO, which seems wrong
  for a level interrupt - but I am not 100% sure.
- ...but the PLIC 24 pin (corresponding to GPIO 1 pin's IRQ) does not have a
  pending irq, which suggests it missed the GPIO IRQ re-triggering
- and after a second key press, the only change is that now the GPIO chip did
  see a falling edge, and it now has all its pending bits active for this pin.

On the /proc/interrupts front, here is what I see:

          CPU0       CPU1       CPU2       CPU3
 1:        637          0          0          0  SiFive PLIC  39
10010000.serial
 2:          0          0          0          0  SiFive PLIC  40
10011000.serial
 3:       1829          0          0          0  SiFive PLIC  52  10030000.i2c
 4:          0          0          0          0  SiFive PLIC  41  10040000.spi
 5:       7647      10727       9670      11094  RISC-V INTC   5  riscv-timer
 6:         96          0          0          0  SiFive PLIC  43  10050000.spi
 7:          0          0          0          0  SiFive PLIC  55  eth0
17:          0          0          0          0  SiFive PLIC  19  l2_ecc
18:          1          0          0          0  SiFive PLIC  21  l2_ecc
19:          0          0          0          0  SiFive PLIC  22  l2_ecc
20:          0          0          0          0  SiFive PLIC  20  l2_ecc
46:          0          0          0          0   PCI-MSI   0  PCIe PME, aerdrv
53:         22          0          0          0   PCI-MSI 3145728  nvme0q0
54:          1          0          0          0  sifive-gpio   1  da9063-irq
55:          1          0          0          0  da9063-irq   0  ONKEY
56:          0          0          0          0  da9063-irq   1  ALARM
63:          0          0          0          0  da9063-irq   8  LDO_LIM
84:        694          0          0          0   PCI-MSI 3145729  nvme0q1
85:       1780          0          0          0   PCI-MSI 3145730  nvme0q2
86:       1092          0          0          0   PCI-MSI 3145731  nvme0q3
87:       1303          0          0          0   PCI-MSI 3145732  nvme0q4
88:       6523          0          0          0   PCI-MSI 2097152  xhci_hcd
89:          0          0          0          0   PCI-MSI 2097153  xhci_hcd
90:          0          0          0          0   PCI-MSI 2097154  xhci_hcd
91:          0          0          0          0   PCI-MSI 2097155  xhci_hcd
92:          0          0          0          0   PCI-MSI 2097156  xhci_hcd
93:          0          0          0          0  sifive-gpio   6  lm90
94:       1029          0          0          0   PCI-MSI 2621440
iwlwifi: default queue
95:        282          0          0          0   PCI-MSI 2621441
iwlwifi: queue 1
96:          5          0          0          0   PCI-MSI 2621442
iwlwifi: queue 2
97:         31          0          0          0   PCI-MSI 2621443
iwlwifi: queue 3
98:          8          0          0          0   PCI-MSI 2621444
iwlwifi: queue 4
99:          5          0          0          0   PCI-MSI 2621445
iwlwifi: exception
IPI0:       109        125        110        104  Rescheduling interrupts
IPI1:      5072      10959       6008      12207  Function call interrupts
IPI2:         0          0          0          0  CPU stop interrupts
IPI3:         0          0          0          0  IRQ work interrupts
IPI4:         0          0          0          0  Timer broadcast interrupts

I note the absence of a "SiFive PLIC  24 sifive-gpio" line, but
I do not know if this is to be expected.

In kernel/debug/irqs, I further see:

# cat irqs/55
handler:  handle_bad_irq
device:   (null)
status:   0x00008508
           _IRQ_NOPROBE
           _IRQ_NESTED_THREAD
istate:   0x00000020
           IRQS_ONESHOT
ddepth:   0
wdepth:   0
dstate:   0x00402208
           IRQ_TYPE_LEVEL_LOW
           IRQD_LEVEL
           IRQD_ACTIVATED
           IRQD_IRQ_STARTED
node:     0
affinity: 0-3
domain:  :soc:i2c@10030000:pmic@58
hwirq:   0x0
chip:    da9063-irq
 flags:   0x0

# cat irqs/54
handler:  handle_level_irq
device:   (null)
status:   0x00000508
           _IRQ_NOPROBE
istate:   0x00000020
           IRQS_ONESHOT
ddepth:   0
wdepth:   0
dstate:   0x02403208
           IRQ_TYPE_LEVEL_LOW
           IRQD_LEVEL
           IRQD_ACTIVATED
           IRQD_IRQ_STARTED
           IRQD_AFFINITY_SET
           IRQD_DEFAULT_TRIGGER_SET
node:     0
affinity: 0-3
domain:  :soc:gpio@10060000
hwirq:   0x1
chip:    sifive-gpio
 flags:   0x0
parent:
   domain:  :soc:interrupt-controller@c000000
    hwirq:   0x18
    chip:    SiFive PLIC
     flags:   0x0

# cat irqs/22
handler:  handle_fasteoi_irq
device:   (null)
status:   0x00000400
           _IRQ_NOPROBE
istate:   0x00000000
ddepth:   1
wdepth:   0
dstate:   0x02031000
           IRQD_IRQ_DISABLED
           IRQD_IRQ_MASKED
           IRQD_AFFINITY_SET
           IRQD_DEFAULT_TRIGGER_SET
node:     0
affinity: 0-3
domain:  :soc:interrupt-controller@c000000
hwirq:   0x18
chip:    SiFive PLIC
 flags:   0x0

So:
- the GPIO driver seems to have properly told the kernel that
  it reports to interrupt-controller@c000000's domain,
  on line 24.
- ...but that line (soft irq 22 on this specific boot) is
  disabled and masked.
- the da9063 does not seem to have express its reliance on
  the gpio's irq domain, but still the gpio irq is enabled

What is abnormal here ?
What could be a probable cause ? PLIC driver ? GPIO driver ?
da9063 driver ? devicetree ?

Side note: the above traces are with a few of my hacks, they
may not fully represent a clean kernel. I did confirm that
the high-level symptom still exists.

Regards,
--
Vincent Pelletier

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gpiochip_lock_as_irq on pins without FLAG_REQUESTED: bug or feature ?
  2021-06-28  3:36 gpiochip_lock_as_irq on pins without FLAG_REQUESTED: bug or feature ? Vincent Pelletier
  2021-06-28 13:40 ` Andy Shevchenko
@ 2021-07-02  0:09 ` Linus Walleij
  2021-07-02 10:48   ` Vincent Pelletier
  1 sibling, 1 reply; 10+ messages in thread
From: Linus Walleij @ 2021-07-02  0:09 UTC (permalink / raw)
  To: Vincent Pelletier
  Cc: Bartosz Golaszewski, open list:GPIO SUBSYSTEM, Linux Kernel Mailing List

On Mon, Jun 28, 2021 at 5:37 AM Vincent Pelletier <plr.vincent@gmail.com> wrote:

> gpiolib (gpiochip_irq_reqres, gpiochip_reqres_irq, gpiochip_lock_as_irq)
> does not call gpiod_request_{,commit}, resulting in a pin which is available
> for use.

Nope and they should not.

> Is this intentional ?

Yes.

The basic reason is that gpiochips and irqchips are orthogonal.
You can request an IRQ on a GPIO line without requesting the
GPIO line for anything else.

This is also used when drivers want to inspect the state of a GPIO
line (read the value) while the same line triggers IRQs. This is
perfectly legal. An extreme example is:
drivers/media/cec/platform/cec-gpio/cec-gpio.c

There is sometimes confusion around gpiod_to_irq().
This is just a convenience function locating the IRQ for a certain
GPIO. Both resources still have to be requested separately
and there is no dependency between them, they are just
often implemented in the same driver, using two different
subsystem APIs, in the end.

sysfs can't be used as any guide here since it conflates GPIO lines
and IRQs and provides several dangerous ways to shoot oneself
in the foot. The chardev does a better job at keeping this in
order.

> Also, I notice that both gpiochip_hierarchy_add_domain and
> gpiochip_reqres_irq call gpiochip_lock_as_irq, and I am surprised I do not
> get any error about this: in my understanding only the first call on a given pin
> should succeed, but with my WARN_ON I am seeing both stack traces and
> no other warning.

Hm that may be a subtle bug.

The state is just a bool so the first to leave will turn out the lights
for whoever is left in the room :P

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gpiochip_lock_as_irq on pins without FLAG_REQUESTED: bug or feature ?
  2021-06-28 22:52     ` Vincent Pelletier
  2021-06-29 10:19       ` Andy Shevchenko
@ 2021-07-02  0:24       ` Linus Walleij
  1 sibling, 0 replies; 10+ messages in thread
From: Linus Walleij @ 2021-07-02  0:24 UTC (permalink / raw)
  To: Vincent Pelletier
  Cc: Andy Shevchenko, Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
	Linux Kernel Mailing List

On Tue, Jun 29, 2021 at 12:52 AM Vincent Pelletier
<plr.vincent@gmail.com> wrote:

> While on this topic, there is a bullet point in
> Documentation/driver-api/gpio/driver.rst which I fail to understand:
>
> | - Nominally set all handlers to handle_bad_irq() in the setup call and pass
> |   handle_bad_irq() as flow handler parameter in
> gpiochip_irqchip_add() if it is
> |   expected for GPIO driver that irqchip .set_type() callback will be called
> |   before using/enabling each GPIO IRQ. Then set the handler to
> |   handle_level_irq() and/or handle_edge_irq() in the irqchip .set_type()
> |   callback depending on what your controller supports and what is requested
> |   by the consumer.
>
> - why the plural in "set all handlers to handle_bad_irq()" ? Isn't
>   there only a single handler in struct gpio_irq_chip ?

girq->handler will be applied (inside gpiolib) successively to
all the irq lines on the irqchip, as many as you have GPIO lines.
It will then be set to something usable when we get
to ->set_type() when an IRQ is being requested.

> - I do not find a function named gpiochip_irqchip_add(), only
>   gpiochip_irqchip_add_domain()

This refers to the old API that didn't add the irqchip with the
gpiochip. Needs updating. Patches welcome ;)

> - "Then set the handler to [...] in the irqchip .set_type() callback"
>   Isn't set_type per-pin, and isn't the interrupt handler chip-level ?

Those are two different things. The ->set_type() callback is
per-line (call it lines, not pins, because terminology can be
confusing otherwise), these are the interrupt handlers per-line
apart from of course the handler the consumer provides in
e.g. request_[threaded_]irq().

There is usually another cascading interrupt handler for the GPIO
irqchip itself, usually what is passed in girq->parent_handler
*BUT* you are looking at an hierarchical interrupt controller with
one line per gpio line to the next level of irq controller, so
you do not see this. You don't have a chip-level interrupt
handler in the sifive driver, all falls through upward in the
hierarchy to the next overarching interrupt controller.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gpiochip_lock_as_irq on pins without FLAG_REQUESTED: bug or feature ?
  2021-07-02  0:09 ` Linus Walleij
@ 2021-07-02 10:48   ` Vincent Pelletier
  2021-07-02 21:46     ` Linus Walleij
  0 siblings, 1 reply; 10+ messages in thread
From: Vincent Pelletier @ 2021-07-02 10:48 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Bartosz Golaszewski, open list:GPIO SUBSYSTEM, Linux Kernel Mailing List

On Fri, 2 Jul 2021 02:09:17 +0200, Linus Walleij <linus.walleij@linaro.org> wrote:
> The basic reason is that gpiochips and irqchips are orthogonal.
> You can request an IRQ on a GPIO line without requesting the
> GPIO line for anything else.
> 
> This is also used when drivers want to inspect the state of a GPIO
> line (read the value) while the same line triggers IRQs. This is
> perfectly legal. An extreme example is:
> drivers/media/cec/platform/cec-gpio/cec-gpio.c

Interesting, thank you very much.

> On Mon, Jun 28, 2021 at 5:37 AM Vincent Pelletier <plr.vincent@gmail.com> wrote:
> > Also, I notice that both gpiochip_hierarchy_add_domain and
> > gpiochip_reqres_irq call gpiochip_lock_as_irq, and I am surprised I do not
> > get any error about this: in my understanding only the first call on a given pin
> > should succeed, but with my WARN_ON I am seeing both stack traces and
> > no other warning.  
> 
> Hm that may be a subtle bug.
> 
> The state is just a bool so the first to leave will turn out the lights
> for whoever is left in the room :P

Actually my question came from yet another misunderstanding on my side:
I expected this function to act as an exclusive access control (because
of the "lock" in the name), but I then realised my assumption is wrong.

So while this could be a subtle bug indeed (irq_disable without
irq_shutdown is not the exact same state as right after irq_startup),
it's likely not the one I'm chasing - if it leads to any actual issue
at all.

Regards,
-- 
Vincent Pelletier
GPG fingerprint 983A E8B7 3B91 1598 7A92 3845 CAC9 3691 4257 B0C1

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gpiochip_lock_as_irq on pins without FLAG_REQUESTED: bug or feature ?
  2021-07-02 10:48   ` Vincent Pelletier
@ 2021-07-02 21:46     ` Linus Walleij
  0 siblings, 0 replies; 10+ messages in thread
From: Linus Walleij @ 2021-07-02 21:46 UTC (permalink / raw)
  To: Vincent Pelletier
  Cc: Bartosz Golaszewski, open list:GPIO SUBSYSTEM, Linux Kernel Mailing List

On Fri, Jul 2, 2021 at 12:48 PM Vincent Pelletier <plr.vincent@gmail.com> wrote:

> gpiochip_lock_as_irq,
(...)
> Actually my question came from yet another misunderstanding on my side:
> I expected this function to act as an exclusive access control (because
> of the "lock" in the name), but I then realised my assumption is wrong.

Well it locks its use to be compliant with being used for IRQ.
So it cannot be turned into an output for example. While reading
its value as input is fine.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-07-02 21:46 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-28  3:36 gpiochip_lock_as_irq on pins without FLAG_REQUESTED: bug or feature ? Vincent Pelletier
2021-06-28 13:40 ` Andy Shevchenko
2021-06-28 13:42   ` Andy Shevchenko
2021-06-28 22:52     ` Vincent Pelletier
2021-06-29 10:19       ` Andy Shevchenko
2021-07-01 13:36         ` Vincent Pelletier
2021-07-02  0:24       ` Linus Walleij
2021-07-02  0:09 ` Linus Walleij
2021-07-02 10:48   ` Vincent Pelletier
2021-07-02 21:46     ` Linus Walleij

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.