gpiolib: fix oops on reading sysfs exported GPIOs
PATCH 1/1 gpiolib: fix oops on reading sysfs exported GPIOs
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.
--- drivers/gpio/gpiolib.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/gpio/gpiolib.c +2 -0
@@ -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));
Signature · 1 line
1.5.6.3
Quoted 4 lines · Anton Vorontsov
Oops: Kernel access of bad area, sig: 11 [#1][...]
NIP [00000000]0x0
LR [c0182fb0]gpio_get_value_cansleep+0x40/0x50
Quoted 8 lines
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.
Quoted 2 lines
GPIO users should always issue the
gpio_direction_input()call and check its return value prior to tryinggpio_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.
On Thu, Oct 16, 2008 at 08:43:08AM -0700, David Brownell wrote:
[...]
Quoted 6 lines
Quoted 2 lines
GPIO users should always issue the
gpio_direction_input()call and check its return value prior to tryinggpio_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,
Signature · 3 lines
Anton Vorontsov email: [email protected] irc://irc.freenode.net/bd2
Quoted 6 lines · Anton Vorontsov
Quoted 3 lines
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.
Quoted 4 lines
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