[PATCH] gpiolib: fix oops on reading sysfs exported GPIOs

[PATCH] gpiolib: fix oops on reading sysfs exported GPIOs

[Anton Vorontsov]

We can get the following oops when a GPIO controller doesn't provide
.direction_input and .get callbacks:

root@b1:~# cat /sys/class/gpio/gpio255/value
Unable to handle kernel paging request for instruction fetch
Faulting instruction address: 0x00000000
Oops: Kernel access of bad area, sig: 11 [#1]
[...]
NIP [00000000] 0x0
LR [c0182fb0] gpio_get_value_cansleep+0x40/0x50
Call Trace:
[c7b79e80] [c0183f28] gpio_value_show+0x5c/0x94
[c7b79ea0] [c01a584c] dev_attr_show+0x30/0x7c
[c7b79eb0] [c00d6b48] fill_read_buffer+0x68/0xe0
[c7b79ed0] [c00d6c54] sysfs_read_file+0x94/0xbc
[c7b79ef0] [c008f24c] vfs_read+0xb4/0x16c
[c7b79f10] [c008f580] sys_read+0x4c/0x90
[c7b79f40] [c0013a14] ret_from_syscall+0x0/0x38

GPIO users should always issue the gpio_direction_input() call and
check its return value prior to trying gpio_get_value().

For sysfs users there are few ways to solve the problem:

1. Call gpio_direction_input() in the gpio_value_show(). This isn't
   good because some GPIO controllers provide capability to read-back
   output pins. Using the FLAG_IS_OUT isn't good for the same reason.

2. Call gpio_direction_input() at the export time, if succeeded, set
   FLAG_CAN_INPUT. Then check that flag in the gpio_value_show().
   Viable.

3. Just check for .get != NULL in the gpio_value_show(). Most
   straightforward. This is implemented in the patch.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/gpio/gpiolib.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 8d29405..4e4a498 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -236,6 +236,8 @@ static ssize_t gpio_value_show(struct device *dev,
 
 	if (!test_bit(FLAG_EXPORT, &desc->flags))
 		status = -EIO;
+	else if (!desc->chip->get)
+		status = -EINVAL;
 	else
 		status = sprintf(buf, "%d\n", gpio_get_value_cansleep(gpio));
 
-- 
1.5.6.3

Re: [PATCH] gpiolib: fix oops on reading sysfs exported GPIOs

[David Brownell]

On Thursday 16 October 2008, Anton Vorontsov wrote:

> Oops: Kernel access of bad area, sig: 11 [#1]
> [...]
> NIP [00000000] 0x0
> LR [c0182fb0] gpio_get_value_cansleep+0x40/0x50
> Call Trace:
> [c7b79e80] [c0183f28] gpio_value_show+0x5c/0x94
> [c7b79ea0] [c01a584c] dev_attr_show+0x30/0x7c
> [c7b79eb0] [c00d6b48] fill_read_buffer+0x68/0xe0
> [c7b79ed0] [c00d6c54] sysfs_read_file+0x94/0xbc
> [c7b79ef0] [c008f24c] vfs_read+0xb4/0x16c
> [c7b79f10] [c008f580] sys_read+0x4c/0x90
> [c7b79f40] [c0013a14] ret_from_syscall+0x0/0x38

Rude.  But notice it's not unique to the sysfs path.
Trying to fix only the sysfs path allows oosping in
other cases.


> GPIO users should always issue the gpio_direction_input() call and
> check its return value prior to trying gpio_get_value().

Not true; the API explicitly allows GPIOs to be treated
as bidirectional, even when they're configured as outputs.
That's because most GPIOs *are* bidirectional.

See the better patch in my next message.

Re: [PATCH] gpiolib: fix oops on reading sysfs exported GPIOs

[Anton Vorontsov]

On Thu, Oct 16, 2008 at 08:43:08AM -0700, David Brownell wrote:
[...]
> > GPIO users should always issue the gpio_direction_input() call and
> > check its return value prior to trying gpio_get_value().
> 
> Not true; the API explicitly allows GPIOs to be treated
> as bidirectional, even when they're configured as outputs.
> That's because most GPIOs *are* bidirectional.

I just recalling somebody was speaking about not wasting cycles with
the checks in the gpio_{set,get}_value(). And the solution was that
before using the gpio_{set,get} one should always try to issue the
"direction" calls to ensure that gpio controller is capable of that
direction... That way gpio_{set,get}_value() don't have to check
anything.

Though I can't find the discussion now, maybe it was a dream... :-)


Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] gpiolib: fix oops on reading sysfs exported GPIOs

[David Brownell]

On Thursday 16 October 2008, Anton Vorontsov wrote:
> > Not true; the API explicitly allows GPIOs to be treated
> > as bidirectional, even when they're configured as outputs.
> > That's because most GPIOs *are* bidirectional.
>
> I just recalling somebody was speaking about not wasting cycles with
> the checks in the gpio_{set,get}_value().

void __gpio_set_value(unsigned gpio, int value)
{
        struct gpio_chip        *chip;

        chip = gpio_to_chip(gpio);
        WARN_ON(extra_checks && chip->can_sleep);
        chip->set(chip, gpio - chip->base, value);
}

So not many checks there at all.  The get() path has one
additional check -- is the method null? -- for the reason
summarized above.  And if you're coming from userspace via
sysfs, you won't even notice those checks.


> And the solution was that 
> before using the gpio_{set,get} one should always try to issue the
> "direction" calls to ensure that gpio controller is capable of that
> direction... 

The reason to use gpio_direction_{output,input}() calls is
mostly to turn the output drivers on/off.  Secondarily they
do report errors when that particular GPIO can't do that.

If you really want GPIO operations to be two instructions
with no subroutine calls, make sure your platform supports
inlining the gpio_get_value() calls that have constant params
that correspond to on-chip GPIOs.  (Can't be done over I2C...)

That *CAN* make a big difference in bitbang speed ... multiple
megabits per second with SPI, for example, vs less than one.

- Dave