[PATCH 0/2] Annotate gpio-configuration with __must_check

[PATCH 0/2] Annotate gpio-configuration with __must_check

[Wolfram Sang]

Here is a small series generating a lot of warnings, especially in board
bringup-files. Still, I think it is worthwhile to be strict about checking
return values of gpio-configuration-functions. My suggestion to keep the noise
a bit lower is to put it into linux-next for one cycle and then merge it for
2.6.39? That should give people some time to fix the issues in time. Looking
forward to comments.

Wolfram Sang (2):
  gpiolib: annotate gpio-intialization with __must_check
  gpiolib: add missing functions to generic fallback

 Documentation/gpio.txt     |    2 +-
 include/asm-generic/gpio.h |   10 +++++-----
 include/linux/gpio.h       |   26 +++++++++++++++++++++++---
 3 files changed, 29 insertions(+), 9 deletions(-)

-- 
1.7.2.3

[PATCH 1/2] gpiolib: annotate gpio-intialization with __must_check

[Wolfram Sang]

Because GPIOs can have crucial functions especially in embedded systems,
we are better safe than sorry regarding their configuration. For
gpio_request, the documentation is simply enforced: <quote>"The return
value of gpio_request() must be checked."</quote> For gpio_direction_*
and gpio_request_*, we now act accordingly.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Greg KH <gregkh@suse.de>
---
 Documentation/gpio.txt     |    2 +-
 include/asm-generic/gpio.h |   10 +++++-----
 include/linux/gpio.h       |    6 +++---
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/Documentation/gpio.txt b/Documentation/gpio.txt
index 792faa3..a492d92 100644
--- a/Documentation/gpio.txt
+++ b/Documentation/gpio.txt
@@ -135,7 +135,7 @@ setting up a platform_device using the GPIO, is mark its direction:
 	int gpio_direction_input(unsigned gpio);
 	int gpio_direction_output(unsigned gpio, int value);
 
-The return value is zero for success, else a negative errno.  It should
+The return value is zero for success, else a negative errno.  It must
 be checked, since the get/set calls don't have error returns and since
 misconfiguration is possible.  You should normally issue these calls from
 a task context.  However, for spinlock-safe GPIOs it's OK to use them
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index ff5c660..6098cae 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -147,11 +147,11 @@ extern struct gpio_chip *gpiochip_find(void *data,
 /* Always use the library code for GPIO management calls,
  * or when sleeping may be involved.
  */
-extern int gpio_request(unsigned gpio, const char *label);
+extern int __must_check gpio_request(unsigned gpio, const char *label);
 extern void gpio_free(unsigned gpio);
 
-extern int gpio_direction_input(unsigned gpio);
-extern int gpio_direction_output(unsigned gpio, int value);
+extern int __must_check gpio_direction_input(unsigned gpio);
+extern int __must_check gpio_direction_output(unsigned gpio, int value);
 
 extern int gpio_set_debounce(unsigned gpio, unsigned debounce);
 
@@ -192,8 +192,8 @@ struct gpio {
 	const char	*label;
 };
 
-extern int gpio_request_one(unsigned gpio, unsigned long flags, const char *label);
-extern int gpio_request_array(struct gpio *array, size_t num);
+extern int __must_check gpio_request_one(unsigned gpio, unsigned long flags, const char *label);
+extern int __must_check gpio_request_array(struct gpio *array, size_t num);
 extern void gpio_free_array(struct gpio *array, size_t num);
 
 #ifdef CONFIG_GPIO_SYSFS
diff --git a/include/linux/gpio.h b/include/linux/gpio.h
index e41f7dd..1d5214a 100644
--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -29,7 +29,7 @@ static inline int gpio_is_valid(int number)
 	return 0;
 }
 
-static inline int gpio_request(unsigned gpio, const char *label)
+static inline int __must_check gpio_request(unsigned gpio, const char *label)
 {
 	return -ENOSYS;
 }
@@ -42,12 +42,12 @@ static inline void gpio_free(unsigned gpio)
 	WARN_ON(1);
 }
 
-static inline int gpio_direction_input(unsigned gpio)
+static inline int __must_check gpio_direction_input(unsigned gpio)
 {
 	return -ENOSYS;
 }
 
-static inline int gpio_direction_output(unsigned gpio, int value)
+static inline int __must_check gpio_direction_output(unsigned gpio, int value)
 {
 	return -ENOSYS;
 }
-- 
1.7.2.3

[PATCH 2/2] gpiolib: add missing functions to generic fallback

[Wolfram Sang]

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Greg KH <gregkh@suse.de>
---
 include/linux/gpio.h |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/include/linux/gpio.h b/include/linux/gpio.h
