On Mon, 24 Jun 2013, Daniel Santos wrote:
> >> I submit an out URB (to rx my response)
> > No, an OUT URB transfers data _out_ from the computer to the device.
> > The response goes from the device _in_ to the computer.
>
> I sure do appreciate this because I was initially confused about this
> and am now re-confused. I presumed that "out" meant "out of the
> computer and into the USB device".
That is exactly what it means.
> However, I indeed have the variable
> labeled "out" that I am using to receive responses and visa versa. I
> have this little struct I use for both the IN and OUT URBs in my device
> struct:
>
> struct mcp2210_endpoint_proxy {
> const struct usb_host_endpoint *ep;
> struct urb *urb;
> struct mcp2210_msg *buffer;
> /* FIXME: inefficient */
> u8 submitted:1;
> u8 completed:1;
> u8 kill:1;
> u8 retry_count:4;
> u8 is_dir_in:1;
> };
>
> /* then in my device struct: */
> struct mcp2210_device {
> struct usb_device *udev;
> struct usb_interface *intf;
> struct spi_master *spi_master;
> struct gpio_chip *gpio_chip;
>
> ...
> struct mcp2210_endpoint_proxy in;
> struct mcp2210_endpoint_proxy out;
> ...
> };
>
> I initialize them both with a single function and it works! This is
> what caused me to believe that the URB directions were described from
> the USB device standpoint. (please pardon my mailer's mangling of
> whitespace)
>
>
> static __always_inline int init_endpoint(struct mcp2210_device *dev,
> const struct usb_host_endpoint *ep)
> {
> int is_dir_in = !!usb_endpoint_dir_in(&ep->desc);
> struct mcp2210_endpoint_proxy *dest;
> unsigned int pipe;
> usb_complete_t complete_fn;
> struct urb *urb;
>
> if (is_dir_in) {
> dest = &dev->in;
> pipe = usb_sndintpipe(dev->udev, ep->desc.bEndpointAddress);
> complete_fn = urb_complete_in;
> } else {
> dest = &dev->out;
> pipe = usb_rcvintpipe(dev->udev, ep->desc.bEndpointAddress);
> complete_fn = urb_complete_out;
> }
> dest->is_dir_in = is_dir_in;
So you're assigning an OUT pipe value to the "in" proxy and vice versa.
But the OUT pipe uses the endpoint number of the IN endpoint, and vice
versa.
This might work if the two endpoints have the same number (for example,
if the OUT endpoint's address is 0x02 and the IN endpoint's address is
0x82). However, it certainly isn't what you should be doing!
I guess we should add a check to the debugging code, to make sure the
pipe's direction matches the endpoint's direction.
> if (unlikely(dest->ep)) {
> mcp2210_warn("Unexpected: more than one interrupt %s endpoint "
> "discovered, ingoring them\n",
> urb_dir_str[is_dir_in]);
> return 0;
> }
>
> urb = usb_alloc_urb(0, GFP_KERNEL);
> if (unlikely(!urb)) {
> mcp2210_err("Failed to alloc %s URB\n", urb_dir_str[is_dir_in]);
> return -ENOMEM;
> }
>
> dest->buffer = usb_alloc_coherent(dev->udev, 64, GFP_KERNEL,
> &urb->transfer_dma);
Most likely you don't need to use a coherent buffer.
> if (unlikely(!dest->buffer)) {
> usb_free_urb(urb);
> mcp2210_err("Failed to alloc %s URB DMA buffer\n",
> urb_dir_str[is_dir_in]);
> return -ENOMEM;
> }
>
> dest->ep = ep;
> dest->urb = urb;
>
> usb_fill_int_urb(urb,
> dev->udev,
> pipe,
> dest->buffer,
> 64,
> complete_fn,
> dev,
> ep->desc.bInterval);
> urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
>
> return 0;
> }
>
> This is how I use it in my probe function:
>
> int mcp2210_probe(struct usb_interface *intf, const struct usb_device_id
> *id)
> {
> struct usb_device *udev = interface_to_usbdev(intf);
> struct usb_host_interface *intf_desc = intf->cur_altsetting;
> struct mcp2210_device *dev;
> struct usb_host_endpoint *ep, *ep_end;
> int ret = -ENODEV;
> int i;
>
> printk("mcp2210_probe\n");
>
> ...
>
> /* Set up interrupt endpoint information. */
> ep_end = &intf_desc->endpoint[intf_desc->desc.bNumEndpoints];
> for (ep = intf_desc->endpoint; ep != ep_end; ++ep) {
>
> if (!usb_endpoint_xfer_int(&ep->desc))
> continue;
>
> if (init_endpoint(dev, ep))
> goto error0;
> }
>
> if (unlikely(!dev->in.ep || !dev->out.ep)) {
> mcp2210_err("could not find in and/or out interrupt endpoints");
> goto error0;
> }
>
> ...
> }
>
> I don't understand why this works and the previous didn't in light of
> your comment (as well as my original assumptions). Can you see any
> flaws in this? I've used !!usb_endpoint_dir_in(&ep->desc) to get a one
> or zero for rather or not I have the IN endpoint since I store this in a
> u8:1 bitfield.
usb_endpoint_dir_in() always returns a 0 or 1 value, so the !! isn't
needed.
> Gotcha here! My thought on submitting them both at the same time is to
> (hopefully, theoretically, etc.) speed up the transaction, but also to
> give me more opportunities to allocate memory from the GFP_KERNEL pool
> (I'm not really sure how much usb_submit_urb needs to allocate).
Not much memory is needed. Anyway, submitting them both should not
cause any problems.
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
