On 21/01/16 14:18, Oliver Neukum wrote:
Quting the relevant thread:
In fact, I suspect the locking added by the kernel 3.13 commit for
read_descriptors() is invalid because read_descriptors() performs no USB
activity; read_descriptors() just reads information from an allocated
memory structure. This structure is protected as the structure is
existing before and after the sysfs vfs descriptors entry is created or
destroyed.
You're right. For some reason I thought that usb_deauthorize_device()
would destroy the rawdescriptor structures (as mentioned in that
commit's Changelog), but it doesn't. The locking in read_descriptors()
is unnecessary.
The information is only written at the time of enumeration
and does not change. At least that is my understanding.
It is noted that in our testing of kernel 3.8 on ARM, that sysfs
read_descriptors() was non-blocking because the kernel 3.13 comment was
not there.
The pre-kernel 3.13 sysfs read_descriptors() seemed to work OK.
Proposal:
=========
Remove the usb_lock_device(udev) and usb_unlock_device(udev) from
devices/usb/core/sysfs.c in read_descriptors() that was added by the
kernel 3.13 commit
"232275a USB: fix substandard locking for the sysfs files"
Any comments to this proposal ?
It seems okay to me. Please submit a patch.
So this removes the locking making the point about -EINTR in
the first path moot.
Oliver, in my opinion it would be simpler to reverse your 2 patches so
that the locking is removed from read_descriptors() first and then the
fix for -EINTR for the other functions be added in the 2nd commit.
At least that would make it simpler for us to just take your commit to
remove the read_descriptors() locking from our kernel trees without the
need to re-work due to back-porting.
Anyway, please go ahead and provide a patch to remove the locking in
read_descriptors() because I am too busy to concentrate on this issue at
the moment.
Thanks,
Regards,
Dean
Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
drivers/usb/core/sysfs.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index 69da1d5..1ea7244 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -840,16 +840,13 @@ read_descriptors(struct file *filp, struct kobject *kobj,
struct usb_device *udev = to_usb_device(dev);
size_t nleft = count;
size_t srclen, n;
- int cfgno, rc;
+ int cfgno;
void *src;
/* The binary attribute begins with the device descriptor.
* Following that are the raw descriptor entries for all the
* configurations (config plus subsidiary descriptors).
*/
- rc = usb_lock_device_interruptible(udev);
- if (rc < 0)
- return -EINTR;
for (cfgno = -1; cfgno < udev->descriptor.bNumConfigurations &&
nleft > 0; ++cfgno) {
if (cfgno < 0) {
@@ -870,7 +867,6 @@ read_descriptors(struct file *filp, struct kobject *kobj,
off -= srclen;
}
}
- usb_unlock_device(udev);
return count - nleft;
}
--
Dean Jenkins
Embedded Software Engineer
Linux Transportation Solutions
Mentor Embedded Software Division
Mentor Graphics (UK) Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
