[BlueZ,1/9] Enable alternate Bluetooth connection modes

From: Vicki Pfau <vi@endrift.com>

This patch improves Bluetooth connectivity, especially with multiple
controllers and while docked.

Testing:
$ btmgmt
[mgmt]# phy

Verify the SupportedPHYs in main.conf are listed.
Verify that multiple controllers can connect and work well.

Co-Authored-By: Rachel Blackman <rachel.blackman@synapse.com>

[Emil Velikov]
Remove unused function, add default entries into parser, keep only
default entries in main.conf - commented out, like the other options.
---
 src/adapter.c | 46 +++++++++++++++++++++++++++++++++++++++++
 src/btd.h     |  2 ++
 src/main.c    | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/main.conf |  5 +++++
 4 files changed, 119 insertions(+)

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=819667

---Test result---

Test Summary:
CheckPatch                    FAIL      4.59 seconds
GitLint                       PASS      2.64 seconds
BuildEll                      PASS      24.25 seconds
BluezMake                     FAIL      4.89 seconds
MakeCheck                     FAIL      0.12 seconds
MakeDistcheck                 FAIL      4.40 seconds
CheckValgrind                 FAIL      4.38 seconds
CheckSmatch                   FAIL      4.52 seconds
bluezmakeextell               FAIL      4.42 seconds
IncrementalBuild              FAIL      4731.57 seconds
ScanBuild                     FAIL      480.27 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ,1/9] Enable alternate Bluetooth connection modes
WARNING:BAD_SIGN_OFF: Non-standard signature: Co-Authored-By:
#87: 
Co-Authored-By: Rachel Blackman <rachel.blackman@synapse.com>

WARNING:BAD_SIGN_OFF: 'Co-authored-by:' is the preferred signature form
#87: 
Co-Authored-By: Rachel Blackman <rachel.blackman@synapse.com>

WARNING:LONG_LINE: line length of 102 exceeds 80 columns
#133: FILE: src/adapter.c:869:
+								adapter->dev_id, mgmt_errstr(status));

WARNING:LONG_LINE: line length of 84 exceeds 80 columns
#146: FILE: src/adapter.c:882:
+				set_phy_support_complete, (void*)adapter, NULL) > 0)

ERROR:POINTER_LOCATION: "(foo*)" should be "(foo *)"
#146: FILE: src/adapter.c:882:
+				set_phy_support_complete, (void*)adapter, NULL) > 0)

WARNING:LONG_LINE_COMMENT: line length of 81 exceeds 80 columns
#163: FILE: src/adapter.c:10503:
+	// Some adapters do not want to accept this before being started/powered.

