From: Grygorii Strashko <[email protected]>
Now GPIOs, wich are requested as IRQ only, will not be displayed
through GPIO debugfs. For example:
# cat /proc/interrupts
CPU0 CPU1
...
209: 0 0 4805d000.gpio 11 Edge 0-0021
# cat /debug/gpio
...
GPIOs 160-191, platform/4805d000.gpio, gpio:
<--- no info about gpio used as IRQ only here
GPIOs 192-223, platform/48051000.gpio, gpio:
gpio-203 (vtt_fixed ) out hi
...
Hence, improve GPIO debugfs code to show such kind of gpio and print
IRQ number also. In addition, add marker "requested" for GPIOs wich
were requested by using gpioX_request().
After this patch sys/kernel/debug/gpio will produce following output:
# cat /debug/gpio
...
GPIOs 160-191, platform/4805d000.gpio, gpio:
gpio-171 ((null) ) in hi IRQ209
GPIOs 192-223, platform/48051000.gpio, gpio:
gpio-203 (vtt_fixed ) out hi requested
Signed-off-by: Grygorii Strashko <[email protected]>
---
drivers/gpio/gpiolib.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 59eaa23..ea11706 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2259,19 +2259,23 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
int is_irq;
for (i = 0; i < chip->ngpio; i++, gpio++, gdesc++) {
- if (!test_bit(FLAG_REQUESTED, &gdesc->flags))
+ if (!test_bit(FLAG_REQUESTED, &gdesc->flags) &&
+ !test_bit(FLAG_USED_AS_IRQ, &gdesc->flags))
continue;
gpiod_get_direction(gdesc);
is_out = test_bit(FLAG_IS_OUT, &gdesc->flags);
is_irq = test_bit(FLAG_USED_AS_IRQ, &gdesc->flags);
- seq_printf(s, " gpio-%-3d (%-20.20s) %s %s %s",
+ seq_printf(s, " gpio-%-3d (%-20.20s) %s %s",
gpio, gdesc->label,
is_out ? "out" : "in ",
chip->get
? (chip->get(chip, i) ? "hi" : "lo")
- : "? ",
- is_irq ? "IRQ" : " ");
+ : "? ");
+ if (is_irq)
+ seq_printf(s, " IRQ%d", gpiod_to_irq(gdesc));
+ if (test_bit(FLAG_REQUESTED, &gdesc->flags))
+ seq_puts(s, " requested");
seq_printf(s, "\n");
}
}
--
1.9.1
On Fri, May 15, 2015 at 04:25:21PM +0300, [email protected] wrote:
> From: Grygorii Strashko <[email protected]>
>
> Now GPIOs, wich are requested as IRQ only, will not be displayed
> through GPIO debugfs. For example:
> # cat /proc/interrupts
> CPU0 CPU1
> ...
> 209: 0 0 4805d000.gpio 11 Edge 0-0021
>
> # cat /debug/gpio
> ...
> GPIOs 160-191, platform/4805d000.gpio, gpio:
> <--- no info about gpio used as IRQ only here
>
> GPIOs 192-223, platform/48051000.gpio, gpio:
> gpio-203 (vtt_fixed ) out hi
> ...
>
> Hence, improve GPIO debugfs code to show such kind of gpio and print
> IRQ number also. In addition, add marker "requested" for GPIOs wich
> were requested by using gpioX_request().
>
> After this patch sys/kernel/debug/gpio will produce following output:
>
> # cat /debug/gpio
> ...
> GPIOs 160-191, platform/4805d000.gpio, gpio:
> gpio-171 ((null) ) in hi IRQ209
>
> GPIOs 192-223, platform/48051000.gpio, gpio:
> gpio-203 (vtt_fixed ) out hi requested
This is backwards. All gpios *should* be requested. *If* we are to
include not-requested gpios in the debug output, then it is those pins
that need to be marked as not-requested.
The irq-number mapping could perhaps be useful, but it should go in a
separate patch. I'd suggest adding a '-' before the irq-number (e.g.
"IRQ-209").
Thanks,
Johan
Hi Johan,
On 05/18/2015 02:02 PM, Johan Hovold wrote:
> On Fri, May 15, 2015 at 04:25:21PM +0300, [email protected] wrote:
>> From: Grygorii Strashko <[email protected]>
>>
>> Now GPIOs, wich are requested as IRQ only, will not be displayed
>> through GPIO debugfs. For example:
>> # cat /proc/interrupts
>> CPU0 CPU1
>> ...
>> 209: 0 0 4805d000.gpio 11 Edge 0-0021
>>
>> # cat /debug/gpio
>> ...
>> GPIOs 160-191, platform/4805d000.gpio, gpio:
>> <--- no info about gpio used as IRQ only here
>>
>> GPIOs 192-223, platform/48051000.gpio, gpio:
>> gpio-203 (vtt_fixed ) out hi
>> ...
>>
>> Hence, improve GPIO debugfs code to show such kind of gpio and print
>> IRQ number also. In addition, add marker "requested" for GPIOs wich
>> were requested by using gpioX_request().
>>
>> After this patch sys/kernel/debug/gpio will produce following output:
>>
>> # cat /debug/gpio
>> ...
>> GPIOs 160-191, platform/4805d000.gpio, gpio:
>> gpio-171 ((null) ) in hi IRQ209
>>
>> GPIOs 192-223, platform/48051000.gpio, gpio:
>> gpio-203 (vtt_fixed ) out hi requested
>
> This is backwards. All gpios *should* be requested. *If* we are to
> include not-requested gpios in the debug output, then it is those pins
> that need to be marked as not-requested.
Sry, but I didn't fully understand your point here ( - Why is it backward?
Now GPIO can be requested in three ways:
1) As pure GPIO (gpioX_request())
2) As pure GPIO IRQ, especially in DT boot case.
DT:
interrupt-parent = <&gpio6>;
interrupts = <11 IRQ_TYPE_EDGE_FALLING>;
Code:
platform_get_irq() or of_irq_get()
request_irq()
3) combination of (1) and (2) with one restriction
- GPIO direction should be 'In' and can't be changed.
Personally I'm using this case for debug purposes to do a fast check of GPIO pin state
through GPIO sysfs (GPIO export) when such GPIO is used as GPIO IRQ in some driver
and I don't see that corresponding IRQ is triggered as expected.
So, this patch just adds missed information in GPIO debugfs for the case (2) in general.
Of course, format of the marker "requested" is discussable. Could be:
- "requested" --> "not-requested"
- "I R" or "I G" where I - IRQ, G - GPIO, R - requested
- etc.
>
> The irq-number mapping could perhaps be useful, but it should go in a
> separate patch. I'd suggest adding a '-' before the irq-number (e.g.
> "IRQ-209").
I've thought about this, but finally decided not to split it.
Could be done, if you insist )
--
regards,
-grygorii
On Mon, May 18, 2015 at 04:06:08PM +0300, [email protected] wrote:
> Hi Johan,
> On 05/18/2015 02:02 PM, Johan Hovold wrote:
> > On Fri, May 15, 2015 at 04:25:21PM +0300, [email protected] wrote:
> >> From: Grygorii Strashko <[email protected]>
> >>
> >> Now GPIOs, wich are requested as IRQ only, will not be displayed
> >> through GPIO debugfs. For example:
> >> # cat /proc/interrupts
> >> CPU0 CPU1
> >> ...
> >> 209: 0 0 4805d000.gpio 11 Edge 0-0021
> >>
> >> # cat /debug/gpio
> >> ...
> >> GPIOs 160-191, platform/4805d000.gpio, gpio:
> >> <--- no info about gpio used as IRQ only here
> >>
> >> GPIOs 192-223, platform/48051000.gpio, gpio:
> >> gpio-203 (vtt_fixed ) out hi
> >> ...
> >>
> >> Hence, improve GPIO debugfs code to show such kind of gpio and print
> >> IRQ number also. In addition, add marker "requested" for GPIOs wich
> >> were requested by using gpioX_request().
> >>
> >> After this patch sys/kernel/debug/gpio will produce following output:
> >>
> >> # cat /debug/gpio
> >> ...
> >> GPIOs 160-191, platform/4805d000.gpio, gpio:
> >> gpio-171 ((null) ) in hi IRQ209
> >>
> >> GPIOs 192-223, platform/48051000.gpio, gpio:
> >> gpio-203 (vtt_fixed ) out hi requested
> >
> > This is backwards. All gpios *should* be requested. *If* we are to
> > include not-requested gpios in the debug output, then it is those pins
> > that need to be marked as not-requested.
>
> Sry, but I didn't fully understand your point here ( - Why is it
> backward?
My main point was that you should mark the pins used for irq only
instead of the other way round.
We should also consider making sure that pins only used for irq are also
requested.
With the current sysfs-interface it is for example to possible to
request the same irq, and when that pin is unexported (or only irq
released) the pin will no longer be marked for irq (just a flag that is
cleared) so that the direction can be changed...
gpiolib also depends on pins to be requested to prevent the
gpio-controller driver from being unloaded while in use.
[...]
> Of course, format of the marker "requested" is discussable. Could be:
> - "requested" --> "not-requested"
> - "I R" or "I G" where I - IRQ, G - GPIO, R - requested
> - etc.
How about instead of
GPIOs 160-191, platform/4805d000.gpio, gpio:
gpio-171 ((null) ) in hi IRQ209
you do something like:
GPIOs 160-191, platform/4805d000.gpio, gpio:
gpio-171 (<irq-only> ) in hi IRQ-209
> > The irq-number mapping could perhaps be useful, but it should go in a
> > separate patch. I'd suggest adding a '-' before the irq-number (e.g.
> > "IRQ-209").
>
> I've thought about this, but finally decided not to split it.
> Could be done, if you insist )
Yes, you should split self-contained, logical changes into separate
patches (so they can be reviewed, applied or rejected separately as
well).
Johan
On 05/18/2015 06:08 PM, Johan Hovold wrote:
> On Mon, May 18, 2015 at 04:06:08PM +0300, [email protected] wrote:
>> On 05/18/2015 02:02 PM, Johan Hovold wrote:
>>> On Fri, May 15, 2015 at 04:25:21PM +0300, [email protected] wrote:
>>>> From: Grygorii Strashko <[email protected]>
>>>>
>>>> Now GPIOs, wich are requested as IRQ only, will not be displayed
>>>> through GPIO debugfs. For example:
>>>> # cat /proc/interrupts
>>>> CPU0 CPU1
>>>> ...
>>>> 209: 0 0 4805d000.gpio 11 Edge 0-0021
>>>>
>>>> # cat /debug/gpio
>>>> ...
>>>> GPIOs 160-191, platform/4805d000.gpio, gpio:
>>>> <--- no info about gpio used as IRQ only here
>>>>
>>>> GPIOs 192-223, platform/48051000.gpio, gpio:
>>>> gpio-203 (vtt_fixed ) out hi
>>>> ...
>>>>
>>>> Hence, improve GPIO debugfs code to show such kind of gpio and print
>>>> IRQ number also. In addition, add marker "requested" for GPIOs wich
>>>> were requested by using gpioX_request().
>>>>
>>>> After this patch sys/kernel/debug/gpio will produce following output:
>>>>
>>>> # cat /debug/gpio
>>>> ...
>>>> GPIOs 160-191, platform/4805d000.gpio, gpio:
>>>> gpio-171 ((null) ) in hi IRQ209
>>>>
>>>> GPIOs 192-223, platform/48051000.gpio, gpio:
>>>> gpio-203 (vtt_fixed ) out hi requested
>>>
>>> This is backwards. All gpios *should* be requested. *If* we are to
>>> include not-requested gpios in the debug output, then it is those pins
>>> that need to be marked as not-requested.
>>
>> Sry, but I didn't fully understand your point here ( - Why is it
>> backward?
>
> My main point was that you should mark the pins used for irq only
> instead of the other way round.
>
> We should also consider making sure that pins only used for irq are also
> requested.
>
> With the current sysfs-interface it is for example to possible to
> request the same irq, and when that pin is unexported (or only irq
> released) the pin will no longer be marked for irq (just a flag that is
> cleared) so that the direction can be changed...
>
> gpiolib also depends on pins to be requested to prevent the
> gpio-controller driver from being unloaded while in use.
>
> [...]
>
>> Of course, format of the marker "requested" is discussable. Could be:
>> - "requested" --> "not-requested"
>> - "I R" or "I G" where I - IRQ, G - GPIO, R - requested
>> - etc.
>
> How about instead of
>
> GPIOs 160-191, platform/4805d000.gpio, gpio:
> gpio-171 ((null) ) in hi IRQ209
>
> you do something like:
>
> GPIOs 160-191, platform/4805d000.gpio, gpio:
> gpio-171 (<irq-only> ) in hi IRQ-209
In general agree, but i propose to do it as
GPIOs 160-191, platform/4805d000.gpio, gpio:
gpio-171 ((null) ) in hi IRQ-209 <irq-only>
My intention is - this interface could be considered as more or less stable, so
it is better to add additional information at the end of each line to avoid
potential breakage of User space SW (test/debug scripts).
if you agree I'll update and re-send.
>>> The irq-number mapping could perhaps be useful, but it should go in a
>>> separate patch. I'd suggest adding a '-' before the irq-number (e.g.
>>> "IRQ-209").
>>
>> I've thought about this, but finally decided not to split it.
>> Could be done, if you insist )
>
> Yes, you should split self-contained, logical changes into separate
> patches (so they can be reviewed, applied or rejected separately as
> well).
np. There will be two patches
- one to display "irq-only" gpios
- second to add IRQ number
Thanks.
--
regards,
-grygorii
On Mon, May 18, 2015 at 06:17:45PM +0300, [email protected] wrote:
> On 05/18/2015 06:08 PM, Johan Hovold wrote:
> > How about instead of
> >
> > GPIOs 160-191, platform/4805d000.gpio, gpio:
> > gpio-171 ((null) ) in hi IRQ209
> >
> > you do something like:
> >
> > GPIOs 160-191, platform/4805d000.gpio, gpio:
> > gpio-171 (<irq-only> ) in hi IRQ-209
>
> In general agree, but i propose to do it as
> GPIOs 160-191, platform/4805d000.gpio, gpio:
> gpio-171 ((null) ) in hi IRQ-209 <irq-only>
I have no strong opinion on whether to use the name-field here or not.
Using the name-field rather than adding a new one could perhaps be
less confusing to current parsers.
> My intention is - this interface could be considered as more or less
> stable, so it is better to add additional information at the end of
> each line to avoid potential breakage of User space SW (test/debug
> scripts).
But if the interface is considered stable (and some people do) you would
not be able to add anything here. This *is* a real issue, but I'll defer
this one to Linus and Alexandre.
Perhaps we should just leave things as they are.
Johan
On Mon, May 18, 2015 at 5:17 PM, [email protected]
<[email protected]> wrote:
> On 05/18/2015 06:08 PM, Johan Hovold wrote:
>> GPIOs 160-191, platform/4805d000.gpio, gpio:
>> gpio-171 (<irq-only> ) in hi IRQ-209
>
> In general agree, but i propose to do it as
> GPIOs 160-191, platform/4805d000.gpio, gpio:
> gpio-171 ((null) ) in hi IRQ-209 <irq-only>
>
> My intention is - this interface could be considered as more or less stable, so
> it is better to add additional information at the end of each line to avoid
> potential breakage of User space SW (test/debug scripts).
What? If I wanted a stable interface I would use sysfs and document
the ABI in Documentation/ABI/*.
debugfs is not ABI.
Debugfs is instable by definition, it is not for production. If tests depend on
it they need to be ready to break and be updated, and in such case
it is a very very good idea to put any such tests in tools/* in the
kernel itself, as does trace-cmd and friends so you can patch the
tests at the same time you patch the code.
Yours,
Linus Walleij
On Mon, May 18, 2015 at 1:02 PM, Johan Hovold <[email protected]> wrote:
> On Fri, May 15, 2015 at 04:25:21PM +0300, [email protected] wrote:
>> GPIOs 192-223, platform/48051000.gpio, gpio:
>> gpio-203 (vtt_fixed ) out hi requested
>
> This is backwards. All gpios *should* be requested. *If* we are to
> include not-requested gpios in the debug output, then it is those pins
> that need to be marked as not-requested.
It depends, really. As concluded in earlier discussions when we
introduced gpiochip_[un]lock_as_irq() the gpiolib and irqchip APIs
are essentially orthogonal.
At one point I was suggesting that any driver using a GPIO for
IRQ should first request the gpio line with gpiod_get() and then
use gpio_to_irq() on the descriptor. Always.
However as it turns out (and after being hammered down by
the DT and ACPI people for this stance) I changed opinion,
because even if that stringence would be nice, it doesn't
abstract the hardware well enough for drivers that "just need
an IRQ". These don't care of whether a line to the primary
IRQ controller or a dedicated external IRQ pin or some
cascaded GPIO chip is providing the IRQ, they just want
their IRQ resource.
If I would persist in my stance that the API must be used like
so it would have implications on how HW descriptions such
as DT or ACPI describe their resources and essentially put
requirements out to other operating systems than Linux using
DT or ACPI to do it the same way for this reason. That would
be unreasonable.
So it was deemed these resource APIs need to be orthogonal.
The same would apply to any other resource that is similar to a
GPIO and an IRQ line, don't know what a good example would be.
So to atleast try to safeguard from a scenario such as
- Client A requests IRQ from the irqchip side of the API
and sets up a level active-low IRQ on it
- Client B request the same line as GPIO
- Client B sets it to output and drivers it low.
- Client A crashes in an infinite IRQ loop as that line
is not hammered low and will generate IRQs until
the end of time.
I introduced the gpiochip_[un]lock_as_irq() calls so we
could safeguard against this. Notably that blocks client A
from setting the line as output. I hope.
I thought this would mean the line would only be used as IRQ
in this case, so I could block any gpiod_get() calls to that
line but *of course* some driver is using the IRQ and snooping
into the GPIO value at the same time. So can't simplify things
like so either.
Maybe I'm smashing open doors here...
Anyway to get back to the original statement:
> This is backwards. All gpios *should* be requested. *If* we are to
> include not-requested gpios in the debug output, then it is those pins
> that need to be marked as not-requested.
This is correct, all GPIOs accessed *as gpios* should be
requested, no matter if they are then cast to IRQs by gpiod_to_irq
or not. However if the same hardware is used as only "some IRQ"
through it's irqchip interface, it needs not be requested, but
that is by definition not a GPIO, it is an IRQ.
Yours,
Linus Walleij
Hi Linus,
On 05/19/2015 05:12 PM, Linus Walleij wrote:
> On Mon, May 18, 2015 at 5:17 PM, [email protected]
> <[email protected]> wrote:
>> On 05/18/2015 06:08 PM, Johan Hovold wrote:
>
>>> GPIOs 160-191, platform/4805d000.gpio, gpio:
>>> gpio-171 (<irq-only> ) in hi IRQ-209
>>
>> In general agree, but i propose to do it as
>> GPIOs 160-191, platform/4805d000.gpio, gpio:
>> gpio-171 ((null) ) in hi IRQ-209 <irq-only>
>>
>> My intention is - this interface could be considered as more or less stable, so
>> it is better to add additional information at the end of each line to avoid
>> potential breakage of User space SW (test/debug scripts).
>
> What? If I wanted a stable interface I would use sysfs and document
> the ABI in Documentation/ABI/*.
>
> debugfs is not ABI.
>
> Debugfs is instable by definition, it is not for production. If tests depend on
> it they need to be ready to break and be updated, and in such case
> it is a very very good idea to put any such tests in tools/* in the
> kernel itself, as does trace-cmd and friends so you can patch the
> tests at the same time you patch the code.
Okay. Sorry, My comment was not fully correct - keyword was "more or less stable"
and of course it is not ABI.
Any way, the question is till here - How would it better to do?
gpio-171 (<irq-only> ) in hi IRQ-209
-- or --
gpio-171 ((null) ) in hi IRQ-209 <irq-only>
Thanks a lot for your comments.
--
regards,
-grygorii
On Tue, May 19, 2015 at 4:37 PM, [email protected]
<[email protected]> wrote:
> Any way, the question is till here - How would it better to do?
> gpio-171 (<irq-only> ) in hi IRQ-209
> -- or --
> gpio-171 ((null) ) in hi IRQ-209 <irq-only>
The latter I think. No strong opinion really.
Yours,
Linus Walleij
On Tue, May 19, 2015 at 04:12:35PM +0200, Linus Walleij wrote:
> On Mon, May 18, 2015 at 5:17 PM, [email protected]
> <[email protected]> wrote:
> > On 05/18/2015 06:08 PM, Johan Hovold wrote:
>
> >> GPIOs 160-191, platform/4805d000.gpio, gpio:
> >> gpio-171 (<irq-only> ) in hi IRQ-209
> >
> > In general agree, but i propose to do it as
> > GPIOs 160-191, platform/4805d000.gpio, gpio:
> > gpio-171 ((null) ) in hi IRQ-209 <irq-only>
> >
> > My intention is - this interface could be considered as more or less stable, so
> > it is better to add additional information at the end of each line to avoid
> > potential breakage of User space SW (test/debug scripts).
>
> What? If I wanted a stable interface I would use sysfs and document
> the ABI in Documentation/ABI/*.
>
> debugfs is not ABI.
As I mentioned in my response to Grygorii, not everyone -- and most
notably apparently not even Linus Torvalds -- agrees on this:
"The fact that something is documented (whether correctly or
not) has absolutely _zero_ impact on anything at all. What makes
something an ABI is that it's useful and available. The only way
something isn't an ABI is by _explicitly_ making sure that it's
not available even by mistake in a stable form for binary use.
Example: kernel internal data structures and function calls. We
make sure that you simply _cannot_ make a binary that works
across kernel versions. That is the only way for an ABI to not
form."
https://lwn.net/Articles/309298/
In this case, it could be worked around by providing another debugfs
file with gpios used as IRQs, I guess.
Johan
On Tue, May 19, 2015 at 5:39 PM, Johan Hovold <[email protected]> wrote:
> On Tue, May 19, 2015 at 04:12:35PM +0200, Linus Walleij wrote:
>> What? If I wanted a stable interface I would use sysfs and document
>> the ABI in Documentation/ABI/*.
>>
>> debugfs is not ABI.
>
> As I mentioned in my response to Grygorii, not everyone -- and most
> notably apparently not even Linus Torvalds -- agrees on this:
Yeah I was sloppy I guess.
What I mean, precisely is that sysfs is ABI, whether documented or
not.
Even debugfs is actually blurry, as per
Documentation/filesystems/debugfs.txt:
"The debugfs filesystem is also intended to not serve as a stable
ABI to user space; in theory, there are no stability constraints placed on
files exported there. The real world is not always so simple, though [1];
even debugfs interfaces are best designed with the idea that they will need
to be maintained forever."
But I haven't been bitten by it yet so that's why I allow some poetic
license.
Yours,
Linus Walleij
On Tue, May 19, 2015 at 04:28:55PM +0200, Linus Walleij wrote:
> On Mon, May 18, 2015 at 1:02 PM, Johan Hovold <[email protected]> wrote:
> > On Fri, May 15, 2015 at 04:25:21PM +0300, [email protected] wrote:
>
> >> GPIOs 192-223, platform/48051000.gpio, gpio:
> >> gpio-203 (vtt_fixed ) out hi requested
> >
> > This is backwards. All gpios *should* be requested. *If* we are to
> > include not-requested gpios in the debug output, then it is those pins
> > that need to be marked as not-requested.
>
> It depends, really. As concluded in earlier discussions when we
> introduced gpiochip_[un]lock_as_irq() the gpiolib and irqchip APIs
> are essentially orthogonal.
[...]
> So to atleast try to safeguard from a scenario such as
>
> - Client A requests IRQ from the irqchip side of the API
> and sets up a level active-low IRQ on it
>
> - Client B request the same line as GPIO
>
> - Client B sets it to output and drivers it low.
>
> - Client A crashes in an infinite IRQ loop as that line
> is not hammered low and will generate IRQs until
> the end of time.
>
> I introduced the gpiochip_[un]lock_as_irq() calls so we
> could safeguard against this. Notably that blocks client A
> from setting the line as output. I hope.
A problem with the current implementation is that it uses as a flag
rather than a refcount. As I pointed out elsewhere in this thread, it is
possible to request a shared IRQ (e.g. via the sysfs interface) and
release it, thereby making it possible to change the direction of the
pin while still in use for irq.
> I thought this would mean the line would only be used as IRQ
> in this case, so I could block any gpiod_get() calls to that
> line but *of course* some driver is using the IRQ and snooping
> into the GPIO value at the same time. So can't simplify things
> like so either.
>
> Maybe I'm smashing open doors here...
No, I understand that use case. But there are some issues with how it's
currently implemented. Besides the example above, nothing pins a gpio
chip driver in memory unless it has requested gpios, specifically,
requesting a pin for irq use is not enough.
> Anyway to get back to the original statement:
>
> > This is backwards. All gpios *should* be requested. *If* we are to
> > include not-requested gpios in the debug output, then it is those pins
> > that need to be marked as not-requested.
>
> This is correct, all GPIOs accessed *as gpios* should be
> requested, no matter if they are then cast to IRQs by gpiod_to_irq
> or not. However if the same hardware is used as only "some IRQ"
> through it's irqchip interface, it needs not be requested, but
> that is by definition not a GPIO, it is an IRQ.
True. And since it is not a GPIO, should it show up in
/sys/kernel/debug/gpio? ;)
Johan
On Wed, May 20, 2015 at 09:21:16AM +0200, Linus Walleij wrote:
> On Tue, May 19, 2015 at 5:39 PM, Johan Hovold <[email protected]> wrote:
> > On Tue, May 19, 2015 at 04:12:35PM +0200, Linus Walleij wrote:
>
> >> What? If I wanted a stable interface I would use sysfs and document
> >> the ABI in Documentation/ABI/*.
> >>
> >> debugfs is not ABI.
> >
> > As I mentioned in my response to Grygorii, not everyone -- and most
> > notably apparently not even Linus Torvalds -- agrees on this:
>
> Yeah I was sloppy I guess.
>
> What I mean, precisely is that sysfs is ABI, whether documented or
> not.
>
> Even debugfs is actually blurry, as per
> Documentation/filesystems/debugfs.txt:
>
> "The debugfs filesystem is also intended to not serve as a stable
> ABI to user space; in theory, there are no stability constraints placed on
> files exported there. The real world is not always so simple, though [1];
> even debugfs interfaces are best designed with the idea that they will need
> to be maintained forever."
>
> But I haven't been bitten by it yet so that's why I allow some poetic
> license.
Yes, and the [1] reference in that quote is the LWN article I referred
to.
I also see you already "broke" that ABI in a similar way in 2013 with
d468bf9ecaab ("gpio: add API to be strict about GPIO IRQ usage") by
adding the IRQ field.
Your call. :)
Johan
On 05/21/2015 05:25 PM, Johan Hovold wrote:
> On Tue, May 19, 2015 at 04:28:55PM +0200, Linus Walleij wrote:
>> On Mon, May 18, 2015 at 1:02 PM, Johan Hovold <[email protected]> wrote:
>>> On Fri, May 15, 2015 at 04:25:21PM +0300, [email protected] wrote:
>>
>>>> GPIOs 192-223, platform/48051000.gpio, gpio:
>>>> gpio-203 (vtt_fixed ) out hi requested
>>>
>>> This is backwards. All gpios *should* be requested. *If* we are to
>>> include not-requested gpios in the debug output, then it is those pins
>>> that need to be marked as not-requested.
>>
>> It depends, really. As concluded in earlier discussions when we
>> introduced gpiochip_[un]lock_as_irq() the gpiolib and irqchip APIs
>> are essentially orthogonal.
>
> [...]
>
>> So to atleast try to safeguard from a scenario such as
>>
>> - Client A requests IRQ from the irqchip side of the API
>> and sets up a level active-low IRQ on it
>>
>> - Client B request the same line as GPIO
>>
>> - Client B sets it to output and drivers it low.
>>
>> - Client A crashes in an infinite IRQ loop as that line
>> is not hammered low and will generate IRQs until
>> the end of time.
>>
>> I introduced the gpiochip_[un]lock_as_irq() calls so we
>> could safeguard against this. Notably that blocks client A
>> from setting the line as output. I hope.
>
> A problem with the current implementation is that it uses as a flag
> rather than a refcount. As I pointed out elsewhere in this thread, it is
> possible to request a shared IRQ (e.g. via the sysfs interface) and
> release it, thereby making it possible to change the direction of the
> pin while still in use for irq.
Yes (checked). And this is an issue which need to be fixed.
- gpio sysfs should not call gpiochip_un/lock_as_irq()
- gpio drivers should use gpiochip API or implement
.irq_release/request_resources() callbacks
in this case case IRQ core will do just what is required. Right?
>
>> I thought this would mean the line would only be used as IRQ
>> in this case, so I could block any gpiod_get() calls to that
>> line but *of course* some driver is using the IRQ and snooping
>> into the GPIO value at the same time. So can't simplify things
>> like so either.
>>
>> Maybe I'm smashing open doors here...
>
> No, I understand that use case. But there are some issues with how it's
> currently implemented. Besides the example above, nothing pins a gpio
> chip driver in memory unless it has requested gpios, specifically,
> requesting a pin for irq use is not enough.
ok. An issue. Possible fix below:
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index ea11706..64392ad 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -514,6 +514,9 @@ static int gpiochip_irq_reqres(struct irq_data *d)
{
struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+ if (!try_module_get(chip->owner))
+ return -ENODEV;
+
if (gpiochip_lock_as_irq(chip, d->hwirq)) {
chip_err(chip,
"unable to lock HW IRQ %lu for IRQ\n",
@@ -528,6 +531,7 @@ static void gpiochip_irq_relres(struct irq_data *d)
struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
gpiochip_unlock_as_irq(chip, d->hwirq);
+ module_put(chip->owner);
}
>
>> Anyway to get back to the original statement:
>>
>>> This is backwards. All gpios *should* be requested. *If* we are to
>>> include not-requested gpios in the debug output, then it is those pins
>>> that need to be marked as not-requested.
>>
>> This is correct, all GPIOs accessed *as gpios* should be
>> requested, no matter if they are then cast to IRQs by gpiod_to_irq
>> or not. However if the same hardware is used as only "some IRQ"
>> through it's irqchip interface, it needs not be requested, but
>> that is by definition not a GPIO, it is an IRQ.
>
> True. And since it is not a GPIO, should it show up in
> /sys/kernel/debug/gpio? ;)
"Nice" idea :)
This information needed for debugging and testing which includes
checking of pin state (hi/lo) - especially useful during board's
bring-up when a lot of mistakes are detected related to wrong usage
of IRQ/GPIO numbers.
--
regards,
-grygorii
On Thu, May 21, 2015 at 11:33:01PM +0300, [email protected] wrote:
> On 05/21/2015 05:25 PM, Johan Hovold wrote:
> > On Tue, May 19, 2015 at 04:28:55PM +0200, Linus Walleij wrote:
> >> I introduced the gpiochip_[un]lock_as_irq() calls so we
> >> could safeguard against this. Notably that blocks client A
> >> from setting the line as output. I hope.
> >
> > A problem with the current implementation is that it uses as a flag
> > rather than a refcount. As I pointed out elsewhere in this thread, it is
> > possible to request a shared IRQ (e.g. via the sysfs interface) and
> > release it, thereby making it possible to change the direction of the
> > pin while still in use for irq.
>
> Yes (checked). And this is an issue which need to be fixed.
> - gpio sysfs should not call gpiochip_un/lock_as_irq()
This is a known but unrelated issue. The lock/unlock call in the sysfs
implementation is at worst redundant. I suggested removing it earlier,
but Linus pointed out that there were still a few drivers not
implementing the irq resource callbacks that need to be updated first.
> - gpio drivers should use gpiochip API or implement
> .irq_release/request_resources() callbacks
>
> in this case case IRQ core will do just what is required. Right?
No, the problem is that the "lock" is implemented using a flag rather
than a refcount.
> >> I thought this would mean the line would only be used as IRQ
> >> in this case, so I could block any gpiod_get() calls to that
> >> line but *of course* some driver is using the IRQ and snooping
> >> into the GPIO value at the same time. So can't simplify things
> >> like so either.
> >>
> >> Maybe I'm smashing open doors here...
> >
> > No, I understand that use case. But there are some issues with how it's
> > currently implemented. Besides the example above, nothing pins a gpio
> > chip driver in memory unless it has requested gpios, specifically,
> > requesting a pin for irq use is not enough.
>
> ok. An issue. Possible fix below:
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index ea11706..64392ad 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -514,6 +514,9 @@ static int gpiochip_irq_reqres(struct irq_data *d)
> {
> struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>
> + if (!try_module_get(chip->owner))
> + return -ENODEV;
> +
> if (gpiochip_lock_as_irq(chip, d->hwirq)) {
> chip_err(chip,
> "unable to lock HW IRQ %lu for IRQ\n",
> @@ -528,6 +531,7 @@ static void gpiochip_irq_relres(struct irq_data *d)
> struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>
> gpiochip_unlock_as_irq(chip, d->hwirq);
> + module_put(chip->owner);
> }
The resource callbacks would be the place to do resource allocation, but
the above snippet is obviously broken as its leaking resources in the
error path.
> >> Anyway to get back to the original statement:
> >>
> >>> This is backwards. All gpios *should* be requested. *If* we are to
> >>> include not-requested gpios in the debug output, then it is those pins
> >>> that need to be marked as not-requested.
> >>
> >> This is correct, all GPIOs accessed *as gpios* should be
> >> requested, no matter if they are then cast to IRQs by gpiod_to_irq
> >> or not. However if the same hardware is used as only "some IRQ"
> >> through it's irqchip interface, it needs not be requested, but
> >> that is by definition not a GPIO, it is an IRQ.
> >
> > True. And since it is not a GPIO, should it show up in
> > /sys/kernel/debug/gpio? ;)
>
> "Nice" idea :)
> This information needed for debugging and testing which includes
> checking of pin state (hi/lo) - especially useful during board's
> bring-up when a lot of mistakes are detected related to wrong usage
> of IRQ/GPIO numbers.
At least the gpio-irq mapping for requested gpios could be useful.
Another issue here is that you would start calling gpio accessors for
unrequested gpios. Are you sure all gpio drivers can, and will always be
able to, handle that? [ When using the gpiod interface, gpios will always
be requested and must not be accessed after having been released. ]
Johan
On 05/24/2015 08:12 PM, Johan Hovold wrote:
> On Thu, May 21, 2015 at 11:33:01PM +0300, [email protected] wrote:
>> On 05/21/2015 05:25 PM, Johan Hovold wrote:
>>> On Tue, May 19, 2015 at 04:28:55PM +0200, Linus Walleij wrote:
>
>>>> I introduced the gpiochip_[un]lock_as_irq() calls so we
>>>> could safeguard against this. Notably that blocks client A
>>>> from setting the line as output. I hope.
>>>
>>> A problem with the current implementation is that it uses as a flag
>>> rather than a refcount. As I pointed out elsewhere in this thread, it is
>>> possible to request a shared IRQ (e.g. via the sysfs interface) and
>>> release it, thereby making it possible to change the direction of the
>>> pin while still in use for irq.
>>
>> Yes (checked). And this is an issue which need to be fixed.
>> - gpio sysfs should not call gpiochip_un/lock_as_irq()
>
> This is a known but unrelated issue. The lock/unlock call in the sysfs
> implementation is at worst redundant. I suggested removing it earlier,
> but Linus pointed out that there were still a few drivers not
> implementing the irq resource callbacks that need to be updated first.
>
>> - gpio drivers should use gpiochip API or implement
>> .irq_release/request_resources() callbacks
>>
>> in this case case IRQ core will do just what is required. Right?
>
> No, the problem is that the "lock" is implemented using a flag rather
> than a refcount.
I've rechecked __setup_irq() and what I can see from it is that
irq_request_resources(), __irq_set_trigger() and irq_startup()
functions will be called only when the first IRQ action is installed.
So, It looks like flag should work here. Am I missing smth?
>
>>>> I thought this would mean the line would only be used as IRQ
>>>> in this case, so I could block any gpiod_get() calls to that
>>>> line but *of course* some driver is using the IRQ and snooping
>>>> into the GPIO value at the same time. So can't simplify things
>>>> like so either.
>>>>
>>>> Maybe I'm smashing open doors here...
>>>
>>> No, I understand that use case. But there are some issues with how it's
>>> currently implemented. Besides the example above, nothing pins a gpio
>>> chip driver in memory unless it has requested gpios, specifically,
>>> requesting a pin for irq use is not enough.
>>
>> ok. An issue. Possible fix below:
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index ea11706..64392ad 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -514,6 +514,9 @@ static int gpiochip_irq_reqres(struct irq_data *d)
>> {
>> struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>>
>> + if (!try_module_get(chip->owner))
>> + return -ENODEV;
>> +
>> if (gpiochip_lock_as_irq(chip, d->hwirq)) {
>> chip_err(chip,
>> "unable to lock HW IRQ %lu for IRQ\n",
>> @@ -528,6 +531,7 @@ static void gpiochip_irq_relres(struct irq_data *d)
>> struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>>
>> gpiochip_unlock_as_irq(chip, d->hwirq);
>> + module_put(chip->owner);
>> }
>
> The resource callbacks would be the place to do resource allocation, but
> the above snippet is obviously broken as its leaking resources in the
> error path.
True, Thanks. This was the very fast try to solve issue. It could be converted
to the patch if GPIO maintainers agree with this approach.
>
>>>> Anyway to get back to the original statement:
>>>>
>>>>> This is backwards. All gpios *should* be requested. *If* we are to
>>>>> include not-requested gpios in the debug output, then it is those pins
>>>>> that need to be marked as not-requested.
>>>>
>>>> This is correct, all GPIOs accessed *as gpios* should be
>>>> requested, no matter if they are then cast to IRQs by gpiod_to_irq
>>>> or not. However if the same hardware is used as only "some IRQ"
>>>> through it's irqchip interface, it needs not be requested, but
>>>> that is by definition not a GPIO, it is an IRQ.
>>>
>>> True. And since it is not a GPIO, should it show up in
>>> /sys/kernel/debug/gpio? ;)
>>
>> "Nice" idea :)
>> This information needed for debugging and testing which includes
>> checking of pin state (hi/lo) - especially useful during board's
>> bring-up when a lot of mistakes are detected related to wrong usage
>> of IRQ/GPIO numbers.
>
> At least the gpio-irq mapping for requested gpios could be useful.
>
> Another issue here is that you would start calling gpio accessors for
> unrequested gpios. Are you sure all gpio drivers can, and will always be
> able to, handle that? [ When using the gpiod interface, gpios will always
> be requested and must not be accessed after having been released. ]
Agree :(. I'm not sure. This is very sensible remark:
- call of gpiod_get_direction() can be avoided, in general, for <irq-only> case
- but gpiod_to_irq() -- is not.
.. Looks like it's time to drop this stuff :,,(
Thanks all.
--
regards,
-grygorii
On Mon, May 25, 2015 at 09:54:29PM +0300, [email protected] wrote:
> On 05/24/2015 08:12 PM, Johan Hovold wrote:
> > On Thu, May 21, 2015 at 11:33:01PM +0300, [email protected] wrote:
> >> On 05/21/2015 05:25 PM, Johan Hovold wrote:
> >>> A problem with the current implementation is that it uses as a flag
> >>> rather than a refcount. As I pointed out elsewhere in this thread, it is
> >>> possible to request a shared IRQ (e.g. via the sysfs interface) and
> >>> release it, thereby making it possible to change the direction of the
> >>> pin while still in use for irq.
> >>
> >> Yes (checked). And this is an issue which need to be fixed.
> >> - gpio sysfs should not call gpiochip_un/lock_as_irq()
> >
> > This is a known but unrelated issue. The lock/unlock call in the sysfs
> > implementation is at worst redundant. I suggested removing it earlier,
> > but Linus pointed out that there were still a few drivers not
> > implementing the irq resource callbacks that need to be updated first.
> >
> >> - gpio drivers should use gpiochip API or implement
> >> .irq_release/request_resources() callbacks
> >>
> >> in this case case IRQ core will do just what is required. Right?
> >
> > No, the problem is that the "lock" is implemented using a flag rather
> > than a refcount.
>
> I've rechecked __setup_irq() and what I can see from it is that
> irq_request_resources(), __irq_set_trigger() and irq_startup()
> functions will be called only when the first IRQ action is installed.
> So, It looks like flag should work here. Am I missing smth?
You're absolutely right. I didn't look at the code after I managed to
trigger the bug using sysfs and my memory failed me. Bah, sorry about
that.
It is indeed a sysfs-interface bug.
> > At least the gpio-irq mapping for requested gpios could be useful.
> >
> > Another issue here is that you would start calling gpio accessors for
> > unrequested gpios. Are you sure all gpio drivers can, and will always be
> > able to, handle that? [ When using the gpiod interface, gpios will always
> > be requested and must not be accessed after having been released. ]
>
> Agree :(. I'm not sure. This is very sensible remark:
> - call of gpiod_get_direction() can be avoided, in general, for <irq-only> case
> - but gpiod_to_irq() -- is not.
>
> .. Looks like it's time to drop this stuff :,,(
You can always export the pins using sysfs or request them using the new
hogging mechanism from DT so that the pins you're interested in
debugging show up in debugfs.
Thanks,
Johan
On Mon, May 25, 2015 at 8:54 PM, [email protected]
<[email protected]> wrote:
> .. Looks like it's time to drop this stuff :,,(
Ooops missed this part of the discussion. Indeed it will call accessors
on non-requested GPIO lines. Damned. Taking these patches out again.
Yours,
Linus Walleij
Hi Linus,
On 06/01/2015 04:09 PM, Linus Walleij wrote:
> On Mon, May 25, 2015 at 8:54 PM, [email protected]
> <[email protected]> wrote:
>
>> .. Looks like it's time to drop this stuff :,,(
>
> Ooops missed this part of the discussion. Indeed it will call accessors
> on non-requested GPIO lines. Damned. Taking these patches out again.
Yep. I've missed to recall v2 patches, sorry for that.
--
regards,
-grygorii