| Message ID | 1431696321-7257-1-git-send-email-grygorii.strashko@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Johan, On 05/18/2015 02:02 PM, Johan Hovold wrote: > On Fri, May 15, 2015 at 04:25:21PM +0300, grygorii.strashko@linaro.org wrote: >> From: Grygorii Strashko <grygorii.strashko@linaro.org> >> >> 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 )
On 05/18/2015 06:08 PM, Johan Hovold wrote: > On Mon, May 18, 2015 at 04:06:08PM +0300, Grygorii.Strashko@linaro.org wrote: >> On 05/18/2015 02:02 PM, Johan Hovold wrote: >>> On Fri, May 15, 2015 at 04:25:21PM +0300, grygorii.strashko@linaro.org wrote: >>>> From: Grygorii Strashko <grygorii.strashko@linaro.org> >>>> >>>> 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.
On Mon, May 18, 2015 at 5:17 PM, Grygorii.Strashko@linaro.org <grygorii.strashko@linaro.org> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Mon, May 18, 2015 at 1:02 PM, Johan Hovold <johan@kernel.org> wrote: > On Fri, May 15, 2015 at 04:25:21PM +0300, grygorii.strashko@linaro.org 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Hi Linus, On 05/19/2015 05:12 PM, Linus Walleij wrote: > On Mon, May 18, 2015 at 5:17 PM, Grygorii.Strashko@linaro.org > <grygorii.strashko@linaro.org> 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.
On Tue, May 19, 2015 at 4:37 PM, Grygorii.Strashko@linaro.org <grygorii.strashko@linaro.org> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 19, 2015 at 5:39 PM, Johan Hovold <johan@kernel.org> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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"); } }