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

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

[Ronald Wahl]

The driver assumes that endpoint 4 is always an interrupt endpoint.
Unfortunately the type differs between high-speed and full-speed
configurations while in the former case it is indeed an interrupt
endpoint this is not true for the latter case - here it is a bulk
endpoint. When sending URBs with the wrong type the kernel will
generate a warning message including backtrace.

To fix this we are now sending URBs to endpoint 4 using the type
found in the endpoint descriptor.

A side note: The carl9170 firmware currently specifies endpoint 4 as
interrupt endpoint even in the full-speed configuration but this has
no relevance because before this firmware is loaded the endpoint type
is as described above and after the firmware is running the stick is not
reenumerated and so the old descriptor is used.
---
 drivers/net/wireless/ath/carl9170/carl9170.h |  1 +
 drivers/net/wireless/ath/carl9170/usb.c      | 27 +++++++++++++++++++++++----
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/carl9170/carl9170.h b/drivers/net/wireless/ath/carl9170/carl9170.h
index 8596aba..8bc6505 100644
--- a/drivers/net/wireless/ath/carl9170/carl9170.h
+++ b/drivers/net/wireless/ath/carl9170/carl9170.h
@@ -256,6 +256,7 @@ struct ar9170 {
 	atomic_t rx_work_urbs;
 	atomic_t rx_pool_urbs;
 	kernel_ulong_t features;
+	bool usb_ep4_is_bulk;
 
 	/* firmware settings */
 	struct completion fw_load_wait;
diff --git a/drivers/net/wireless/ath/carl9170/usb.c b/drivers/net/wireless/ath/carl9170/usb.c
index f35c7f3..e712090 100644
--- a/drivers/net/wireless/ath/carl9170/usb.c
+++ b/drivers/net/wireless/ath/carl9170/usb.c
@@ -621,9 +621,14 @@ int __carl9170_exec_cmd(struct ar9170 *ar, struct carl9170_cmd *cmd,
 		goto err_free;
 	}
 
-	usb_fill_int_urb(urb, ar->udev, usb_sndintpipe(ar->udev,
-		AR9170_USB_EP_CMD), cmd, cmd->hdr.len + 4,
-		carl9170_usb_cmd_complete, ar, 1);
+	if (ar->usb_ep4_is_bulk)
+		usb_fill_bulk_urb(urb, ar->udev, usb_sndbulkpipe(ar->udev,
+			AR9170_USB_EP_CMD), cmd, cmd->hdr.len + 4,
+			carl9170_usb_cmd_complete, ar);
+	else
+		usb_fill_int_urb(urb, ar->udev, usb_sndintpipe(ar->udev,
+			AR9170_USB_EP_CMD), cmd, cmd->hdr.len + 4,
+			carl9170_usb_cmd_complete, ar, 1);
 
 	if (free_buf)
 		urb->transfer_flags |= URB_FREE_BUFFER;
@@ -1032,9 +1037,10 @@ static void carl9170_usb_firmware_step2(const struct firmware *fw,
 static int carl9170_usb_probe(struct usb_interface *intf,
 			      const struct usb_device_id *id)
 {
+	struct usb_endpoint_descriptor *ep;
 	struct ar9170 *ar;
 	struct usb_device *udev;
-	int err;
+	int i, err;
 
 	err = usb_reset_device(interface_to_usbdev(intf));
 	if (err)
@@ -1050,6 +1056,19 @@ 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) == 4)
+			ar->usb_ep4_is_bulk = true;
+	}
+
 	usb_set_intfdata(intf, ar);
 	SET_IEEE80211_DEV(ar->hw, &intf->dev);
 
-- 
1.9.3

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

[Ronald Wahl]

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;
> +	}

Hi,

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

- ron

-- 
Ronald Wahl - ronald.wahl@raritan.com - Phone +49 375271349-0 Fax -99
Raritan Deutschland GmbH, Kornmarkt 7, 08056 Zwickau, Germany
USt-IdNr. DE813094160, Steuer-Nr. 227/117/01749
Amtsgericht Chemnitz HRB 23605
Geschäftsführung: Stuart Hopper, Ralf Ploenes

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

[Ronald Wahl]

On 07.08.2014 11:33, 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;
>> +    }
>
> Hi,
>
> 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...

A fellow just pointed out that a central part of my patch was wrong as I 
actually forgot to check the endpoint type and instead hardcoded the 
result. So I had to create v2 of the patch now.

Mea culpa,
ron

