[PATCH] Bluetooth: ath3k: workaround the compatibility issue with xHCI controller

[PATCH] Bluetooth: ath3k: workaround the compatibility issue with xHCI controller

[Adam Lee]

BugLink: https://bugs.launchpad.net/bugs/1400215

ath3k devices fail to load firmwares on xHCI buses, but work well on
EHCI, this might be a compatibility issue between xHCI and ath3k chips.
As my testing result, those chips will work on xHCI buses again with
this patch.

This workaround is from Qualcomm, they also did some workarounds in
Windows driver.

Signed-off-by: Adam Lee <adam.lee@canonical.com>
---
 drivers/bluetooth/ath3k.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/bluetooth/ath3k.c b/drivers/bluetooth/ath3k.c
index 1ee27ac..98ae0b1 100644
--- a/drivers/bluetooth/ath3k.c
+++ b/drivers/bluetooth/ath3k.c
@@ -174,6 +174,7 @@ static const struct usb_device_id ath3k_blist_tbl[] = {
 #define USB_REQ_DFU_DNLOAD	1
 #define BULK_SIZE		4096
 #define FW_HDR_SIZE		20
+#define TIMEGAP_US		100
 
 static int ath3k_load_firmware(struct usb_device *udev,
 				const struct firmware *firmware)
@@ -205,6 +206,9 @@ static int ath3k_load_firmware(struct usb_device *udev,
 	pipe = usb_sndbulkpipe(udev, 0x02);
 
 	while (count) {
+		/* workaround the compatibility issue with xHCI controller*/
+		usleep_range(TIMEGAP_US - 50, TIMEGAP_US);
+
 		size = min_t(uint, count, BULK_SIZE);
 		memcpy(send_buf, firmware->data + sent, size);
 
@@ -302,6 +306,9 @@ static int ath3k_load_fwfile(struct usb_device *udev,
 	pipe = usb_sndbulkpipe(udev, 0x02);
 
 	while (count) {
+		/* workaround the compatibility issue with xHCI controller*/
+		usleep_range(TIMEGAP_US - 50, TIMEGAP_US);
+
 		size = min_t(uint, count, BULK_SIZE);
 		memcpy(send_buf, firmware->data + sent, size);
 
-- 
2.1.4

Re: [PATCH] Bluetooth: ath3k: workaround the compatibility issue with xHCI controller

[Marcel Holtmann]

Hi Adam,

> BugLink: https://bugs.launchpad.net/bugs/1400215
> 
> ath3k devices fail to load firmwares on xHCI buses, but work well on
> EHCI, this might be a compatibility issue between xHCI and ath3k chips.
> As my testing result, those chips will work on xHCI buses again with
> this patch.
> 
> This workaround is from Qualcomm, they also did some workarounds in
> Windows driver.
> 
> Signed-off-by: Adam Lee <adam.lee@canonical.com>
> ---
> drivers/bluetooth/ath3k.c | 7 +++++++
> 1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/bluetooth/ath3k.c b/drivers/bluetooth/ath3k.c
> index 1ee27ac..98ae0b1 100644
> --- a/drivers/bluetooth/ath3k.c
> +++ b/drivers/bluetooth/ath3k.c
> @@ -174,6 +174,7 @@ static const struct usb_device_id ath3k_blist_tbl[] = {
> #define USB_REQ_DFU_DNLOAD	1
> #define BULK_SIZE		4096
> #define FW_HDR_SIZE		20
> +#define TIMEGAP_US		100
> 
> static int ath3k_load_firmware(struct usb_device *udev,
> 				const struct firmware *firmware)
> @@ -205,6 +206,9 @@ static int ath3k_load_firmware(struct usb_device *udev,
> 	pipe = usb_sndbulkpipe(udev, 0x02);
> 
> 	while (count) {
> +		/* workaround the compatibility issue with xHCI controller*/
> +		usleep_range(TIMEGAP_US - 50, TIMEGAP_US);
> +

why are you using a usleep_range() here. Why not just sleep for 100us.

Regards

Marcel

Re: [PATCH] Bluetooth: ath3k: workaround the compatibility issue with xHCI controller

[Adam Lee]

On Wed, Jan 28, 2015 at 12:10:21PM -0800, Marcel Holtmann wrote:
> Hi Adam,
> ...
> > --- a/drivers/bluetooth/ath3k.c
> > +++ b/drivers/bluetooth/ath3k.c
> > @@ -174,6 +174,7 @@ static const struct usb_device_id ath3k_blist_tbl[] = {
> > #define USB_REQ_DFU_DNLOAD	1
> > #define BULK_SIZE		4096
> > #define FW_HDR_SIZE		20
> > +#define TIMEGAP_US		100
> > 
> > static int ath3k_load_firmware(struct usb_device *udev,
> > 				const struct firmware *firmware)
> > @@ -205,6 +206,9 @@ static int ath3k_load_firmware(struct usb_device *udev,
> > 	pipe = usb_sndbulkpipe(udev, 0x02);
> > 
> > 	while (count) {
> > +		/* workaround the compatibility issue with xHCI controller*/
> > +		usleep_range(TIMEGAP_US - 50, TIMEGAP_US);
> > +
> 
> why are you using a usleep_range() here. Why not just sleep for 100us.
> 
> Regards
> 
> Marcel

Yes, I intended to do that, but timer is not that simple, check this:

https://www.kernel.org/doc/Documentation/timers/timers-howto.txt

SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
	* Use usleep_range

	- Why not msleep for (1ms - 20ms)?
		Explained originally here:
			http://lkml.org/lkml/2007/8/3/250
		msleep(1~20) may not do what the caller intends, and
		will often sleep longer (~20 ms actual sleep for any
		value given in the 1~20ms range). In many cases this
		is not the desired behavior.

	- Why is there no "usleep" / What is a good range?
		Since usleep_range is built on top of hrtimers, the
		wakeup will be very precise (ish), thus a simple
		usleep function would likely introduce a large number
		of undesired interrupts.

		With the introduction of a range, the scheduler is
		free to coalesce your wakeup with any other wakeup
		that may have happened for other reasons, or at the
		worst case, fire an interrupt for your upper bound.

		The larger a range you supply, the greater a chance
		that you will not trigger an interrupt; this should
		be balanced with what is an acceptable upper bound on
		delay / performance for your specific code path. Exact
		tolerances here are very situation specific, thus it
		is left to the caller to determine a reasonable range.

-- 
Adam Lee

Re: [PATCH] Bluetooth: ath3k: workaround the compatibility issue with xHCI controller

[Marcel Holtmann]

Hi Adam,

>>> --- a/drivers/bluetooth/ath3k.c
>>> +++ b/drivers/bluetooth/ath3k.c
>>> @@ -174,6 +174,7 @@ static const struct usb_device_id ath3k_blist_tbl[] = {
>>> #define USB_REQ_DFU_DNLOAD	1
>>> #define BULK_SIZE		4096
>>> #define FW_HDR_SIZE		20
>>> +#define TIMEGAP_US		100
>>> 
>>> static int ath3k_load_firmware(struct usb_device *udev,
>>> 				const struct firmware *firmware)
>>> @@ -205,6 +206,9 @@ static int ath3k_load_firmware(struct usb_device *udev,
>>> 	pipe = usb_sndbulkpipe(udev, 0x02);
>>> 
>>> 	while (count) {
>>> +		/* workaround the compatibility issue with xHCI controller*/
>>> +		usleep_range(TIMEGAP_US - 50, TIMEGAP_US);
>>> +
>> 
>> why are you using a usleep_range() here. Why not just sleep for 100us.
>> 
>> Regards
>> 
>> Marcel
> 
> Yes, I intended to do that, but timer is not that simple, check this:
> 
> https://www.kernel.org/doc/Documentation/timers/timers-howto.txt
> 
> SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
> 	* Use usleep_range
> 
> 	- Why not msleep for (1ms - 20ms)?
> 		Explained originally here:
> 			http://lkml.org/lkml/2007/8/3/250
> 		msleep(1~20) may not do what the caller intends, and
> 		will often sleep longer (~20 ms actual sleep for any
> 		value given in the 1~20ms range). In many cases this
> 		is not the desired behavior.
> 
> 	- Why is there no "usleep" / What is a good range?
> 		Since usleep_range is built on top of hrtimers, the
> 		wakeup will be very precise (ish), thus a simple
> 		usleep function would likely introduce a large number
> 		of undesired interrupts.
> 
> 		With the introduction of a range, the scheduler is
> 		free to coalesce your wakeup with any other wakeup
> 		that may have happened for other reasons, or at the
> 		worst case, fire an interrupt for your upper bound.
> 
> 		The larger a range you supply, the greater a chance
> 		that you will not trigger an interrupt; this should
> 		be balanced with what is an acceptable upper bound on
> 		delay / performance for your specific code path. Exact
> 		tolerances here are very situation specific, thus it
> 		is left to the caller to determine a reasonable range.

I am still curious why you specified it like this. It would expect something like this:

	usleep_range(TIMEGAP_MIN, TIMEGAP_MAX);

Btw. _US seems a bit to unclear if that is suppose to represent usec or something chip internal called US. If you want to keep it in there, then I prefer _USEC so I do not have to second guess every time I look at the code.

Otherwise this is fine. Just use two constants and not some magic range calculation.

Regards

Marcel

Re: [PATCH] Bluetooth: ath3k: workaround the compatibility issue with xHCI controller

[Adam Lee]

On Wed, Jan 28, 2015 at 12:22:51PM -0800, Marcel Holtmann wrote:
> I am still curious why you specified it like this. It would expect something like this:
> 
> 	usleep_range(TIMEGAP_MIN, TIMEGAP_MAX);
> 
> Btw. _US seems a bit to unclear if that is suppose to represent usec
> or something chip internal called US. If you want to keep it in there,
> then I prefer _USEC so I do not have to second guess every time I look
> at the code.
> 
> Otherwise this is fine. Just use two constants and not some magic
> range calculation.
> 
> Regards
> 
> Marcel

Good point, will resend, thanks.

-- 
Adam Lee