usb sysfs file handling functions don't take

2013-05-18 Thread Hans de Goede 

Hi,

As written in my mail titled: "Linux sysfs usb descriptors
file has broken configuration length handling"

I've been taking a close look at the usb sysfs handling
code, specifically for the descriptors sysfs file.

One other difference I've noticed is that the usbfs
code for reading the descriptors does a usb_lock_device,
whereas read_descriptors for the sysfs descriptors file
does not. Unless I'm mistaken that means the sysfs
code can race with (re)-enumeration and bad things could
happen.

Similar concerns apply to the other usb sysfs files.

Regards,

Hans

--
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

Linux sysfs usb descriptors file has broken configuration length handling

2013-05-18 Thread Hans de Goede 

Hi All,

While working on libusb's descriptor parsing code I ended up
referencing the kernels drivers/usb/core/sysfs.c and
drivers/usb/core/devio.c files. And I noticed a worrisome
discrepancy.

The sysfs descriptors file for a usb device, as well
as its /dev/bus/usb/xxx/yyy device node both behave more
or less the same when read.

There is 1 difference which is by-design / has historical
grown that way. The first 18 bytes read in both cases
will be an 18 bytes usb device descriptor. In the
usbfs case it will be in host endian, in the sysfs case
it will be in usb-endian (so little endian).

But there is another difference which I believe to
be a problem, after the usb device descriptor both
implementations follow with the config descriptor(s) in
raw format, using struct usb_device->rawdescriptors[x]
as the source.

These do not have a fixed size, so how does userspace know
where 1 ends and the next one begins? Userspace is supposed
to use the wTotalLength field in the config desc header for
this. Which comes from the device, and the actual read of
the config desc from the device may have returned less
bytes, so usb_device->rawdescriptors[x] may contain less
data then this.

Both the usbfs and sysfs code ensure to not return bogus
data by limiting the amount of read data to
usb_device->config[x].desc.wTotalLength
which has been set to the actual amount of available data.

But then things start to differ, usbfs leaves holes in
the file the size of the missing data, so that in
case of usb_device->config[x].desc.wTotalLength being
less then the length advertised by the rawdescriptors,
the next descriptor will still start where user space
expects it to start.

But the sysfs descriptors file will just packs the
rawdescriptors one behind the other, using
usb_device->config[x].desc.wTotalLength, where as
userspace only sees the length advertised by
the rawdescriptors, which may be different, and when
it is userspace will this have no idea where the next
descriptor starts.

I believe the proper way to fix this is to make the
sysfs code deal with this the same way the usbfs code
does (filling the holes with 0 to avoid leaking kmem),
if people agree I can write a patch for this.

Regards,

Hans

p.s.

In the mean time I will switch libusb to use usbfs
device nodes to get the descriptors exclusively,
since those will work reliable (even on non fixed
kernels), whereas the sysfs descriptors file will not.

Neither causes any device io, so the performance
impact should be negligible.
--
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

Re: Question about "generic" HID devices

2013-05-18 Thread Alan Stern 
On Fri, 17 May 2013, Daniel Santos wrote:

