gpio/gpio-sysfs: Try to export busy GPIO line leads to wrong GPIO line exporting

PATCH In review Denis Kuzmenko 2 participants 5 messages Nov 13, 2011 – Nov 15, 2011 GPIO

lore

Share & tools

lore

Denis Kuzmenko

author

2011-11-13 01:30 UTC #0 lore raw

PATCH 1/1 gpio/gpio-sysfs: Try to export busy GPIO line leads to wrong GPIO line exporting

From: Denis Kuzmenko <[email protected]>

Fix bug in gpio-sysfs interface (export of busy GPIO line leads to export of different GPIO line).

Signed-off-by Denis Kuzmenko <[email protected]>

---

Patch is against 3.0.9
When trying to export GPIO line 37(40) which is already exported/requested by kernel code we got GPIO line 3(4) exported. Looks like this is done because export_store function doesn't return the number of processed bytes and gets a part of previous buffer again. This fix works for me (Samsung s3c2440).

drivers/gpio/gpiolib.c +1 -1

index a971e3d..ccec497 100644
@@ -633,7 +633,7 @@ static ssize_t export_store(struct class *class,
 done:
 	if (status)
 		pr_debug("%s: status %d\n", __func__, status);
-	return status ? : len;
+	return len;
 }
 
 static ssize_t unexport_store(struct class *class,

Stephen Warren

2011-11-14 20:45 UTC #1 lore raw

Denis Kuzmenko wrote at Saturday, November 12, 2011 6:31 PM:

Quoted 19 lines

From: Denis Kuzmenko <[email protected]>
Fix bug in gpio-sysfs interface (export of busy GPIO line leads to export of different GPIO line).
Signed-off-by Denis Kuzmenko <[email protected]>
---
Patch is against 3.0.9
When trying to export GPIO line 37(40) which is already exported/requested by kernel code we got GPIO line 3(4) exported.
Looks like this is done because export_store function doesn't return the number of processed bytes and gets a part of previous buffer again.
This fix works for me (Samsung s3c2440).
drivers/gpio/gpiolib.c +0 -0
index a971e3d..ccec497 100644

drivers/gpio/gpiolib.c static ssize_t export_store(struct class *class, · line 637

Quoted 6 lines

drivers/gpio/gpiolib.c +1 -1

@@ -633,7 +633,7 @@ static ssize_t export_store(struct class *class,
 done:
 	if (status)
 		pr_debug("%s: status %d\n", __func__, status);
-	return status ? : len;
+	return len;
 }

I assume that when the error occurs, status is negative. Is it some special value like EINTR, EAGAIN? I'm surprised that the retried write is smaller than the whole original buffer.

What's actually retrying the failed write? Is it user-space in response to the previous failed write, in a (mistaken?) attempt to handle shorter- than-expected-writes? You could confirm this with strace.

If the patch above really is correct, there are other places it'd be needed; it looks like e.g. drivers/video/backlight/backlight.c would have the same issue if you gave too-large integers to its sysfs files.

Signature · 1 line

nvpublic

Denis Kuzmenko

author

2011-11-14 23:13 UTC #2 lore raw

Quoted 18 lines · Stephen Warren

Denis Kuzmenko wrote at Saturday, November 12, 2011 6:31 PM:
drivers/gpio/gpiolib.c static ssize_t export_store(struct class *class, · line 637
Quoted 12 lines
drivers/gpio/gpiolib.c +1 -1
index a971e3d..ccec497 100644
@@ -633,7 +633,7 @@ static ssize_t export_store(struct class *class,
 done:
 	if (status)
 		pr_debug("%s: status %d\n", __func__, status);
-	return status ? : len;
+	return len;
 }
I assume that when the error occurs, status is negative. Is it some special value like EINTR, EAGAIN? I'm surprised that the retried write is smaller than the whole original buffer.

Quoted 8 lines

What's actually retrying the failed write? Is it user-space in response to the previous failed write, in a (mistaken?) attempt to handle shorter- than-expected-writes? You could confirm this with strace.
If the patch above really is correct, there are other places it'd be needed; it looks like e.g. drivers/video/backlight/backlight.c would have the same issue if you gave too-large integers to its sysfs files.

Hi,

Looks like userspace doesn't retries write. Here is trace:

open("/sys/class/gpio/export", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 3
ioctl(3, SNDCTL_TMR_TIMEBASE or TCGETS, 0xbef76c64) = -1 ENOTTY (Inappropriate ioctl for device)

brk(0x30e000)                           = 0x30e000
write(3, "37\n", 3)                     = -1 EBUSY (Device or resource busy)
close(3)                                = 0
exit(0)                                 = ?

this was made by simple self-written program:

	fopen("/sys/class/gpio/export", "w"); fprintf(fp, "37\n");

additional kernel-trace (WARN_ON(0 > status)) was also added before function return:

WARNING: at drivers/gpio/gpiolib.c:637 export_store+0xa0/0xb4()
Modules linked in:
[<c0028064>] (unwind_backtrace+0x0/0xf0) from [<c0039234>] (warn_slowpath_common+0x48/0x60)
[<c0039234>] (warn_slowpath_common+0x48/0x60) from [<c0039268>] (warn_slowpath_null+0x1c/0x24)
[<c0039268>] (warn_slowpath_null+0x1c/0x24) from [<c0182da8>] (export_store+0xa0/0xb4)
[<c0182da8>] (export_store+0xa0/0xb4) from [<c01b17e0>] (class_attr_store+0x1c/0x28)
[<c01b17e0>] (class_attr_store+0x1c/0x28) from [<c00db9ac>] (sysfs_write_file+0xfc/0x180)
[<c00db9ac>] (sysfs_write_file+0xfc/0x180) from [<c0093304>] (vfs_write+0xac/0x13c)
[<c0093304>] (vfs_write+0xac/0x13c) from [<c0093578>] (sys_write+0x40/0x6c)
[<c0093578>] (sys_write+0x40/0x6c) from [<c0023ffc>] (__sys_trace_return+0x0/0x24)
---[ end trace 8141a98436c712ec ]---

I'll try digg into functions in backtrace and find retry if any.

Best regards, Denis Kuzmenko.

Denis Kuzmenko

author

2011-11-15 20:22 UTC #3 lore raw

Hi,

Quoted 7 lines · Denis Kuzmenko

Quoted 1 line · Stephen Warren
Denis Kuzmenko wrote at Saturday, November 12, 2011 6:31 PM:
Looks like userspace doesn't retries write. Here is trace:
open("/sys/class/gpio/export", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 3
ioctl(3, SNDCTL_TMR_TIMEBASE or TCGETS, 0xbef76c64) = -1 ENOTTY (Inappropriate ioctl for device)

Quoted 7 lines

brk(0x30e000)                           = 0x30e000
write(3, "37\n", 3)                     = -1 EBUSY (Device or resource busy)
close(3)                                = 0
exit(0)                                 = ?

this was made by simple self-written program:

	fopen("/sys/class/gpio/export", "w"); fprintf(fp, "37\n");

That was userspace - problem in not reproducible with a code above. The only way I know to reproduce is:
echo 37 > /sys/class/gpio/export

Traces are generated:

execve("./test-gpio.sh", ["./test-gpio.sh"], [/* 15 vars */]) = 0
mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS|0x4000000, -1, 0) = 0x400c0000
open("/lib/libc.so.0", O_RDONLY) = 3
fstat(3, {st_mode=S_IFREG|0755, st_size=244456, ...}) = 0
mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS|0x4000000, -1, 0) = 0x40183000 read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0(\0\1\0\0\0 \204\0\0004\0\0\0"..., 4096) = 4096 mmap2(NULL, 294912, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x401b9000 mmap2(0x401b9000, 237612, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_FIXED, 3, 0) = 0x401b9000 mmap2(0x401fb000, 4848, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED, 3, 0x3a) = 0x401fb000 mmap2(0x401fd000, 12940, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x401fd000
close(3) = 0
munmap(0x40183000, 4096) = 0
stat("/lib/ld-uClibc.so.0", {st_mode=S_IFREG|0755, st_size=21200, ...}) = 0
mprotect(0x401fb000, 4096, PROT_READ) = 0
mprotect(0x4005f000, 4096, PROT_READ) = 0
ioctl(0, SNDCTL_TMR_TIMEBASE or TCGETS, {B115200 opost isig icanon echo ...}) = 0 ioctl(1, SNDCTL_TMR_TIMEBASE or TCGETS, {B115200 opost isig icanon echo ...}) = 0

getuid32()                              = 0
brk(0)                                  = 0xb63000
brk(0xb64000)                           = 0xb64000
brk(0xb65000)                           = 0xb65000
getpid()                                = 540

rt_sigaction(SIGCHLD, {SIG_DFL, [CHLD], SA_RESTART|0x4000000}, {SIG_DFL, [], 0}, 8) = 0 rt_sigaction(SIGHUP, {SIG_DFL, [HUP], SA_RESTART|0x4000000}, {SIG_DFL, [], 0}, 8) = 0
getppid() = 539
stat64("/root", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0 stat64(".", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0 open("./test-gpio.sh", O_RDONLY|O_LARGEFILE) = 3

fcntl64(3, F_DUPFD, 10)                 = 10
close(3)                                = 0
fcntl64(10, F_SETFD, FD_CLOEXEC)        = 0

rt_sigaction(SIGINT, NULL, {SIG_DFL, [], 0}, 8) = 0 rt_sigaction(SIGINT, {0x2f724, ~[], 0x4000000 /* SA_??? */}, NULL, 8) = 0 rt_sigaction(SIGQUIT, NULL, {SIG_DFL, [], 0}, 8) = 0 rt_sigaction(SIGQUIT, {SIG_IGN, [], 0x4000000 /* SA_??? */}, NULL, 8) = 0 rt_sigaction(SIGTERM, NULL, {SIG_DFL, [], 0}, 8) = 0 rt_sigaction(SIGTERM, {SIG_DFL, [], 0x4000000 /* SA_??? */}, NULL, 8) = 0 read(10, "#!/bin/sh\necho 37 > /sys/class/g"..., 1023) = 43 open("/sys/class/gpio/export", O_WRONLY|O_CREAT|O_TRUNC|O_LARGEFILE, 0666) = 3

fcntl64(1, F_DUPFD, 10)                 = 11
dup2(3, 1)                              = 1
close(3)                                = 0
wait4(-1, 0xbed568d4, WNOHANG, NULL)    = -1 ECHILD (No child processes)
fcntl64(1, F_GETFL)                     = 0x20001 (flags O_WRONLY|O_LARGEFILE)
write(1, "37\n", 3)                     = -1 EBUSY (Device or resource busy)
write(1, "3", 1)                        = -1 EBUSY (Device or resource busy)
write(1, "3", 1)                        = -1 EBUSY (Device or resource busy)
dup2(11, 1)                             = 1
close(11)                               = 0

write(1, "3", 13) = 1
read(10, "", 1023) = 0
exit(255) = ?

Traces added to kernel:

export_store: 0x33
export_store: 0x37
export_store: 0xa
export_store: 37

------------[ cut here ]------------
WARNING: at drivers/gpio/gpiolib.c:650 export_store+0xe8/0x148()
Modules linked in:
[<c0028064>] (unwind_backtrace+0x0/0xf0) from [<c0039234>] (warn_slowpath_common+0x48/0x60)
[<c0039234>] (warn_slowpath_common+0x48/0x60) from [<c0039268>] (warn_slowpath_null+0x1c/0x24)
[<c0039268>] (warn_slowpath_null+0x1c/0x24) from [<c0182df0>] (export_store+0xe8/0x148)
[<c0182df0>] (export_store+0xe8/0x148) from [<c01b1bf0>] (class_attr_store+0x30/0xcc)
[<c01b1bf0>] (class_attr_store+0x30/0xcc) from [<c00db9ac>] (sysfs_write_file+0xfc/0x180)
[<c00db9ac>] (sysfs_write_file+0xfc/0x180) from [<c0093304>] (vfs_write+0xac/0x13c)
[<c0093304>] (vfs_write+0xac/0x13c) from [<c0093578>] (sys_write+0x40/0x6c)
[<c0093578>] (sys_write+0x40/0x6c) from [<c0023ffc>] (__sys_trace_return+0x0/0x24)
---[ end trace a23bd23b77702f39 ]---
export_store: 0x33 
export_store: 3
------------[ cut here ]------------
WARNING: at drivers/gpio/gpiolib.c:650 export_store+0xe8/0x148()
Modules linked in:
[<c0028064>] (unwind_backtrace+0x0/0xf0) from [<c0039234>] (warn_slowpath_common+0x48/0x60)
[<c0039234>] (warn_slowpath_common+0x48/0x60) from [<c0039268>] (warn_slowpath_null+0x1c/0x24)
[<c0039268>] (warn_slowpath_null+0x1c/0x24) from [<c0182df0>] (export_store+0xe8/0x148)
[<c0182df0>] (export_store+0xe8/0x148) from [<c01b1bf0>] (class_attr_store+0x30/0xcc)
[<c01b1bf0>] (class_attr_store+0x30/0xcc) from [<c00db9ac>] (sysfs_write_file+0xfc/0x180)
[<c00db9ac>] (sysfs_write_file+0xfc/0x180) from [<c0093304>] (vfs_write+0xac/0x13c)
[<c0093304>] (vfs_write+0xac/0x13c) from [<c0093578>] (sys_write+0x40/0x6c)
[<c0093578>] (sys_write+0x40/0x6c) from [<c0023ffc>] (__sys_trace_return+0x0/0x24)
---[ end trace a23bd23b77702f3a ]---
export_store: 0x33 
export_store: 3
------------[ cut here ]------------
WARNING: at drivers/gpio/gpiolib.c:650 export_store+0xe8/0x148()
Modules linked in:
[<c0028064>] (unwind_backtrace+0x0/0xf0) from [<c0039234>] (warn_slowpath_common+0x48/0x60)
[<c0039234>] (warn_slowpath_common+0x48/0x60) from [<c0039268>] (warn_slowpath_null+0x1c/0x24)
[<c0039268>] (warn_slowpath_null+0x1c/0x24) from [<c0182df0>] (export_store+0xe8/0x148)
[<c0182df0>] (export_store+0xe8/0x148) from [<c01b1bf0>] (class_attr_store+0x30/0xcc)
[<c01b1bf0>] (class_attr_store+0x30/0xcc) from [<c00db9ac>] (sysfs_write_file+0xfc/0x180)
[<c00db9ac>] (sysfs_write_file+0xfc/0x180) from [<c0093304>] (vfs_write+0xac/0x13c)
[<c0093304>] (vfs_write+0xac/0x13c) from [<c0093578>] (sys_write+0x40/0x6c)
[<c0093578>] (sys_write+0x40/0x6c) from [<c0023ffc>] (__sys_trace_return+0x0/0x24)
---[ end trace a23bd23b77702f3b ]---

Can someone point me where the problem can be? Should i search it in command interpreter or in libc?

Signature · 1 line

Best regards, Denis Kuzmenko.

Stephen Warren

2011-11-15 21:14 UTC #4 lore raw

Denis Kuzmenko wrote at Tuesday, November 15, 2011 1:24 PM:

Quoted 22 lines · Denis Kuzmenko

Quoted 7 lines · Stephen Warren
Quoted 1 line
Denis Kuzmenko wrote at Saturday, November 12, 2011 6:31 PM:
Looks like userspace doesn't retries write. Here is trace:
open("/sys/class/gpio/export", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 3
ioctl(3, SNDCTL_TMR_TIMEBASE or TCGETS, 0xbef76c64) = -1 ENOTTY (Inappropriate ioctl for device)
Quoted 7 lines
brk(0x30e000)                           = 0x30e000
write(3, "37\n", 3)                     = -1 EBUSY (Device or resource busy)
close(3)                                = 0
exit(0)                                 = ?
this was made by simple self-written program:
	fopen("/sys/class/gpio/export", "w"); fprintf(fp, "37\n");
That was userspace - problem in not reproducible with a code above. The only way I know to reproduce is:
echo 37 > /sys/class/gpio/export
Traces are generated:

...

Quoted 1 line

stat("/lib/ld-uClibc.so.0", {st_mode=S_IFREG|0755, st_size=21200, ...}) = 0

Hmmm. I'd be tempted to look at the stdio implementation in uClibc first, or possibly the shell you're running.

Quoted 3 lines

write(1, "37\n", 3)                     = -1 EBUSY (Device or resource busy)
write(1, "3", 1)                        = -1 EBUSY (Device or resource busy)
write(1, "3", 1)                        = -1 EBUSY (Device or resource busy)

My ARM system running some random build of Debian Wheezy doesn't repro that; I see the first write() fail, and then no more writes are attempted.

For the record, I tried GPIO 570 (an invalid value), and got back -EINVAL, and GPIO 57 (a valid but already requested GPIO) and got back -EBUSY. Both had the same behavior.

Signature · 1 line

nvpublic