Re: [PATCH v4 1/2] Bluetooth: hci_sync: Add a new quirk to skip HCI_FLT_CLEAR_ALL

[Hans de Goede]

Hi,

On 3/7/22 21:04, Ismael Ferreras Morezuelas wrote:
> Some controllers have problems with being sent a command to clear
> all filtering. While the HCI code does not unconditionally
> send a clear-all anymore at BR/EDR setup (after the state machine
> refactor), there might be more ways of hitting these codepaths
> in the future as the kernel develops.
> 
> Cc: stable@xxxxxxxxxxxxxxx
> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
> Signed-off-by: Ismael Ferreras Morezuelas <swyterzone@xxxxxxxxx>

Thanks, the series looks good to me:

Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>

for both patches.

Regards,

Hans

> ---
>  include/net/bluetooth/hci.h | 10 ++++++++++
>  net/bluetooth/hci_sync.c    | 16 ++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 35c073d44ec5..5cb095b09a94 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -255,6 +255,16 @@ enum {
>  	 * during the hdev->setup vendor callback.
>  	 */
>  	HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER,
> +
> +	/* When this quirk is set, HCI_OP_SET_EVENT_FLT requests with
> +	 * HCI_FLT_CLEAR_ALL are ignored and event filtering is
> +	 * completely avoided. A subset of the CSR controller
> +	 * clones struggle with this and instantly lock up.
> +	 *
> +	 * Note that devices using this must (separately) disable
> +	 * runtime suspend, because event filtering takes place there.
> +	 */
> +	HCI_QUIRK_BROKEN_FILTER_CLEAR_ALL,
>  };
>  
>  /* HCI device flags */
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index e31d1150dc71..c3bdaf2de511 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -2812,6 +2812,9 @@ static int hci_set_event_filter_sync(struct hci_dev *hdev, u8 flt_type,
>  	if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED))
>  		return 0;
>  
> +	if (test_bit(HCI_QUIRK_BROKEN_FILTER_CLEAR_ALL, &hdev->quirks))
> +		return 0;
> +
>  	memset(&cp, 0, sizeof(cp));
>  	cp.flt_type = flt_type;
>  
> @@ -2832,6 +2835,13 @@ static int hci_clear_event_filter_sync(struct hci_dev *hdev)
>  	if (!hci_dev_test_flag(hdev, HCI_EVENT_FILTER_CONFIGURED))
>  		return 0;
>  
> +	/* In theory the state machine should not reach here unless
> +	 * a hci_set_event_filter_sync() call succeeds, but we do
> +	 * the check both for parity and as a future reminder.
> +	 */
> +	if (test_bit(HCI_QUIRK_BROKEN_FILTER_CLEAR_ALL, &hdev->quirks))
> +		return 0;
> +
>  	return hci_set_event_filter_sync(hdev, HCI_FLT_CLEAR_ALL, 0x00,
>  					 BDADDR_ANY, 0x00);
>  }
> @@ -4831,6 +4841,12 @@ static int hci_update_event_filter_sync(struct hci_dev *hdev)
>  	if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED))
>  		return 0;
>  
> +	/* Some fake CSR controllers lock up after setting this type of
> +	 * filter, so avoid sending the request altogether.
> +	 */
> +	if (test_bit(HCI_QUIRK_BROKEN_FILTER_CLEAR_ALL, &hdev->quirks))
> +		return 0;
> +
>  	/* Always clear event filter when starting */
>  	hci_clear_event_filter_sync(hdev);
>