linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Marcel Holtmann <marcel@holtmann.org>
To: Tedd Ho-Jeong An <hj.tedd.an@gmail.com>
Cc: linux-bluetooth@vger.kernel.org
Subject: Re: [PATCH v5 05/11] Bluetooth: btintel: Fix the first HCI command not work with ROM device
Date: Fri, 30 Jul 2021 08:40:01 +0200	[thread overview]
Message-ID: <C26294D2-5DD7-42B8-BFE4-D358B4BF5D66@holtmann.org> (raw)
In-Reply-To: <05fd097e82b5c50ceaba584f36b6f8855792fc69.camel@gmail.com>

Hi Tedd,

>>> The some legacy ROM controllers have a bug with the first HCI command
>>> sent to it returning number of completed commands as zero, which would
>>> stall the command processing in the Bluetooth core.
>>> 
>>> As a workaround, send HCI Rest command first which will reset the
>>> controller to fix the issue.
>>> 
>>> Signed-off-by: Tedd Ho-Jeong An <tedd.an@intel.com>
>>> ---
>>> drivers/bluetooth/btintel.c | 21 +++++++++++++++++++++
>>> drivers/bluetooth/btintel.h |  1 +
>>> drivers/bluetooth/btusb.c   | 16 ++++++++++++++--
>>> 3 files changed, 36 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
>>> index bf0ad05b80fe..65ecf2ae9a10 100644
>>> --- a/drivers/bluetooth/btintel.c
>>> +++ b/drivers/bluetooth/btintel.c
>>> @@ -1659,6 +1659,7 @@ static int btintel_legacy_rom_setup(struct hci_dev *hdev,
>>> 
>>> int btintel_setup_combined(struct hci_dev *hdev)
>>> {
>>> +	struct btintel_data *intel = hci_get_priv(hdev);
>>> 	const u8 param[1] = { 0xFF };
>>> 	struct intel_version ver;
>>> 	struct intel_version_tlv ver_tlv;
>>> @@ -1667,6 +1668,26 @@ int btintel_setup_combined(struct hci_dev *hdev)
>>> 
>>> 	BT_DBG("%s", hdev->name);
>>> 
>>> +	/* The some controllers have a bug with the first HCI command sent to it
>>> +	 * returning number of completed commands as zero. This would stall the
>>> +	 * command processing in the Bluetooth core.
>>> +	 *
>>> +	 * As a workaround, send HCI Reset command first which will reset the
>>> +	 * number of completed commands and allow normal command processing
>>> +	 * from now on.
>>> +	 */
>>> +	if (test_bit(INTEL_BROKEN_READ_VERSION, &intel->flags)) {
>>> +		skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL,
>>> +				     HCI_INIT_TIMEOUT);
>>> +		if (IS_ERR(skb)) {
>>> +			bt_dev_err(hdev,
>>> +				   "sending initial HCI reset failed (%ld)",
>>> +				   PTR_ERR(skb));
>>> +			return PTR_ERR(skb);
>>> +		}
>>> +		kfree_skb(skb);
>>> +	}
>>> +
>>> 	/* Starting from TyP device, the command parameter and response are
>>> 	 * changed even though the OCF for HCI_Intel_Read_Version command
>>> 	 * remains same. The legacy devices can handle even if the
>>> diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
>>> index df7aa30142b4..29b678364a79 100644
>>> --- a/drivers/bluetooth/btintel.h
>>> +++ b/drivers/bluetooth/btintel.h
>>> @@ -143,6 +143,7 @@ struct intel_debug_features {
>>> #define INTEL_FIRMWARE_LOADED		2
>>> #define INTEL_FIRMWARE_FAILED		3
>>> #define INTEL_BOOTING			4
>>> +#define INTEL_BROKEN_READ_VERSION	5
>>> 
>>> struct btintel_data {
>>> 	unsigned long flags;
>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>> index 8c54ab03ee63..a64473c525eb 100644
>>> --- a/drivers/bluetooth/btusb.c
>>> +++ b/drivers/bluetooth/btusb.c
>>> @@ -62,6 +62,7 @@ static struct usb_driver btusb_driver;
>>> #define BTUSB_QCA_WCN6855	0x1000000
>>> #define BTUSB_INTEL_NEWGEN	0x2000000
>>> #define BTUSB_INTEL_COMBINED	0x4000000
>>> +#define BTUSB_INTEL_BROKEN_READ_VERSION 0x8000000
>>> 
>>> static const struct usb_device_id btusb_table[] = {
>>> 	/* Generic Bluetooth USB device */
>>> @@ -376,11 +377,14 @@ static const struct usb_device_id blacklist_table[] = {
>>> 						     BTUSB_WIDEBAND_SPEECH |
>>> 						     BTUSB_VALID_LE_STATES },
>>> 	{ USB_DEVICE(0x8087, 0x07da), .driver_info = BTUSB_CSR },
>>> -	{ USB_DEVICE(0x8087, 0x07dc), .driver_info = BTUSB_INTEL_COMBINED },
>>> -	{ USB_DEVICE(0x8087, 0x0a2a), .driver_info = BTUSB_INTEL_COMBINED },
>>> +	{ USB_DEVICE(0x8087, 0x07dc), .driver_info = BTUSB_INTEL_COMBINED |
>>> +						     BTUSB_INTEL_BROKEN_READ_VERSION },
>>> +	{ USB_DEVICE(0x8087, 0x0a2a), .driver_info = BTUSB_INTEL_COMBINED |
>>> +						     BTUSB_INTEL_BROKEN_READ_VERSION },
>>> 	{ USB_DEVICE(0x8087, 0x0a2b), .driver_info = BTUSB_INTEL_NEW |
>>> 						     BTUSB_WIDEBAND_SPEECH },
>>> 	{ USB_DEVICE(0x8087, 0x0aa7), .driver_info = BTUSB_INTEL_COMBINED |
>>> +						     BTUSB_INTEL_BROKEN_READ_VERSION |
>>> 						     BTUSB_WIDEBAND_SPEECH },
>>> 	{ USB_DEVICE(0x8087, 0x0aaa), .driver_info = BTUSB_INTEL_NEW |
>>> 						     BTUSB_WIDEBAND_SPEECH |
>> 
>> can you check that all 3 have this problem? Don’t we ever produced a ROM where this is fixed?
> 
> It looks like the early version of ROM (WP2) causes the problem. StP and SdP don't have the problem.
> I will update accordingly.

then we should get away with having BTUSB_INTEL_COMBINED (which we can later rename back into BTUSB_INTEL) and one additional BTUSB_INTEL_BROKEN_INITIAL_NCMD. Everything else we can do internally in the Intel vendor specific handling.

> 
> WP2 - I only had mini-PCIe form factor card and it is broken.
> < HCI Command: Intel Read Version (0x3f|0x0005) plen 1                        #1 [hci0] 9.212217
>        Requested Type:
>          All Supported Types(0xff)
>> HCI Event: Command Complete (0x0e) plen 13                                      #2 [hci0] 9.213338
>      Intel Read Version (0x3f|0x0005) ncmd 0
>        Status: Success (0x00)
>        Hardware platform: 0x37
>        Hardware variant: 0x07
>        Hardware revision: 1.0
>        Firmware variant: 0x01
>        Firmware revision: 8.0
>        Firmware build: 2-3.2013
>        Firmware patch: 0
> 
> StP - OK. 
> < HCI Command: Intel Read Version (0x3f|0x0005) plen 1                      #3 [hci0] 108.053455
>        Requested Type:
>          All Supported Types(0xff)
>> HCI Event: Command Complete (0x0e) plen 13                                    #4 [hci0] 108.054034
>      Intel Read Version (0x3f|0x0005) ncmd 1
>        Status: Success (0x00)
>        Hardware platform: 0x37
>        Hardware variant: 0x08
>        Hardware revision: 1.0
>        Firmware variant: 0x01
>        Firmware revision: 1.0
>        Firmware build: 3-17.2014
>        Firmware patch: 0
> 
> SdP - OK.
> < HCI Command: Intel Read Version (0x3f|0x0005) plen 1                    #400 [hci0] 173.911992
>        Requested Type:
>          All Supported Types(0xff)
>> HCI Event: Command Complete (0x0e) plen 13                                  #401 [hci0] 173.912576
>      Intel Read Version (0x3f|0x0005) ncmd 1
>        Status: Success (0x00)
>        Hardware platform: 0x37
>        Hardware variant: 0x08
>        Hardware revision: 1.0
>        Firmware variant: 0x22
>        Firmware revision: 5.0
>        Firmware build: 25-20.2015
>        Firmware patch: 0

We should add a document in bluez/doc/intel-variants.txt that documents our own hardware and which variants and firmware combinations we have tested this with. It is good for revision history and if users complain or have slight variations of the SKUs.

Regards

Marcel


  reply	other threads:[~2021-07-30  6:40 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-29 18:35 [PATCH v5 00/11] Bluetooth: btintel: Refactoring setup routines Tedd Ho-Jeong An
2021-07-29 18:35 ` [PATCH v5 01/11] Bluetooth: Add support hdev to allocate private data Tedd Ho-Jeong An
2021-07-29 19:13   ` Bluetooth: btintel: Refactoring setup routines bluez.test.bot
2021-07-29 18:35 ` [PATCH v5 02/11] Bluetooth: btintel: Add combined setup and shutdown functions Tedd Ho-Jeong An
2021-07-29 19:21   ` Marcel Holtmann
2021-07-29 18:35 ` [PATCH v5 03/11] Bluetooth: btintel: Refactoring setup routine for legacy ROM sku Tedd Ho-Jeong An
2021-07-29 19:25   ` Marcel Holtmann
2021-07-29 18:35 ` [PATCH v5 04/11] Bluetooth: btintel: Add btintel data struct Tedd Ho-Jeong An
2021-07-29 19:31   ` Marcel Holtmann
2021-07-29 18:35 ` [PATCH v5 05/11] Bluetooth: btintel: Fix the first HCI command not work with ROM device Tedd Ho-Jeong An
2021-07-29 19:35   ` Marcel Holtmann
2021-07-29 21:59     ` Tedd Ho-Jeong An
2021-07-30  6:40       ` Marcel Holtmann [this message]
2021-07-29 18:35 ` [PATCH v5 06/11] Bluetooth: btintel: Fix the LED is not turning off immediately Tedd Ho-Jeong An
2021-07-29 19:37   ` Marcel Holtmann
2021-07-29 18:35 ` [PATCH v5 07/11] Bluetooth: btintel: Add combined set_diag functions Tedd Ho-Jeong An
2021-07-29 18:35 ` [PATCH v5 08/11] Bluetooth: btintel: Refactoring setup routine for legacy bootloader Tedd Ho-Jeong An
2021-07-29 19:45   ` Marcel Holtmann
2021-07-29 18:35 ` [PATCH v5 09/11] Bluetooth: btintel: Refactoring setup routine for TLV based booloader Tedd Ho-Jeong An
2021-07-29 18:35 ` [PATCH v5 10/11] Bluetooth: btintel: Clean the exported function to static Tedd Ho-Jeong An
2021-07-29 18:36 ` [PATCH v5 11/11] Bluetooth: btintel: Fix the legacy bootloader returns tlv based version Tedd Ho-Jeong An
2021-07-29 19:40   ` Marcel Holtmann
2021-07-29 20:02     ` Tedd Ho-Jeong An

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=C26294D2-5DD7-42B8-BFE4-D358B4BF5D66@holtmann.org \
    --to=marcel@holtmann.org \
    --cc=hj.tedd.an@gmail.com \
    --cc=linux-bluetooth@vger.kernel.org \
    --subject='Re: [PATCH v5 05/11] Bluetooth: btintel: Fix the first HCI command not work with ROM device' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).