index 1d5214a..f79d67f 100644
--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -13,6 +13,7 @@
 #include <linux/errno.h>
 
 struct device;
+struct gpio;
 struct gpio_chip;
 
 /*
@@ -34,6 +35,17 @@ static inline int __must_check gpio_request(unsigned gpio, const char *label)
 	return -ENOSYS;
 }
 
+static inline int __must_check gpio_request_one(unsigned gpio,
+					unsigned long flags, const char *label)
+{
+	return -ENOSYS;
+}
+
+static inline int __must_check gpio_request_array(struct gpio *array, size_t num)
+{
+	return -ENOSYS;
+}
+
 static inline void gpio_free(unsigned gpio)
 {
 	might_sleep();
@@ -42,6 +54,14 @@ static inline void gpio_free(unsigned gpio)
 	WARN_ON(1);
 }
 
+static inline void gpio_free_array(struct gpio *array, size_t num)
+{
+	might_sleep();
+
+	/* GPIO can never have been requested */
+	WARN_ON(1);
+}
+
 static inline int __must_check gpio_direction_input(unsigned gpio)
 {
 	return -ENOSYS;
-- 
1.7.2.3

Re: [PATCH 0/2] Annotate gpio-configuration with __must_check

[Greg KH]

On Tue, Jan 04, 2011 at 05:51:06PM +0100, Wolfram Sang wrote:
> Here is a small series generating a lot of warnings, especially in board
> bringup-files. Still, I think it is worthwhile to be strict about checking
> return values of gpio-configuration-functions. My suggestion to keep the noise
> a bit lower is to put it into linux-next for one cycle and then merge it for
> 2.6.39? That should give people some time to fix the issues in time. Looking
> forward to comments.

It's ok to add this type of thing, but please, go through and fix the
warnings at the same time.  Otherwise it's a bit rude to force others to
fix their code for something that you did.

Care to also send those patches along?

thanks,

greg k-h

Re: [PATCH 0/2] Annotate gpio-configuration with __must_check

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1966 bytes --]

On Tue, Jan 04, 2011 at 12:27:18PM -0800, Greg KH wrote:
> On Tue, Jan 04, 2011 at 05:51:06PM +0100, Wolfram Sang wrote:
> > Here is a small series generating a lot of warnings, especially in board
> > bringup-files. Still, I think it is worthwhile to be strict about checking
> > return values of gpio-configuration-functions. My suggestion to keep the noise
> > a bit lower is to put it into linux-next for one cycle and then merge it for
> > 2.6.39? That should give people some time to fix the issues in time. Looking
> > forward to comments.
> 
> It's ok to add this type of thing, but please, go through and fix the
> warnings at the same time.  Otherwise it's a bit rude to force others to
> fix their code for something that you did.

Yeah, I understand. I was a "victim" of the patch causing all those "key not in
.data" messages back then. So, I actually did start a coccinelle-script fixing
the issues. I examined one sub-directory using a CPU/SoC I know relatively
well. I had to learn that even then, it is pretty hard to determine what
exactly to do if gpio_request() fails. For example, an unavailable GPIO being
the write-protect-pin for SD-cards might be simply ignored, maybe a warning
printed and the card will be rw by default. Another GPIO might be a chip-select
of a device I have never heard of before. It might be crucial and board_init
should fail if it cannot be requested. Or not. Things get worse for
architectures I never used before. This is why I think it is really better to
let people do the fixups who have/understand the hardware in question.
Otherwise the fixups could indeed be more harmful than helpful.

If this is still too rude for your taste, then what about a mechanism similar
to DEBUG_SECTION_MISMATCH?

Kind regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 0/2] Annotate gpio-configuration with __must_check

[Greg KH]

On Tue, Jan 04, 2011 at 10:29:16PM +0100, Wolfram Sang wrote:
> On Tue, Jan 04, 2011 at 12:27:18PM -0800, Greg KH wrote:
> > On Tue, Jan 04, 2011 at 05:51:06PM +0100, Wolfram Sang wrote:
> > > Here is a small series generating a lot of warnings, especially in board
> > > bringup-files. Still, I think it is worthwhile to be strict about checking
> > > return values of gpio-configuration-functions. My suggestion to keep the noise
> > > a bit lower is to put it into linux-next for one cycle and then merge it for
> > > 2.6.39? That should give people some time to fix the issues in time. Looking
> > > forward to comments.
> > 
> > It's ok to add this type of thing, but please, go through and fix the
> > warnings at the same time.  Otherwise it's a bit rude to force others to
> > fix their code for something that you did.
> 
> Yeah, I understand. I was a "victim" of the patch causing all those "key not in
> .data" messages back then. So, I actually did start a coccinelle-script fixing
> the issues. I examined one sub-directory using a CPU/SoC I know relatively
> well. I had to learn that even then, it is pretty hard to determine what
> exactly to do if gpio_request() fails. For example, an unavailable GPIO being
> the write-protect-pin for SD-cards might be simply ignored, maybe a warning
> printed and the card will be rw by default. Another GPIO might be a chip-select
> of a device I have never heard of before. It might be crucial and board_init
> should fail if it cannot be requested. Or not. Things get worse for
> architectures I never used before. This is why I think it is really better to
> let people do the fixups who have/understand the hardware in question.
> Otherwise the fixups could indeed be more harmful than helpful.

