diff mbox

[9/9] gpiolib: Add gpio name information to /sys/kernel/debug/gpio

Message ID 1437125570-28623-10-git-send-email-mpa@pengutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Markus Pargmann July 17, 2015, 9:32 a.m. UTC
Add some information about gpio names to the debugfs gpio file. name and
label of a GPIO are then displayed next to each other. This way it is
easy to see what the real name of GPIO is and what the driver requested
it for.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/gpio/gpiolib.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Johan Hovold July 28, 2015, 9:58 a.m. UTC | #1
On Fri, Jul 17, 2015 at 11:32:50AM +0200, Markus Pargmann wrote:
> Add some information about gpio names to the debugfs gpio file. name and
> label of a GPIO are then displayed next to each other. This way it is
> easy to see what the real name of GPIO is and what the driver requested
> it for.
> 
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  drivers/gpio/gpiolib.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index dcac3bcf21dd..0f1d1f5faf5d 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2308,14 +2308,19 @@ 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)) {
> +			if (gdesc->name) {
> +				seq_printf(s, " gpio-%-3d (%-20.20s)\n",
> +					   gpio, gdesc->name);
> +			}

So now we'd no longer just be listing requested gpios, but on a similar
format to how requested ones used to be represented.

Then there's the debugfs as ABI discussion...

Johan
Markus Pargmann July 29, 2015, 7:08 a.m. UTC | #2
On Tue, Jul 28, 2015 at 11:58:42AM +0200, Johan Hovold wrote:
> On Fri, Jul 17, 2015 at 11:32:50AM +0200, Markus Pargmann wrote:
> > Add some information about gpio names to the debugfs gpio file. name and
> > label of a GPIO are then displayed next to each other. This way it is
> > easy to see what the real name of GPIO is and what the driver requested
> > it for.
> > 
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > ---
> >  drivers/gpio/gpiolib.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index dcac3bcf21dd..0f1d1f5faf5d 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -2308,14 +2308,19 @@ 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)) {
> > +			if (gdesc->name) {
> > +				seq_printf(s, " gpio-%-3d (%-20.20s)\n",
> > +					   gpio, gdesc->name);
> > +			}
> 
> So now we'd no longer just be listing requested gpios, but on a similar
> format to how requested ones used to be represented.

Better suggestions on how to display those extra information in debugfs?

> 
> Then there's the debugfs as ABI discussion...

I didn't consider debugfs as ABI as I thought it is just for debugging purposes?

Best regards,

Markus
Johan Hovold July 31, 2015, 8:54 a.m. UTC | #3
On Wed, Jul 29, 2015 at 09:08:42AM +0200, Markus Pargmann wrote:
> On Tue, Jul 28, 2015 at 11:58:42AM +0200, Johan Hovold wrote:
> > On Fri, Jul 17, 2015 at 11:32:50AM +0200, Markus Pargmann wrote:
> > > Add some information about gpio names to the debugfs gpio file. name and
> > > label of a GPIO are then displayed next to each other. This way it is
> > > easy to see what the real name of GPIO is and what the driver requested
> > > it for.
> > > 
> > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > > ---
> > >  drivers/gpio/gpiolib.c | 11 ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index dcac3bcf21dd..0f1d1f5faf5d 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -2308,14 +2308,19 @@ 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)) {
> > > +			if (gdesc->name) {
> > > +				seq_printf(s, " gpio-%-3d (%-20.20s)\n",
> > > +					   gpio, gdesc->name);
> > > +			}
> > 
> > So now we'd no longer just be listing requested gpios, but on a similar
> > format to how requested ones used to be represented.
> 
> Better suggestions on how to display those extra information in debugfs?

Perhaps a new file only listing the line names.

> > Then there's the debugfs as ABI discussion...
> 
> I didn't consider debugfs as ABI as I thought it is just for debugging
> purposes?

Some people seem to have expressed a different position:

	"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/

Johan
Markus Pargmann July 31, 2015, 10:41 a.m. UTC | #4
On Fri, Jul 31, 2015 at 10:54:07AM +0200, Johan Hovold wrote:
> On Wed, Jul 29, 2015 at 09:08:42AM +0200, Markus Pargmann wrote:
> > On Tue, Jul 28, 2015 at 11:58:42AM +0200, Johan Hovold wrote:
> > > On Fri, Jul 17, 2015 at 11:32:50AM +0200, Markus Pargmann wrote:
> > > > Add some information about gpio names to the debugfs gpio file. name and
> > > > label of a GPIO are then displayed next to each other. This way it is
> > > > easy to see what the real name of GPIO is and what the driver requested
> > > > it for.
> > > > 
> > > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > > > ---
> > > >  drivers/gpio/gpiolib.c | 11 ++++++++---
> > > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > > index dcac3bcf21dd..0f1d1f5faf5d 100644
> > > > --- a/drivers/gpio/gpiolib.c
> > > > +++ b/drivers/gpio/gpiolib.c
> > > > @@ -2308,14 +2308,19 @@ 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)) {
> > > > +			if (gdesc->name) {
> > > > +				seq_printf(s, " gpio-%-3d (%-20.20s)\n",
> > > > +					   gpio, gdesc->name);
> > > > +			}
> > > 
> > > So now we'd no longer just be listing requested gpios, but on a similar
> > > format to how requested ones used to be represented.
> > 
> > Better suggestions on how to display those extra information in debugfs?
> 
> Perhaps a new file only listing the line names.

Yes that's a possibility. On the other side having it all in one place
is nice. As it helps you to debug faster, you can directly see which
line name was requested for what purpose and so on.

> 
> > > Then there's the debugfs as ABI discussion...
> > 
> > I didn't consider debugfs as ABI as I thought it is just for debugging
> > purposes?
> 
> Some people seem to have expressed a different position:
> 
> 	"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/
> 

Oh, I see. But I think that debugfs should help us to debug issues.
If we start to care about breaking userspace tools it is not as helpful
anymore. If I develop some tool against information that are
clearly marked for debugging, I would somehow expect that my tool breaks
at some point. But thats just my opinion.

Best regards,

Markus
Johan Hovold July 31, 2015, 10:45 a.m. UTC | #5
On Fri, Jul 31, 2015 at 12:41:01PM +0200, Markus Pargmann wrote:
> On Fri, Jul 31, 2015 at 10:54:07AM +0200, Johan Hovold wrote:
> > On Wed, Jul 29, 2015 at 09:08:42AM +0200, Markus Pargmann wrote:
> > > On Tue, Jul 28, 2015 at 11:58:42AM +0200, Johan Hovold wrote:
> > > > On Fri, Jul 17, 2015 at 11:32:50AM +0200, Markus Pargmann wrote:
> > > > > Add some information about gpio names to the debugfs gpio file. name and
> > > > > label of a GPIO are then displayed next to each other. This way it is
> > > > > easy to see what the real name of GPIO is and what the driver requested
> > > > > it for.
> > > > > 
> > > > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > > > > ---
> > > > >  drivers/gpio/gpiolib.c | 11 ++++++++---
> > > > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > > > index dcac3bcf21dd..0f1d1f5faf5d 100644
> > > > > --- a/drivers/gpio/gpiolib.c
> > > > > +++ b/drivers/gpio/gpiolib.c
> > > > > @@ -2308,14 +2308,19 @@ 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)) {
> > > > > +			if (gdesc->name) {
> > > > > +				seq_printf(s, " gpio-%-3d (%-20.20s)\n",
> > > > > +					   gpio, gdesc->name);
> > > > > +			}
> > > > 
> > > > So now we'd no longer just be listing requested gpios, but on a similar
> > > > format to how requested ones used to be represented.
> > > 
> > > Better suggestions on how to display those extra information in debugfs?
> > 
> > Perhaps a new file only listing the line names.
> 
> Yes that's a possibility. On the other side having it all in one place
> is nice. As it helps you to debug faster, you can directly see which
> line name was requested for what purpose and so on.

Certainly, but you'd change the meaning of debug/gpio which used to
contain only requested gpios.

Not saying we shouldn't, but yeah, let's were this discussion goes
first.

> > > > Then there's the debugfs as ABI discussion...
> > > 
> > > I didn't consider debugfs as ABI as I thought it is just for debugging
> > > purposes?
> > 
> > Some people seem to have expressed a different position:
> > 
> > 	"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/
> > 
> 
> Oh, I see. But I think that debugfs should help us to debug issues.
> If we start to care about breaking userspace tools it is not as helpful
> anymore. If I develop some tool against information that are
> clearly marked for debugging, I would somehow expect that my tool breaks
> at some point. But thats just my opinion.

I tend to agree, and wish debugfs was never to be considered ABI, but
it's Torvald's words above...

Johan
Lucas Stach July 31, 2015, 10:49 a.m. UTC | #6
Am Freitag, den 31.07.2015, 10:54 +0200 schrieb Johan Hovold:
> On Wed, Jul 29, 2015 at 09:08:42AM +0200, Markus Pargmann wrote:
> > On Tue, Jul 28, 2015 at 11:58:42AM +0200, Johan Hovold wrote:
> > > On Fri, Jul 17, 2015 at 11:32:50AM +0200, Markus Pargmann wrote:
> > > > Add some information about gpio names to the debugfs gpio file. name and
> > > > label of a GPIO are then displayed next to each other. This way it is
> > > > easy to see what the real name of GPIO is and what the driver requested
> > > > it for.
> > > > 
> > > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > > > ---
> > > >  drivers/gpio/gpiolib.c | 11 ++++++++---
> > > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > > index dcac3bcf21dd..0f1d1f5faf5d 100644
> > > > --- a/drivers/gpio/gpiolib.c
> > > > +++ b/drivers/gpio/gpiolib.c
> > > > @@ -2308,14 +2308,19 @@ 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)) {
> > > > +			if (gdesc->name) {
> > > > +				seq_printf(s, " gpio-%-3d (%-20.20s)\n",
> > > > +					   gpio, gdesc->name);
> > > > +			}
> > > 
> > > So now we'd no longer just be listing requested gpios, but on a similar
> > > format to how requested ones used to be represented.
> > 
> > Better suggestions on how to display those extra information in debugfs?
> 
> Perhaps a new file only listing the line names.
> 
> > > Then there's the debugfs as ABI discussion...
> > 
> > I didn't consider debugfs as ABI as I thought it is just for debugging
> > purposes?
> 
> Some people seem to have expressed a different position:
> 
> 	"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/

Assuming debugfs is ABI is completely backwards. debugfs makes no ABI
guarantees, that's why stuff that needs a stable ABI like tracing is
actively moving out of debugfs into other places, where ABI stability
can be assumed.

Regards,
Lucas
diff mbox

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index dcac3bcf21dd..0f1d1f5faf5d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2308,14 +2308,19 @@  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)) {
+			if (gdesc->name) {
+				seq_printf(s, " gpio-%-3d (%-20.20s)\n",
+					   gpio, gdesc->name);
+			}
 			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",
-			gpio, gdesc->label,
+		seq_printf(s, " gpio-%-3d (%-20.20s|%-20.20s) %s %s %s",
+			gpio, gdesc->name ? gdesc->name : "", gdesc->label,
 			is_out ? "out" : "in ",
 			chip->get
 				? (chip->get(chip, i) ? "hi" : "lo")