On Sat, 18 May 2013, Daniel Santos wrote:
> 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);
Note that this call won't work if you are holding a spinlock.
> 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.
Using interrupt transfers for commands and responses is another bad
design. No doubt it flows from the decision to support the HID
interface.
> 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.
I don't know enough about the HID stack to answer this question.
However, I suspect that the HID stack won't be a very good match to
something which isn't really an HID device.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