WARNING:STATIC_CONST_CHAR_ARRAY: static const char * array should probably be static const char * const
#199: FILE: src/main.c:186:
+static const char *conf_phys_str[] = {

/github/workspace/src/src/13529758.patch total: 1 errors, 6 warnings, 182 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/src/13529758.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[BlueZ,5/9] profiles: remove unused suspend-dummy.c
WARNING:UNKNOWN_COMMIT_ID: Unknown commit id 'fb55b7a6a', maybe rebased or not pulled?
#77: 
The file has been used for about 8 years now - see commit fb55b7a6a

/github/workspace/src/src/13529762.patch total: 0 errors, 1 warnings, 8 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/src/13529762.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: BluezMake - FAIL
Desc: Build BlueZ
Output:

configure.ac:21: installing './compile'
configure.ac:36: installing './config.guess'
configure.ac:36: installing './config.sub'
configure.ac:5: installing './install-sh'
configure.ac:5: installing './missing'
Makefile.am: installing './depcomp'
parallel-tests: installing './test-driver'
./configure: line 15145: syntax error: unexpected end of file
##############################
Test: MakeCheck - FAIL
Desc: Run Bluez Make Check
Output:

make: *** No rule to make target 'check'.  Stop.
##############################
Test: MakeDistcheck - FAIL
Desc: Run Bluez Make Distcheck
Output:

configure.ac:21: installing './compile'
configure.ac:5: installing './missing'
Makefile.am: installing './depcomp'
./configure: line 15145: syntax error: unexpected end of file
##############################
Test: CheckValgrind - FAIL
Desc: Run Bluez Make Check with Valgrind
Output:

configure.ac:21: installing './compile'
configure.ac:5: installing './missing'
Makefile.am: installing './depcomp'
./configure: line 15145: syntax error: unexpected end of file
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:

configure.ac:21: installing './compile'
configure.ac:5: installing './missing'
Makefile.am: installing './depcomp'
./configure: line 15145: syntax error: unexpected end of file
##############################
Test: bluezmakeextell - FAIL
Desc: Build Bluez with External ELL
Output:

configure.ac:21: installing './compile'
configure.ac:5: installing './missing'
Makefile.am: installing './depcomp'
./configure: line 15145: syntax error: unexpected end of file
##############################
Test: IncrementalBuild - FAIL
Desc: Incremental build with the patches in the series
Output:
[BlueZ,8/9] obex: remove phonebook tracker backend

./configure: line 15145: syntax error: unexpected end of file
make: *** [Makefile:4713: config.status] Error 2
configure.ac:21: installing './compile'
configure.ac:5: installing './missing'
Makefile.am: installing './depcomp'
./configure: line 15145: syntax error: unexpected end of file
##############################
Test: ScanBuild - FAIL
Desc: Run Scan Build
Output:

configure.ac:21: installing './compile'
configure.ac:36: installing './config.guess'
configure.ac:36: installing './config.sub'
configure.ac:5: installing './install-sh'
configure.ac:5: installing './missing'
Makefile.am: installing './depcomp'
parallel-tests: installing './test-driver'
./configure: line 15145: syntax error: unexpected end of file


---
Regards,
Linux Bluetooth

Hi Emil,

I didn't write this patch. It was written by Rachel Blackman, and I believe I just rebased it onto our local tree with the expectation that it was just our local tree. It would be better attributed to her, potentially with a Co-Authored-By for me.

Vicki

On 1/24/24 15:43, Emil Velikov via B4 Relay wrote:
> From: Vicki Pfau <vi@endrift.com>
> 
> This patch improves Bluetooth connectivity, especially with multiple
> controllers and while docked.
> 
> Testing:
> $ btmgmt
> [mgmt]# phy
> 
> Verify the SupportedPHYs in main.conf are listed.
> Verify that multiple controllers can connect and work well.
> 
> Co-Authored-By: Rachel Blackman <rachel.blackman@synapse.com>
> 
> [Emil Velikov]
> Remove unused function, add default entries into parser, keep only
> default entries in main.conf - commented out, like the other options.
> ---
>  src/adapter.c | 46 +++++++++++++++++++++++++++++++++++++++++
>  src/btd.h     |  2 ++
>  src/main.c    | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/main.conf |  5 +++++
>  4 files changed, 119 insertions(+)
> 
> diff --git a/src/adapter.c b/src/adapter.c
> index 022390f0d..4c6b8f40f 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -86,6 +86,18 @@
>  #define DISTANCE_VAL_INVALID	0x7FFF
>  #define PATHLOSS_MAX		137
>  
> +#define LE_PHY_1M 0x01
> +#define LE_PHY_2M 0x02
> +#define LE_PHY_CODED 0x04
> +
> +#define PHYVAL_REQUIRED 0x07ff
> +#define PHYVAL_1M_TX (1<<9)
> +#define PHYVAL_1M_RX (1<<10)
> +#define PHYVAL_2M_TX (1<<11)
> +#define PHYVAL_2M_RX (1<<12)
> +#define PHYVAL_CODED_TX (1<<13)
> +#define PHYVAL_CODED_RX (1<<14)
> +
>  /*
>   * These are known security keys that have been compromised.
>   * If this grows or there are needs to be platform specific, it is
> @@ -847,6 +859,36 @@ static bool set_discoverable(struct btd_adapter *adapter, uint8_t mode,
>  	return false;
>  }
>  
> +static void set_phy_support_complete(uint8_t status, uint16_t length,
> +					const void *param, void *user_data)
> +{
> +	if (status != 0) {
> +		struct btd_adapter *adapter = (struct btd_adapter *)user_data;
> +
> +		btd_error(adapter->dev_id, "PHY setting rejected for %u: %s",
> +								adapter->dev_id, mgmt_errstr(status));
> +	}
> +}
> +
> +static bool set_phy_support(struct btd_adapter *adapter, uint32_t phy_mask)
> +{
> +	struct mgmt_cp_set_phy_confguration cp;
> +
> +	memset(&cp, 0, sizeof(cp));
> +	cp.selected_phys = cpu_to_le32(phy_mask | PHYVAL_REQUIRED);
> +
> +	if (mgmt_send(adapter->mgmt, MGMT_OP_SET_PHY_CONFIGURATION,
> +				adapter->dev_id, sizeof(cp), &cp,
> +				set_phy_support_complete, (void*)adapter, NULL) > 0)
> +		return true;
> +
> +	btd_error(adapter->dev_id, "Failed to set PHY for index %u",
> +							adapter->dev_id);
> +
> +	return false;
> +
> +}
> +
>  static bool pairable_timeout_handler(gpointer user_data)
>  {
>  	struct btd_adapter *adapter = user_data;
> @@ -10458,6 +10500,10 @@ static void read_info_complete(uint8_t status, uint16_t length,
>  	if (btd_adapter_get_powered(adapter))
>  		adapter_start(adapter);
>  
> +	// Some adapters do not want to accept this before being started/powered.
> +	if (btd_opts.phys > 0)
> +		set_phy_support(adapter, btd_opts.phys);
> +
>  	return;
>  
>  failed:
> diff --git a/src/btd.h b/src/btd.h
> index b7e7ebd61..2b84f7a51 100644
> --- a/src/btd.h
> +++ b/src/btd.h
> @@ -151,6 +151,8 @@ struct btd_opts {
>  	struct btd_advmon_opts	advmon;
>  
>  	struct btd_csis csis;
> +
> +	uint32_t	phys;
>  };
>  
>  extern struct btd_opts btd_opts;
> diff --git a/src/main.c b/src/main.c
> index b1339c230..faedb853c 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -128,6 +128,7 @@ static const char *le_options[] = {
>  	"AdvMonAllowlistScanDuration",
>  	"AdvMonNoFilterScanDuration",
>  	"EnableAdvMonInterleaveScan",
> +	"SupportedPHYs",
>  	NULL
>  };
>  
> @@ -182,10 +183,32 @@ static const struct group_table {
>  	{ }
>  };
>  
> +static const char *conf_phys_str[] = {
> +	"BR1M1SLOT",
> +	"BR1M3SLOT",
> +	"BR1M5SLOT",
> +	"EDR2M1SLOT",
> +	"EDR2M3SLOT",
> +	"EDR2M5SLOT",
> +	"EDR3M1SLOT",
> +	"EDR3M3SLOT",
> +	"EDR3M5SLOT",
> +	"LE1MTX",
> +	"LE1MRX",
> +	"LE2MTX",
> +	"LE2MRX",
> +	"LECODEDTX",
> +	"LECODEDRX",
> +};
> +
>  #ifndef MIN
>  #define MIN(x, y) ((x) < (y) ? (x) : (y))
>  #endif
>  
> +#ifndef NELEM
> +#define NELEM(x) (sizeof(x) / sizeof((x)[0]))
> +#endif
> +
>  static int8_t check_sirk_alpha_numeric(char *str)
>  {
>  	int8_t val = 0;
> @@ -226,6 +249,36 @@ static size_t hex2bin(const char *hexstr, uint8_t *buf, size_t buflen)
>  	return len;
>  }
>  
> +static bool str2phy(const char *phy_str, uint32_t *phy_val)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < NELEM(conf_phys_str); i++) {
> +		if (strcasecmp(conf_phys_str[i], phy_str) == 0) {
> +			*phy_val = (1 << i);
> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}
> +
> +static void btd_parse_phy_list(char **list)
> +{
> +	uint32_t phys = 0;
> +
> +	for (int i = 0; list[i]; i++) {
> +		uint32_t phy_val;
> +
> +		info("Enabling PHY option: %s", list[i]);
> +
> +		if (str2phy(list[i], &phy_val))
> +			phys |= phy_val;
> +	}
> +
> +	btd_opts.phys = phys;
> +}
> +
>  GKeyFile *btd_get_main_conf(void)
>  {
>  	return main_conf;
> @@ -673,11 +726,24 @@ static void parse_le_config(GKeyFile *config)
>  		  0,
>  		  1},
>  	};
> +	char **strlist;
> +	GError *err = NULL;
>  
>  	if (btd_opts.mode == BT_MODE_BREDR)
>  		return;
>  
>  	parse_mode_config(config, "LE", params, ARRAY_SIZE(params));
> +
> +	strlist = g_key_file_get_string_list(config, "LE", "SupportedPHYs",
> +						NULL, &err);
> +	if (err) {
> +		g_clear_error(&err);
> +		strlist = g_new0(char *, 3);
> +		strlist[0] = g_strdup("LE1MTX");
> +		strlist[1] = g_strdup("LE1MRX");
> +	}
> +	btd_parse_phy_list(strlist);
> +	g_strfreev(strlist);
>  }
>  
>  static bool match_experimental(const void *data, const void *match_data)
> diff --git a/src/main.conf b/src/main.conf
> index 085c81a46..59d31e494 100644
> --- a/src/main.conf
> +++ b/src/main.conf
> @@ -231,6 +231,11 @@
>  # Defaults to 1
>  #EnableAdvMonInterleaveScan=
>  
> +# Which Bluetooth LE PHYs should be enabled/supported?
> +# Options are LE1MTX LE1MRX LE2MTX LE2MRX LECODEDTX LECODEDRX
> +# Defaults to LE1MTX,LE1MRX
> +#SupportedPHYs=LE1MTX,LE1MRX
> +
>  [GATT]
>  # GATT attribute cache.
>  # Possible values:
>

Hi Emil,

On Wed, Jan 24, 2024 at 6:46 PM Emil Velikov via B4 Relay
<devnull+emil.l.velikov.gmail.com@kernel.org> wrote:
>
> From: Vicki Pfau <vi@endrift.com>
>
> This patch improves Bluetooth connectivity, especially with multiple
> controllers and while docked.
>
> Testing:
> $ btmgmt
> [mgmt]# phy
>
> Verify the SupportedPHYs in main.conf are listed.
> Verify that multiple controllers can connect and work well.
>
> Co-Authored-By: Rachel Blackman <rachel.blackman@synapse.com>
>
> [Emil Velikov]
> Remove unused function, add default entries into parser, keep only
> default entries in main.conf - commented out, like the other options.
> ---
>  src/adapter.c | 46 +++++++++++++++++++++++++++++++++++++++++
>  src/btd.h     |  2 ++
>  src/main.c    | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/main.conf |  5 +++++
>  4 files changed, 119 insertions(+)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 022390f0d..4c6b8f40f 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -86,6 +86,18 @@
>  #define DISTANCE_VAL_INVALID   0x7FFF
>  #define PATHLOSS_MAX           137
>
> +#define LE_PHY_1M 0x01
> +#define LE_PHY_2M 0x02
> +#define LE_PHY_CODED 0x04
> +
> +#define PHYVAL_REQUIRED 0x07ff
> +#define PHYVAL_1M_TX (1<<9)
> +#define PHYVAL_1M_RX (1<<10)
> +#define PHYVAL_2M_TX (1<<11)
> +#define PHYVAL_2M_RX (1<<12)
> +#define PHYVAL_CODED_TX (1<<13)
> +#define PHYVAL_CODED_RX (1<<14)
> +
>  /*
>   * These are known security keys that have been compromised.
>   * If this grows or there are needs to be platform specific, it is
> @@ -847,6 +859,36 @@ static bool set_discoverable(struct btd_adapter *adapter, uint8_t mode,
>         return false;
>  }
>
> +static void set_phy_support_complete(uint8_t status, uint16_t length,
> +                                       const void *param, void *user_data)
> +{
> +       if (status != 0) {
> +               struct btd_adapter *adapter = (struct btd_adapter *)user_data;
> +
> +               btd_error(adapter->dev_id, "PHY setting rejected for %u: %s",
> +                                                               adapter->dev_id, mgmt_errstr(status));
> +       }
> +}
> +
> +static bool set_phy_support(struct btd_adapter *adapter, uint32_t phy_mask)
> +{
> +       struct mgmt_cp_set_phy_confguration cp;
> +
> +       memset(&cp, 0, sizeof(cp));
> +       cp.selected_phys = cpu_to_le32(phy_mask | PHYVAL_REQUIRED);
> +
> +       if (mgmt_send(adapter->mgmt, MGMT_OP_SET_PHY_CONFIGURATION,
> +                               adapter->dev_id, sizeof(cp), &cp,
> +                               set_phy_support_complete, (void*)adapter, NULL) > 0)
> +               return true;
> +
> +       btd_error(adapter->dev_id, "Failed to set PHY for index %u",
> +                                                       adapter->dev_id);
> +
> +       return false;
> +
> +}
> +
>  static bool pairable_timeout_handler(gpointer user_data)
>  {
>         struct btd_adapter *adapter = user_data;
> @@ -10458,6 +10500,10 @@ static void read_info_complete(uint8_t status, uint16_t length,
>         if (btd_adapter_get_powered(adapter))
>                 adapter_start(adapter);
>
> +       // Some adapters do not want to accept this before being started/powered.
> +       if (btd_opts.phys > 0)
> +               set_phy_support(adapter, btd_opts.phys);
> +
>         return;
>
>  failed:
> diff --git a/src/btd.h b/src/btd.h
> index b7e7ebd61..2b84f7a51 100644
> --- a/src/btd.h
> +++ b/src/btd.h
> @@ -151,6 +151,8 @@ struct btd_opts {
>         struct btd_advmon_opts  advmon;
>
>         struct btd_csis csis;
> +
> +       uint32_t        phys;
>  };
>
>  extern struct btd_opts btd_opts;
> diff --git a/src/main.c b/src/main.c
> index b1339c230..faedb853c 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -128,6 +128,7 @@ static const char *le_options[] = {
>         "AdvMonAllowlistScanDuration",
>         "AdvMonNoFilterScanDuration",
>         "EnableAdvMonInterleaveScan",
> +       "SupportedPHYs",
>         NULL
>  };
>
> @@ -182,10 +183,32 @@ static const struct group_table {
>         { }
>  };
>
> +static const char *conf_phys_str[] = {
> +       "BR1M1SLOT",
> +       "BR1M3SLOT",
> +       "BR1M5SLOT",
> +       "EDR2M1SLOT",
> +       "EDR2M3SLOT",
> +       "EDR2M5SLOT",
> +       "EDR3M1SLOT",
> +       "EDR3M3SLOT",
> +       "EDR3M5SLOT",
> +       "LE1MTX",
> +       "LE1MRX",
> +       "LE2MTX",
> +       "LE2MRX",
> +       "LECODEDTX",
> +       "LECODEDRX",
> +};
> +
>  #ifndef MIN
>  #define MIN(x, y) ((x) < (y) ? (x) : (y))
>  #endif
>
> +#ifndef NELEM
> +#define NELEM(x) (sizeof(x) / sizeof((x)[0]))
> +#endif
> +
>  static int8_t check_sirk_alpha_numeric(char *str)
>  {
>         int8_t val = 0;
> @@ -226,6 +249,36 @@ static size_t hex2bin(const char *hexstr, uint8_t *buf, size_t buflen)
>         return len;
>  }
>
> +static bool str2phy(const char *phy_str, uint32_t *phy_val)
> +{
> +       unsigned int i;
> +
> +       for (i = 0; i < NELEM(conf_phys_str); i++) {
> +               if (strcasecmp(conf_phys_str[i], phy_str) == 0) {
> +                       *phy_val = (1 << i);
> +                       return true;
> +               }
> +       }
> +
> +       return false;
> +}
> +
> +static void btd_parse_phy_list(char **list)
> +{
> +       uint32_t phys = 0;
> +
> +       for (int i = 0; list[i]; i++) {
> +               uint32_t phy_val;
> +
> +               info("Enabling PHY option: %s", list[i]);
> +
> +               if (str2phy(list[i], &phy_val))
> +                       phys |= phy_val;
> +       }
> +
> +       btd_opts.phys = phys;
> +}
> +
>  GKeyFile *btd_get_main_conf(void)
>  {
>         return main_conf;
> @@ -673,11 +726,24 @@ static void parse_le_config(GKeyFile *config)
>                   0,
>                   1},
>         };
> +       char **strlist;
> +       GError *err = NULL;
>
>         if (btd_opts.mode == BT_MODE_BREDR)
>                 return;
>
>         parse_mode_config(config, "LE", params, ARRAY_SIZE(params));
> +
> +       strlist = g_key_file_get_string_list(config, "LE", "SupportedPHYs",
> +                                               NULL, &err);
> +       if (err) {
> +               g_clear_error(&err);
> +               strlist = g_new0(char *, 3);
> +               strlist[0] = g_strdup("LE1MTX");
> +               strlist[1] = g_strdup("LE1MRX");
> +       }
> +       btd_parse_phy_list(strlist);
> +       g_strfreev(strlist);
>  }
>
>  static bool match_experimental(const void *data, const void *match_data)
> diff --git a/src/main.conf b/src/main.conf
> index 085c81a46..59d31e494 100644
> --- a/src/main.conf
> +++ b/src/main.conf
> @@ -231,6 +231,11 @@
>  # Defaults to 1
>  #EnableAdvMonInterleaveScan=
>
> +# Which Bluetooth LE PHYs should be enabled/supported?
> +# Options are LE1MTX LE1MRX LE2MTX LE2MRX LECODEDTX LECODEDRX
> +# Defaults to LE1MTX,LE1MRX
> +#SupportedPHYs=LE1MTX,LE1MRX

I'm sort of surprised by this, we do only use the PHYs listed as
supported by the controller, so is there a bug or is this really a way
to disable PHYs that the controllers report as supported but in
reality don't really work properly? In case of the latter I think we
would be better off having a quirk added in the kernel so it can be
marked to the controllers we know misbehaves rather than limiting all
controllers to 1M PHY by default.

>  [GATT]
>  # GATT attribute cache.
>  # Possible values:
>
> --
> 2.43.0
>
>

Hi Luiz,

On Thu, 25 Jan 2024 at 03:54, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Emil,
>

>
> I'm sort of surprised by this, we do only use the PHYs listed as
> supported by the controller, so is there a bug or is this really a way
> to disable PHYs that the controllers report as supported but in
> reality don't really work properly? In case of the latter I think we
> would be better off having a quirk added in the kernel so it can be
> marked to the controllers we know misbehaves rather than limiting all
> controllers to 1M PHY by default.
>

Using pristine bluez, bluetoothctl/mgmt/phy lists (omitting the slot phys):

Supported phys:     LE1MTX LE1MRX LE2MTX LE2MRX LECODEDTX LECODEDRX
Configurable phys:                LE2MTX LE2MRX LECODEDTX LECODEDRX
Selected phys:      LE1MTX LE1MRX

With this patch + the LE/SupportedPHY config set to "LE1MTX LE1MRX
LE2MTX LE2MRX LECODEDTX LECODEDRX", as per the original patch we get.
Note: I've intentionally dropped the override for submission, happy to
bring it back if you prefer.

Supported phys:     LE1MTX LE1MRX LE2MTX LE2MRX LECODEDTX LECODEDRX
Configurable phys:                LE2MTX LE2MRX LECODEDTX LECODEDRX
Selected phys:      LE1MTX LE1MRX LE2MTX LE2MRX LECODEDTX LECODEDRX

Note: I've intentionally dropped the override for upstreaming, happy
to bring it back if you prefer.

So from what I can tell, the controller reports that all (as far as
we're concerned) PHYs are supported. Yet the selected and configurable
PHYs are mutually exclusive, which doesn't quite compute here.
Mind you, my bluetooth knowledge is a bit limited - I'm just going by the code.

What would you say is the best way to move forward with this? It
doesn't seem like a kernel quirk is needed IMHO.
Generally, if you feel that a different name and/or semantics for the
toggle would help, I'm all ears.

Thanks in advance,
Emil

Hi Vicki,

On Thu, 25 Jan 2024 at 03:05, Vicki Pfau <vi@endrift.com> wrote:
>
> Hi Emil,
>
> I didn't write this patch. It was written by Rachel Blackman, and I believe I just rebased it onto our local tree with the expectation that it was just our local tree. It would be better attributed to her, potentially with a Co-Authored-By for me.
>

Sure can do that. I've intentionally tried to preserve authorship,
which seemed to have backfired in this case :-)

-Emil

Hi Emil,

On Thu, Jan 25, 2024 at 8:39 AM Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> Hi Luiz,
>
> On Thu, 25 Jan 2024 at 03:54, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Emil,
> >
>
> >
> > I'm sort of surprised by this, we do only use the PHYs listed as
> > supported by the controller, so is there a bug or is this really a way
> > to disable PHYs that the controllers report as supported but in
> > reality don't really work properly? In case of the latter I think we
> > would be better off having a quirk added in the kernel so it can be
> > marked to the controllers we know misbehaves rather than limiting all
> > controllers to 1M PHY by default.
> >
>
> Using pristine bluez, bluetoothctl/mgmt/phy lists (omitting the slot phys):
>
> Supported phys:     LE1MTX LE1MRX LE2MTX LE2MRX LECODEDTX LECODEDRX
> Configurable phys:                LE2MTX LE2MRX LECODEDTX LECODEDRX
> Selected phys:      LE1MTX LE1MRX
>
> With this patch + the LE/SupportedPHY config set to "LE1MTX LE1MRX
> LE2MTX LE2MRX LECODEDTX LECODEDRX", as per the original patch we get.
> Note: I've intentionally dropped the override for submission, happy to
> bring it back if you prefer.
>
> Supported phys:     LE1MTX LE1MRX LE2MTX LE2MRX LECODEDTX LECODEDRX
> Configurable phys:                LE2MTX LE2MRX LECODEDTX LECODEDRX
> Selected phys:      LE1MTX LE1MRX LE2MTX LE2MRX LECODEDTX LECODEDRX
>
> Note: I've intentionally dropped the override for upstreaming, happy
> to bring it back if you prefer.
>
> So from what I can tell, the controller reports that all (as far as
> we're concerned) PHYs are supported. Yet the selected and configurable
> PHYs are mutually exclusive, which doesn't quite compute here.
> Mind you, my bluetooth knowledge is a bit limited - I'm just going by the code.
>
> What would you say is the best way to move forward with this? It
> doesn't seem like a kernel quirk is needed IMHO.
> Generally, if you feel that a different name and/or semantics for the
> toggle would help, I'm all ears.

Hmm, are you sure you are not missing something like:

commit 288c90224eec55d13e786844b7954ef060752089
Author: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Date:   Mon Dec 19 13:37:02 2022 -0800

    Bluetooth: Enable all supported LE PHY by default

    This enables 2M and Coded PHY by default if they are marked as supported
    in the LE features bits.

    Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Later one we had to introduce HCI_QUIRK_BROKEN_LE_CODED because of it,
but so far that was the only drawback.

> Thanks in advance,
> Emil

Hi Luiz,

On Thu, 25 Jan 2024 at 14:59, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:

[snip]

> Hmm, are you sure you are not missing something like:
>
> commit 288c90224eec55d13e786844b7954ef060752089
> Author: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> Date:   Mon Dec 19 13:37:02 2022 -0800
>
>     Bluetooth: Enable all supported LE PHY by default
>
>     This enables 2M and Coded PHY by default if they are marked as supported
>     in the LE features bits.
>
>     Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> Later one we had to introduce HCI_QUIRK_BROKEN_LE_CODED because of it,
> but so far that was the only drawback.
>

Hell yeah, that commit should fix our problem. Fwiw we were on the 6.1
stable tree where the above landed in 6.4. Glancing around it was not
picked for any(?) stable branches, which is why we're missing it.

Thanks a million,
Emil

Hi Emil,

On Thu, Jan 25, 2024 at 11:32 AM Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> Hi Luiz,
>
> On Thu, 25 Jan 2024 at 14:59, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>
> [snip]
>
> > Hmm, are you sure you are not missing something like:
> >
> > commit 288c90224eec55d13e786844b7954ef060752089
> > Author: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > Date:   Mon Dec 19 13:37:02 2022 -0800
> >
> >     Bluetooth: Enable all supported LE PHY by default
> >
> >     This enables 2M and Coded PHY by default if they are marked as supported
> >     in the LE features bits.
> >
> >     Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >
> > Later one we had to introduce HCI_QUIRK_BROKEN_LE_CODED because of it,
> > but so far that was the only drawback.
> >
>
> Hell yeah, that commit should fix our problem. Fwiw we were on the 6.1
> stable tree where the above landed in 6.4. Glancing around it was not
> picked for any(?) stable branches, which is why we're missing it.

I didn't tag it for stable since I was afraid something could blow up
like it did, well now at least we know that a quirk is required and
perhaps we can mark both to be backported.

> Thanks a million,
> Emil