Sure, they could be more harmful, but at least try.  Make the patches
up, submit them to the maintainers, and if they are wrong, they will be
the best to fix it up properly.

> If this is still too rude for your taste, then what about a mechanism similar
> to DEBUG_SECTION_MISMATCH?

No, that's still annoying :)

thanks,

greg k-h

Re: [PATCH 0/2] Annotate gpio-configuration with __must_check

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 974 bytes --]


> > should fail if it cannot be requested. Or not. Things get worse for
> > architectures I never used before. This is why I think it is really better to
> > let people do the fixups who have/understand the hardware in question.
> > Otherwise the fixups could indeed be more harmful than helpful.
> 
> Sure, they could be more harmful, but at least try.  Make the patches
> up, submit them to the maintainers, and if they are wrong, they will be
> the best to fix it up properly.

Well, I could generalize all cases and always issue a WARN() if the request
fails. But this would just move a compile-time warning into a runtime warning.
Also, I have my doubts that even the arch/mach-maintainers know all the boards
and their peculiarities. There are thousands of them.

Kind regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 0/2] Annotate gpio-configuration with __must_check

[Greg KH]

On Wed, Jan 05, 2011 at 12:05:08AM +0100, Wolfram Sang wrote:
> 
> > > should fail if it cannot be requested. Or not. Things get worse for
> > > architectures I never used before. This is why I think it is really better to
> > > let people do the fixups who have/understand the hardware in question.
> > > Otherwise the fixups could indeed be more harmful than helpful.
> > 
> > Sure, they could be more harmful, but at least try.  Make the patches
> > up, submit them to the maintainers, and if they are wrong, they will be
> > the best to fix it up properly.
> 
> Well, I could generalize all cases and always issue a WARN() if the request
> fails. But this would just move a compile-time warning into a runtime warning.
> Also, I have my doubts that even the arch/mach-maintainers know all the boards
> and their peculiarities. There are thousands of them.

Well, they better know the peculiarities of the hardware they write code
for :)

But no, a WARN() might not be that nice as it usually is never seen by
embedded developers...

thanks,

greg k-h

Re: [PATCH 0/2] Annotate gpio-configuration with __must_check

[Mark Brown]

On Tue, Jan 04, 2011 at 03:44:40PM -0800, Greg KH wrote:

> But no, a WARN() might not be that nice as it usually is never seen by
> embedded developers...

They're probably about as relaible as build warning; spewing over the
serial console is pretty visible if yu're using one.

Re: [PATCH 0/2] Annotate gpio-configuration with __must_check

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1992 bytes --]

On Tue, Jan 04, 2011 at 03:44:40PM -0800, Greg KH wrote:
> On Wed, Jan 05, 2011 at 12:05:08AM +0100, Wolfram Sang wrote:
> > 
> > > > should fail if it cannot be requested. Or not. Things get worse for
> > > > architectures I never used before. This is why I think it is really better to
> > > > let people do the fixups who have/understand the hardware in question.
> > > > Otherwise the fixups could indeed be more harmful than helpful.
> > > 
> > > Sure, they could be more harmful, but at least try.  Make the patches
> > > up, submit them to the maintainers, and if they are wrong, they will be
> > > the best to fix it up properly.
> > 
> > Well, I could generalize all cases and always issue a WARN() if the request
> > fails. But this would just move a compile-time warning into a runtime warning.
> > Also, I have my doubts that even the arch/mach-maintainers know all the boards
> > and their peculiarities. There are thousands of them.
> 
> Well, they better know the peculiarities of the hardware they write code
> for :)

For the architecture, yes certainly. But for every board? I wonder...
Even if it is possible for one person to fix all the warnings, it is
surely beyond my resources, because I believe it should be done
manually. The pity is: My main intention was not primarily to cure old
bugs, but to prevent new bugs from happening. Some CONFIG-option would
still allow that which is what I'd prefer meanwhile. Okay, I can still
apply my patches locally and simply send fixes for code which I use.
Maybe others will do, too.

> But no, a WARN() might not be that nice as it usually is never seen by
> embedded developers...

Well, not after devices are deployed. It is seen during development
which is also the time those bugs would typically appear.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]