> Hello.  I'm working on a driver for a USB to SPI bridge chip 
> (Microchip's MCP2210) that acts as a HID device (generic subclass). Of 
> course, it can be used from user-space via hid-generic, but I'm not 
> really interested in that. I started this work from somebody else's 
> implementation that was a HID-based driver, but didn't like the 
> requirement to allocate 6k of data for the report for each 64 byte 
> rep/req so I started over as a plane-jane USB driver.  I suppose this 
> was a bit naive, having never written a USB driver before, I guess I 
> relish challenges.  Also, I'm intending to use this driver on a 
> low-memory embedded device.
> 
> So I have the basics of my driver all working, sending 64 byte interrupt 
> URB requests (as per the datasheet - 
> http://ww1.microchip.com/downloads/en/DeviceDoc/22288A.pdf) and 
> receiving 64 byte response URBs,

That doesn't make sense.  Your driver submits URBs to the USB stack and
waits for them to complete; it doesn't receive URBs from anywhere.  
Maybe you meant that your driver receives 64-byte response packets.  
But that doesn't make much sense either; it would require an extremely 
unusual HID device to send 64-byte responses.  For example, a mouse or 
a keyboard generally sends responses in the 3- to 5-byte range.

> except that the responses are always 
> all zeros. :(  So I ran usbmon and observed the traffic when the 
> hid-core and hid-generic drivers use it and noticed quite a bit of 
> traffic on the control endpoint leaving me guessing that this is some 
> type of HID protocol that I naively omitted and that the chip just 
> doesn't respond on it's interrupt endpoints until some HID thing happens 
> on the control endpoint.

The traffic you see is all standard HID requests.

> My other thought was that I was perhaps I'm just not waiting long enough 
> before submitting the response URB (maybe the chip needs a little time 
> to process the request and just returns all zeros when it doesn't have 
> anything to say yet?)

This depends on how you are using it.  The HID protocol includes a 
request whereby the host tells the device not to send data if there 
have not been any changes.

> -- but to test this theory, I'll need to implement 
> some type of bottom half (work queue or something).  I am supplying the 
> interrupt interval specified by bInterval (which is 1, meaning 125 uS if 
> I understand correctly).

If your device runs at high speed then 125 us is correct.  If the
device runs at full speed then 1 means 1 ms.

> Anyway, if somebody has some advice, I would greatly appreciate it. I'm 
> combing through the USB 2 spec to try to find where the HID protocol is 
> covered, but maybe that's an entirely different spec? Advice greatly 
> appreciated!!

See .

Alan Stern

--
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

Re: usb sysfs file handling functions don't take

2013-05-18 Thread Alan Stern 
On Sat, 18 May 2013, Hans de Goede wrote:

> Hi,
> 
> As written in my mail titled: "Linux sysfs usb descriptors
> file has broken configuration length handling"
> 
> I've been taking a close look at the usb sysfs handling
> code, specifically for the descriptors sysfs file.
> 
> One other difference I've noticed is that the usbfs
> code for reading the descriptors does a usb_lock_device,
> whereas read_descriptors for the sysfs descriptors file
> does not. Unless I'm mistaken that means the sysfs
> code can race with (re)-enumeration and bad things could
> happen.
> 
> Similar concerns apply to the other usb sysfs files.

It's true that several of the attribute routines in sysfs.c don't
include proper locking.  I started to work on this some time ago (back
around the 3.5 time frame, or maybe earlier) but never finished up.  
Maybe I should go back and complete the work.

Alan Stern

--
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

Re: [Libusbx-devel] Linux sysfs usb descriptors file has broken configuration length handling

2013-05-18 Thread Alan Stern 
On Sat, 18 May 2013, Hans de Goede wrote:

> Hi All,
> 
> While working on libusb's descriptor parsing code I ended up
> referencing the kernels drivers/usb/core/sysfs.c and
> drivers/usb/core/devio.c files. And I noticed a worrisome
> discrepancy.
> 
> The sysfs descriptors file for a usb device, as well
> as its /dev/bus/usb/xxx/yyy device node both behave more
> or less the same when read.
> 
> There is 1 difference which is by-design / has historical
> grown that way. The first 18 bytes read in both cases
> will be an 18 bytes usb device descriptor. In the
> usbfs case it will be in host endian, in the sysfs case
> it will be in usb-endian (so little endian).
> 
> But there is another difference which I believe to
> be a problem, after the usb device descriptor both
> implementations follow with the config descriptor(s) in
> raw format, using struct usb_device->rawdescriptors[x]
> as the source.
> 
> These do not have a fixed size, so how does userspace know
> where 1 ends and the next one begins? Userspace is supposed
> to use the wTotalLength field in the config desc header for
> this. Which comes from the device, and the actual read of
> the config desc from the device may have returned less
> bytes, so usb_device->rawdescriptors[x] may contain less
> data then this.
> 
> Both the usbfs and sysfs code ensure to not return bogus
> data by limiting the amount of read data to
> usb_device->config[x].desc.wTotalLength
> which has been set to the actual amount of available data.
> 
> But then things start to differ, usbfs leaves holes in
> the file the size of the missing data, so that in
> case of usb_device->config[x].desc.wTotalLength being
> less then the length advertised by the rawdescriptors,
> the next descriptor will still start where user space
> expects it to start.
> 
> But the sysfs descriptors file will just packs the
> rawdescriptors one behind the other, using
> usb_device->config[x].desc.wTotalLength, where as
> userspace only sees the length advertised by
> the rawdescriptors, which may be different, and when
> it is userspace will this have no idea where the next
> descriptor starts.
> 
> I believe the proper way to fix this is to make the
> sysfs code deal with this the same way the usbfs code
> does (filling the holes with 0 to avoid leaking kmem),
> if people agree I can write a patch for this.

That's okay with me.

Alan Stern

--
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

Re: [Libusbx-devel] Linux sysfs usb descriptors file has broken configuration length handling

2013-05-18 Thread Greg KH 
On Sat, May 18, 2013 at 12:14:08PM -0400, Alan Stern wrote:
> On Sat, 18 May 2013, Hans de Goede wrote:
> > But the sysfs descriptors file will just packs the
> > rawdescriptors one behind the other, using
> > usb_device->config[x].desc.wTotalLength, where as
> > userspace only sees the length advertised by
> > the rawdescriptors, which may be different, and when
> > it is userspace will this have no idea where the next
> > descriptor starts.
> > 
> > I believe the proper way to fix this is to make the
> > sysfs code deal with this the same way the usbfs code
> > does (filling the holes with 0 to avoid leaking kmem),
> > if people agree I can write a patch for this.
> 
> That's okay with me.

Sounds good to me as well.

thanks,

greg k-h
--
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

Re: Question about "generic" HID devices

2013-05-18 Thread Daniel Santos 



The HID protocol is a separate specification, see the usb-if.org web
site for it.
Thank you, found it!  It's quite a read and I think I see where I've 
gone wrong.  I've made some terrible assumptions it would appear! :)  
That's ok, I've had fun and learned a lot thus far.


Daniel
--
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

Re: Question about "generic" HID devices

2013-05-18 Thread Daniel Santos 

First off, thanks for your helpful response! :)

On 05/18/2013 10:53 AM, Alan Stern wrote:

On Fri, 17 May 2013, Daniel Santos wrote:


Hello.  I'm working on a driver for a USB to SPI bridge chip
(Microchip's MCP2210) that acts as a HID device (generic subclass). Of
course, it can be used from user-space via hid-generic, but I'm not
really interested in that. I started this work from somebody else's
implementation that was a HID-based driver, but didn't like the
requirement to allocate 6k of data for the report for each 64 byte
rep/req so I started over as a plane-jane USB driver.  I suppose this
was a bit naive, having never written a USB driver before, I guess I
relish challenges.  Also, I'm intending to use this driver on a
low-memory embedded device.

So I have the basics of my driver all working, sending 64 byte interrupt
URB requests (as per the datasheet -
http://ww1.microchip.com/downloads/en/DeviceDoc/22288A.pdf) and
receiving 64 byte response URBs,

That doesn't make sense.  Your driver submits URBs to the USB stack and
waits for them to complete; it doesn't receive URBs from anywhere.
Maybe you meant that your driver receives 64-byte response packets.


Yes, my apologies, I didn't state that very clearly.  I meant that I 
submit a "request" URB to the in interrupt endpoint as well a "response" 
URB to the out interrupt endpoint.This is where I'm currently submitting 
these (for now just assume can_sleep is always zero, it's broken otherwise):


static int ctl_submit_req(struct mcp2210_command *cmd, int can_sleep)
{
struct mcp2210_ctl_command *ctl_cmd = (struct mcp2210_ctl_command 
*)cmd;

struct mcp2210_device *dev;
const gfp_t gfp_flags = can_sleep ? GFP_KERNEL : GFP_ATOMIC;
unsigned long irqflags;
int ret = 0;

if (!cmd || !cmd->dev || cmd->type != &mcp2210_cmd_type_ctrl)
return -EINVAL;

dev = cmd->dev;
memcpy(dev->in.buffer, &ctl_cmd->req, sizeof(ctl_cmd->req));

dev->in.int_urb->complete = cmd->type->complete_req;
dev->out.int_urb->complete = cmd->type->complete_rep;

/* lock command in case we get preempted here and a completion handler
 * is called */
/* FIXME: this makes no sense when can_sleep != 0 */
spin_lock_irqsave(&cmd->spinlock, irqflags);

ret = usb_submit_urb(dev->in.int_urb, gfp_flags);
if (ret) {
dev_err(&dev->udev->dev,
"usb_submit_urb failed for request URB %d", ret);
goto exit_unlock;
}

ret = usb_submit_urb(dev->out.int_urb, gfp_flags);
if (ret) {
dev_err(&dev->udev->dev,
"usb_submit_urb failed for response URB %d", ret);
usb_kill_urb(dev->in.int_urb);
goto exit_unlock;
}

cmd->state = MCP2210_CMD_STATE_SUBMITTED;

exit_unlock:
spin_unlock_irqrestore(&cmd->spinlock, irqflags);

return ret;
}


But that doesn't make much sense either; it would require an extremely
unusual HID device to send 64-byte responses.  For example, a mouse or
a keyboard generally sends responses in the 3- to 5-byte range.


Yes, I know.  This isn't even a device which interfaces with humans!  I 
presume Microchip implemented it as such so that it could be interfaced 
from userspace with the generic HID drivers available on all modern 
platforms.  I think I understand their design decision, but from my 
standpoint, it seems introduce a lot of needless complexity.  Section 
3.0 (page 11) of the datasheet 
(http://ww1.microchip.com/downloads/en/DeviceDoc/22288A.pdf) describes 
the USB command/response pairs it supports and all of them are 64 bytes, 
most padded with lots of zeros.  Then again, it's a $2.10 IC.


I guess this is the reason I've shied away from Mathew King's basic 
alpha driver (https://github.com/MathewKing/mcp2210-linux), with 64 byte 
messages.  For some reason, I originally thought that it was allocated 
6k of memory for each HID report, 
(https://github.com/MathewKing/mcp2210-linux/blob/master/mcp2210-core.c#L102) 
but after actually testing it (on x86_64), it's only 2456 bytes.


I guess this introduces a new (maybe better) question.  If I instead 
convert this back into a proper usbhid driver (instead of a plane USB 
interface driver as it is now), can I allocate my struct hid_report and 
struct hid_field just once and reuse them instead of allocating new ones 
for each command/response pair? The original point of doing this as a 
USB interface driver was to reduce memory requirements.



The traffic you see is all standard HID requests.


Yeah, that's what I figured -- something I wasn't doing.




My other thought was that I was perhaps I'm just not waiting long enough
before submitting the response URB (maybe the chip needs a little time
to process the request and just returns all zeros when it doesn't have
anything to say yet?)

This depends on how you are using it.  The HID protocol includes a
request whereby the host tells the device not to send data if there
have not been any changes.


Well, I have an almost d

Re: [Libusbx-devel] Linux sysfs usb descriptors file has broken configuration length handling

2013-05-18 Thread Hans de Goede 

Hi, Greg, Alan, All,

On 05/18/2013 06:17 PM, Greg KH wrote:

On Sat, May 18, 2013 at 12:14:08PM -0400, Alan Stern wrote:

On Sat, 18 May 2013, Hans de Goede wrote:

But the sysfs descriptors file will just packs the
rawdescriptors one behind the other, using
usb_device->config[x].desc.wTotalLength, where as
userspace only sees the length advertised by
the rawdescriptors, which may be different, and when
it is userspace will this have no idea where the next
descriptor starts.

I believe the proper way to fix this is to make the
sysfs code deal with this the same way the usbfs code
does (filling the holes with 0 to avoid leaking kmem),
if people agree I can write a patch for this.


That's okay with me.


Sounds good to me as well.


Thanks for the input. I've been thinking about this some-more,
and I see a possible other solution which I think we need to
consider.

Although the config.wTotalLength value in /sys/.../descriptors
can not be trusted, the kernel does sanitize things to such a
degree that the standard descriptor header bLength can be
trusted to always be valid inside /sys/.../descriptors, so
alternatively to using config.wTotalLength userspace could
simple parse things one descriptor at a time, until it
encounters another config descriptor or EOF, and reliable figure
out where the next config descriptor starts that way.

I'm sort of tending towards using that solution instead

Advantages:

1) If libusb is modified to do thing this way it will work with
older kernels, without needing to switch to using usbfs (which
in some rough benchmarks I've run turns out to be 7 times as slow
when it comes to open file, read desc, close file).

2) We're not breaking the kernel ABI, which technically my
other proposal does. I know devices with multiple configs are
rare, and one could argue the old behavior is a bug, but we
may have something out there depending on it.

Disadvantages:

1) It means that there will be some inconsistencies to how this
is handled between usbfs and sysfs (note there already is such
an inconsistency with regards to the endian-ness of the device
descriptor).

As said I tend towards using the alternative proposal I've
just suggested, input very much welcome!

Either way I noticed that descriptors is absent from
Documentation/ABI/testing/sysfs-bus-usb

Once we've a decision on which way to go, I'll document the
behavior there.


Regards,

Hans
--
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