diff mbox series

[v2,2/3] Bluetooth: Print broken quirks

Message ID 20220329201646.2296314-2-luiz.dentz@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v2,1/3] Bluetooth: HCI: Add HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONN quirk | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/checkpatch fail [v2,2/3] Bluetooth: Print broken quirks\WARNING:SPLIT_STRING: quoted string split across lines #113: FILE: net/bluetooth/hci_sync.c:3842: + "HCI Delete Stored Link Key command is advertised, " + "but not supported."), WARNING:SPLIT_STRING: quoted string split across lines #116: FILE: net/bluetooth/hci_sync.c:3845: + "HCI Read Default Erroneous Data Reporting command is " + "advertised, but not supported."), WARNING:SPLIT_STRING: quoted string split across lines #119: FILE: net/bluetooth/hci_sync.c:3848: + "HCI Read Transmit Power Level command is advertised, " + "but not supported."), WARNING:SPLIT_STRING: quoted string split across lines #124: FILE: net/bluetooth/hci_sync.c:3853: + "HCI Enhanced Setup Synchronous Connection command is " + "advertised, but not supported.") total: 0 errors, 4 warnings, 0 checks, 78 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/12795219.patch has style problems, please review. NOTE: Ignored message types: UNKNOWN_COMMIT_ID NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS.
tedd_an/gitlint success Gitlint PASS
tedd_an/subjectprefix success PASS

Commit Message

Luiz Augusto von Dentz March 29, 2022, 8:16 p.m. UTC 
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(-)

Comments

Paul Menzel March 30, 2022, 6:13 a.m. UTC | #1 
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
Marcel Holtmann March 30, 2022, 11:51 a.m. UTC | #2 
Hi Luiz,

> 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, \
> +}
> +

#define HCI_QUIRK_BROKEN(_quirk, _desc) { .quirk = _quirk, .desc = _desc }

Doesn’t this fit into a single line?

> +static const struct hci_quirk_broken {
> +	unsigned long quirk;
> +	const char *desc;

I am not sure if we better use an anonymous table here.

static const struct {
	..
} hci_broken_table[] = {
	..
};

> +} hci_broken_table[] = {
> +	HCI_QUIRK_BROKEN(HCI_QUIRK_BROKEN_LOCAL_COMMANDS,
> +			 "HCI Read Local Supported Commands not supported"),

I am after this:

	HCI_QUIRK_BROKEN(LOCAL_COMMANDS, “HCI Read Local Supported ..”),


> +	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;
> +
> +	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);
> +	}

	for (i = 0; .. ) }
		if (!test_bit(hci_broken_table[i], &hdev->quirks))
			continue;

		bt_dev_warn(hdev, “%s”, hci_broken_table[i].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);

Please don’t create spaghetti code. Just add the print of the quirks here.

In addition you also need to print the quirks even if no hdev->setup is defined. Quirks can also be set on probe() without the need of a vendor specific setup function.

> 
> 		/* The transport driver can set the quirk to mark the
> 		 * BD_ADDR invalid before creating the HCI device or in

Regards

Marcel
Luiz Augusto von Dentz March 30, 2022, 6 p.m. UTC | #3 
Hi Paul,

On Tue, Mar 29, 2022 at 11:13 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> 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.

We do print out the firmware filename when loading it so I'm not
convinced printing it again would make any difference for the user.

> > +     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.

I suppose size_t would be better given that sizeof is used.

>
> 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
diff mbox series

Patch

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"),
+	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;
+
+	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