-- 
Ronald Wahl - ronald.wahl@raritan.com - Phone +49 375271349-0 Fax -99
Raritan Deutschland GmbH, Kornmarkt 7, 08056 Zwickau, Germany
USt-IdNr. DE813094160, Steuer-Nr. 227/117/01749
Amtsgericht Chemnitz HRB 23605
Geschäftsführung: Stuart Hopper, Ralf Ploenes

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

[Christian Lamparter]

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

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

[Ronald Wahl]

On 07.08.2014 12:51, Christian Lamparter wrote:
> 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)

Yes, it actually works now. Before this change you could crash the 
system because of the constant logging of the warnings it was almost 
dead. My embedded device just rebooted after a while (maybe the watchdog 
triggered).

>> @@ -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.)

This is what my first patch has done but we need to check the endpoint 
type if its bulk or not. Otherwise it will fail in the high-speed case.
Or did you mean:

		if (usb_endpoint_num(ep) == AR9170_USB_EP_CMD &&
		    usb_endpoint_is_bulk_out(ep) &&
		    usb_endpoint_type(ep) == USB_ENDPOINT_XFER_BULK)
			ar->usb_ep_cmd_is_bulk = true;
?

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

great!

I will fix the remaining things you mentioned.

thx,
ron

-- 
Ronald Wahl - ronald.wahl@raritan.com - Phone +49 375271349-0 Fax -99
Raritan Deutschland GmbH, Kornmarkt 7, 08056 Zwickau, Germany
USt-IdNr. DE813094160, Steuer-Nr. 227/117/01749
Amtsgericht Chemnitz HRB 23605
Geschäftsführung: Stuart Hopper, Ralf Ploenes

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

[Christian Lamparter]

On Thursday, August 07, 2014 01:42:28 PM Ronald Wahl wrote:
> On 07.08.2014 12:51, Christian Lamparter wrote:
> > 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)
> 
> Yes, it actually works now. Before this change you could crash the 
> system because of the constant logging of the warnings it was almost 
> dead. My embedded device just rebooted after a while (maybe the
> watchdog triggered).

Ok, this is important then ;).

> >> @@ -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.)
> 
> This is what my first patch has done but we need to check the endpoint 
> type if its bulk or not. Otherwise it will fail in the high-speed case.
> Or did you mean:
> 
> 		if (usb_endpoint_num(ep) == AR9170_USB_EP_CMD &&
> 		    usb_endpoint_is_bulk_out(ep) &&
> 		    usb_endpoint_type(ep) == USB_ENDPOINT_XFER_BULK)
> 			ar->usb_ep_cmd_is_bulk = true;
usb_endpoint_is_bulk_out [0] "checks if the endpoint is bulk OUT". 
This function should perform both checks (ie.: is bulk? is out?).

> > BTW: I'll make a patch to carl9170 firmware too.
> 
> great!
> 
> I will fix the remaining things you mentioned.

great!

Regards

Christian

[0] http://lxr.free-electrons.com/source/include/uapi/linux/usb/ch9.h#L539

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

[Ronald Wahl]

On 07.08.2014 15:04, Christian Lamparter wrote:
> On Thursday, August 07, 2014 01:42:28 PM Ronald Wahl wrote:
>>> 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.)
>>
>> This is what my first patch has done but we need to check the endpoint
>> type if its bulk or not. Otherwise it will fail in the high-speed case.
>> Or did you mean:
>>
>> 		if (usb_endpoint_num(ep) == AR9170_USB_EP_CMD &&
>> 		    usb_endpoint_is_bulk_out(ep) &&
>> 		    usb_endpoint_type(ep) == USB_ENDPOINT_XFER_BULK)
>> 			ar->usb_ep_cmd_is_bulk = true;
> usb_endpoint_is_bulk_out [0] "checks if the endpoint is bulk OUT".
> This function should perform both checks (ie.: is bulk? is out?).

Oh, I overlooked the _bulk_ in the function name. :-)

Anyway, the v3 patch that I have send out in the meantime did not have 
this minor optimization but this shouldn't actually really matter I think.

- ron

-- 
Ronald Wahl - ronald.wahl@raritan.com - Phone +49 375271349-0 Fax -99
Raritan Deutschland GmbH, Kornmarkt 7, 08056 Zwickau, Germany
USt-IdNr. DE813094160, Steuer-Nr. 227/117/01749
Amtsgericht Chemnitz HRB 23605
Geschäftsführung: Stuart Hopper, Ralf Ploenes