Re: [PATCH] carl9170: fix sending URBs with wrong type when using full-speed

[Christian Lamparter <chunkeey@googlemail.com>]

Hello,

On Thursday, August 07, 2014 11:33:06 AM Ronald Wahl wrote:
> On 07.08.2014 11:24, Ronald Wahl wrote:
> > +	/* We need to remember the type of endpoint 4 because it differs
> > +	 * between high- and full-speed configuration. The high-speed
> > +	 * configuration specifies it as interrupt and the full-speed
> > +	 * configuration as bulk endpoint. This information is required
> > +	 * later when sending urbs to that endpoint.
> > +	 */
> > +	for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; ++i) {
> > +		ep = &intf->cur_altsetting->endpoint[i].desc;
> > +
> > +		if (usb_endpoint_dir_out(ep) && usb_endpoint_num(ep) == 4)
> > +			ar->usb_ep4_is_bulk = true;
> > +	}
> 
> just after sending out that patch I have seen that I should better use 
> AR9170_USB_EP_CMD instead of 4 when checking for the endpoint and maybe 
> rename the flag into usb_ep_cmd_is_bulk. But before creating v2 of the 
> patch I want to hear your comments.

Good catch! Also: thanks for the patch! 

my comments:
 - Patch needs a "signed-off-by: <Your Name> <Your Mail> tag"
   (see Section 12 - Sign your work [0]) for details.

 - Do you see any improvement on a fullspeed port now? 
   (If so, this should be a stable- patch)

 - CC' "John W. Linville" <linville@tuxdriver.com>

 - checkpatch.pl [0] mutters about:

CHECK: Alignment should match open parenthesis
#97: FILE: drivers/net/wireless/ath/carl9170/usb.c:626:
+               usb_fill_bulk_urb(urb, ar->udev, usb_sndbulkpipe(ar->udev,
+                       AR9170_USB_EP_CMD), cmd, cmd->hdr.len + 4,

CHECK: Alignment should match open parenthesis
#101: FILE: drivers/net/wireless/ath/carl9170/usb.c:630:
+               usb_fill_int_urb(urb, ar->udev, usb_sndintpipe(ar->udev,
+                       AR9170_USB_EP_CMD), cmd, cmd->hdr.len + 4,

 - code
>@@ -1050,6 +1056,21 @@ static int carl9170_usb_probe(struct usb_interface *intf,
>        ar->intf = intf;
>        ar->features = id->driver_info;
> 
>+       /* We need to remember the type of endpoint 4 because it differs
>+        * between high- and full-speed configuration. The high-speed
>+        * configuration specifies it as interrupt and the full-speed
>+        * configuration as bulk endpoint. This information is required
>+        * later when sending urbs to that endpoint.
>+        */
>+       for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; ++i) {
>+               ep = &intf->cur_altsetting->endpoint[i].desc;
>+
>+               if (usb_endpoint_dir_out(ep) &&
>+                   usb_endpoint_num(ep) == AR9170_USB_EP_CMD)
>+                       ar->usb_ep_cmd_is_bulk =
>+                               usb_endpoint_type(ep) == USB_ENDPOINT_XFER_BULK;
What about this:

               if (usb_endpoint_num(ep) == AR9170_USB_EP_CMD &&
                   usb_endpoint_is_bulk_out(ep))
                       ar->usb_ep_cmd_is_bulk = true;
(the driver context "ar" is zero'd out - It is not necessary to set 
usb_ep_cmd_is_bulk to false.)

BTW: I'll make a patch to carl9170 firmware too. 

Regards

Christian

[0] https://www.kernel.org/doc/Documentation/SubmittingPatches