Received-SPF: softfail (google.com: domain of transitioning linux-bluetooth-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <e8c5b25a-6e4a-d958-c3c0-aa9dad797c8c@molgen.mpg.de>
Date:   Wed, 30 Mar 2022 08:13:48 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH v2 2/3] Bluetooth: Print broken quirks
Content-Language: en-US
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
References: <20220329201646.2296314-1-luiz.dentz@gmail.com>
 <20220329201646.2296314-2-luiz.dentz@gmail.com>
Cc:     linux-bluetooth@vger.kernel.org
From:   Paul Menzel <pmenzel@molgen.mpg.de>
In-Reply-To: <20220329201646.2296314-2-luiz.dentz@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Dear Luiz,


Thank you for your patch.

Maybe as commit message summary:

Warn about broken quirks

Am 29.03.22 um 22:16 schrieb Luiz Augusto von Dentz:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> This prints warnings for controllers setting broken quirks to increase
> their visibility and warn about broken controllers firmware that
> probably needs updates to behave properly.
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
>   net/bluetooth/hci_sync.c | 66 +++++++++++++++++++++++++++++++++++++---
>   1 file changed, 62 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 8f4c5698913d..8994ff1f94e6 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -3825,6 +3825,67 @@ static int hci_init_sync(struct hci_dev *hdev)
>   	return 0;
>   }
>   
> +#define HCI_QUIRK_BROKEN(_quirk, _desc) \
> +{ \
> +	.quirk = _quirk, \
> +	.desc = _desc, \
> +}
> +
> +static const struct hci_quirk_broken {
> +	unsigned long quirk;
> +	const char *desc;
> +} hci_broken_table[] = {
> +	HCI_QUIRK_BROKEN(HCI_QUIRK_BROKEN_LOCAL_COMMANDS,
> +			 "HCI Read Local Supported Commands not supported"),

The user wouldn’t know, that this is a device firmware problem. Could 
something be added to the warning, like:

Therefore, device firmware VERSION violates the spec. Please contact the 
manufacturer.

> +	HCI_QUIRK_BROKEN(HCI_QUIRK_BROKEN_STORED_LINK_KEY,
> +			 "HCI Delete Stored Link Key command is advertised, "
> +			 "but not supported."),
> +	HCI_QUIRK_BROKEN(HCI_QUIRK_BROKEN_ERR_DATA_REPORTING,
> +			 "HCI Read Default Erroneous Data Reporting command is "
> +			 "advertised, but not supported."),
> +	HCI_QUIRK_BROKEN(HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER,
> +			 "HCI Read Transmit Power Level command is advertised, "
> +			 "but not supported."),
> +	HCI_QUIRK_BROKEN(HCI_QUIRK_BROKEN_FILTER_CLEAR_ALL,
> +			 "HCI Set Event Filter command not supported."),
> +	HCI_QUIRK_BROKEN(HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONN,
> +			 "HCI Enhanced Setup Synchronous Connection command is "
> +			 "advertised, but not supported.")
> +};
> +
> +static void hci_dev_print_broken_quirks(struct hci_dev *hdev)
> +{
> +	int i;

I’d use `unsigned int`, but no idea, what the subsystem does regarding 
counting variables.


Kind regards,

Paul


> +
> +	bt_dev_dbg(hdev, "");
> +
> +	for (i = 0; i < ARRAY_SIZE(hci_broken_table); i++) {
> +		const struct hci_quirk_broken *broken = &hci_broken_table[i];
> +
> +		if (test_bit(broken->quirk, &hdev->quirks))
> +			bt_dev_warn(hdev, "%s", broken->desc);
> +	}
> +}
> +
> +static int hci_dev_setup_sync(struct hci_dev *hdev)
> +{
> +	bt_dev_dbg(hdev, "");
> +
> +	hci_sock_dev_event(hdev, HCI_DEV_SETUP);
> +
> +	if (hdev->setup) {
> +		int ret;
> +
> +		ret = hdev->setup(hdev);
> +		if (ret)
> +			return ret;
> +
> +		hci_dev_print_broken_quirks(hdev);
> +	}
> +
> +	return 0;
> +}
> +
>   int hci_dev_open_sync(struct hci_dev *hdev)
>   {
>   	int ret = 0;
> @@ -3887,10 +3948,7 @@ int hci_dev_open_sync(struct hci_dev *hdev)
>   	    test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
>   		bool invalid_bdaddr;
>   
> -		hci_sock_dev_event(hdev, HCI_DEV_SETUP);
> -
> -		if (hdev->setup)
> -			ret = hdev->setup(hdev);
> +		ret = hci_dev_setup_sync(hdev);
>   
>   		/* The transport driver can set the quirk to mark the
>   		 * BD_ADDR invalid before creating the HCI device or in