Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0574f6d2-51dd-4962-40fe-9424a19cb3c7@redhat.com>
Date:   Sun, 30 Oct 2022 10:59:18 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.1
Subject: Re: [PATCH 1/3] Bluetooth: btusb: Fix Chinese CSR dongles again by
 re-adding ERR_DATA_REPORTING quirk
To:     Ismael Ferreras Morezuelas <swyterzone@gmail.com>,
        marcel@holtmann.org, johan.hedberg@gmail.com, luiz.dentz@gmail.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        luiz.von.dentz@intel.com, quic_zijuhu@quicinc.com
Cc:     linux-kernel@vger.kernel.org, linux-bluetooth@vger.kernel.org,
        netdev@vger.kernel.org
References: <20221029202454.25651-1-swyterzone@gmail.com>
Content-Language: en-US, nl
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20221029202454.25651-1-swyterzone@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Ismael,

On 10/29/22 22:24, Ismael Ferreras Morezuelas wrote:
> A patch series by a Qualcomm engineer essentially removed my
> quirk/workaround because they thought it was unnecessary.
> 
> It wasn't, and it broke everything again:
> 
> https://patchwork.kernel.org/project/netdevbpf/list/?series=661703&archive=both&state=*
> 
> He argues that the quirk is not necessary because the code should check if the dongle
> says if it's supported or not. The problem is that for these Chinese CSR
> clones they say that it would work, but it's a lie. Take a look:
> 
> = New Index: 00:00:00:00:00:00 (Primary,USB,hci0)                              [hci0] 11.272194
> = Open Index: 00:00:00:00:00:00                                                [hci0] 11.272384
> < HCI Command: Read Local Version Information (0x04|0x0001) plen 0          #1 [hci0] 11.272400
>> HCI Event: Command Complete (0x0e) plen 12                                #2
>> [hci0] 11.276039
>       Read Local Version Information (0x04|0x0001) ncmd 1
>         Status: Success (0x00)
>         HCI version: Bluetooth 5.0 (0x09) - Revision 2064 (0x0810)
>         LMP version: Bluetooth 5.0 (0x09) - Subversion 8978 (0x2312)
>         Manufacturer: Cambridge Silicon Radio (10)
> ...
> < HCI Command: Read Local Supported Features (0x04|0x0003) plen 0           #5 [hci0] 11.648370
>> HCI Event: Command Complete (0x0e) plen 68                               #12
>> [hci0] 11.668030
>       Read Local Supported Commands (0x04|0x0002) ncmd 1
>         Status: Success (0x00)
>         Commands: 163 entries
>           ...
>           Read Default Erroneous Data Reporting (Octet 18 - Bit 2)
>           Write Default Erroneous Data Reporting (Octet 18 - Bit 3)
>           ...
> ...
> < HCI Command: Read Default Erroneous Data Reporting (0x03|0x005a) plen 0  #47 [hci0] 11.748352
> = Close Index: 00:1A:7D:DA:71:XX                                               [hci0] 13.776824
> 
> So bring it back wholesale.
> 
> Fixes: 63b1a7dd3 (Bluetooth: hci_sync: Remove HCI_QUIRK_BROKEN_ERR_DATA_REPORTING)
> Fixes: e168f69008 (Bluetooth: btusb: Remove HCI_QUIRK_BROKEN_ERR_DATA_REPORTING for fake CSR)
> Fixes: 766ae2422b (Bluetooth: hci_sync: Check LMP feature bit instead of quirk)
> 
> Cc: stable@vger.kernel.org
> Cc: Zijun Hu <quic_zijuhu@quicinc.com>
> Cc: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com>
> Signed-off-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com>

Thank you very much for your continued work on making these
clones work with Linux!

The entire series looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

for the series.

Regards,

Hans


> ---
>  drivers/bluetooth/btusb.c   |  1 +
>  include/net/bluetooth/hci.h | 11 +++++++++++
>  net/bluetooth/hci_sync.c    |  9 +++++++--
>  3 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 3b269060e91f..1360b2163ec5 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -2174,6 +2174,7 @@ static int btusb_setup_csr(struct hci_dev *hdev)
>  		 * without these the controller will lock up.
>  		 */
>  		set_bit(HCI_QUIRK_BROKEN_STORED_LINK_KEY, &hdev->quirks);
> +		set_bit(HCI_QUIRK_BROKEN_ERR_DATA_REPORTING, &hdev->quirks);
>  		set_bit(HCI_QUIRK_BROKEN_FILTER_CLEAR_ALL, &hdev->quirks);
>  		set_bit(HCI_QUIRK_NO_SUSPEND_NOTIFIER, &hdev->quirks);
>  
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index e004ba04a9ae..0fe789f6a653 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -228,6 +228,17 @@ enum {
>  	 */
>  	HCI_QUIRK_VALID_LE_STATES,
>  
> +	/* When this quirk is set, then erroneous data reporting
> +	 * is ignored. This is mainly due to the fact that the HCI
> +	 * Read Default Erroneous Data Reporting command is advertised,
> +	 * but not supported; these controllers often reply with unknown
> +	 * command and tend to lock up randomly. Needing a hard reset.
> +	 *
> +	 * This quirk can be set before hci_register_dev is called or
> +	 * during the hdev->setup vendor callback.
> +	 */
> +	HCI_QUIRK_BROKEN_ERR_DATA_REPORTING,
> +
>  	/*
>  	 * When this quirk is set, then the hci_suspend_notifier is not
>  	 * registered. This is intended for devices which drop completely
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index bd9eb713b26b..0a7abc817f10 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -3798,7 +3798,8 @@ static int hci_read_page_scan_activity_sync(struct hci_dev *hdev)
>  static int hci_read_def_err_data_reporting_sync(struct hci_dev *hdev)
>  {
>  	if (!(hdev->commands[18] & 0x04) ||
> -	    !(hdev->features[0][6] & LMP_ERR_DATA_REPORTING))
> +	    !(hdev->features[0][6] & LMP_ERR_DATA_REPORTING) ||
> +	    test_bit(HCI_QUIRK_BROKEN_ERR_DATA_REPORTING, &hdev->quirks))
>  		return 0;
>  
>  	return __hci_cmd_sync_status(hdev, HCI_OP_READ_DEF_ERR_DATA_REPORTING,
> @@ -4316,7 +4317,8 @@ static int hci_set_err_data_report_sync(struct hci_dev *hdev)
>  	bool enabled = hci_dev_test_flag(hdev, HCI_WIDEBAND_SPEECH_ENABLED);
>  
>  	if (!(hdev->commands[18] & 0x08) ||
> -	    !(hdev->features[0][6] & LMP_ERR_DATA_REPORTING))
> +	    !(hdev->features[0][6] & LMP_ERR_DATA_REPORTING) ||
> +	    test_bit(HCI_QUIRK_BROKEN_ERR_DATA_REPORTING, &hdev->quirks))
>  		return 0;
>  
>  	if (enabled == hdev->err_data_reporting)
> @@ -4475,6 +4477,9 @@ static const struct {
>  	HCI_QUIRK_BROKEN(STORED_LINK_KEY,
>  			 "HCI Delete Stored Link Key command is advertised, "
>  			 "but not supported."),
> +	HCI_QUIRK_BROKEN(ERR_DATA_REPORTING,
> +			 "HCI Read Default Erroneous Data Reporting command is "
> +			 "advertised, but not supported."),
>  	HCI_QUIRK_BROKEN(READ_TRANSMIT_POWER,
>  			 "HCI Read Transmit Power Level command is advertised, "
>  			 "but not supported."),