[PATCH v3 0/3] bluetooth:Adding driver and quirk defs for multi-role LE

[PATCH v3 0/3] bluetooth:Adding driver and quirk defs for multi-role LE

[Alain Michaud]

This series adds BTUSB and quirk support for the driver to confirm that
the reported LE_states can be trusted.  The quirk will be used to
gradually roll out the feature to supported controllers without
affecting the stability of other controllers.  If all controllers FWs
are fixed or else validated, we can consider removing the quirk over
time.

Alain Michaud (3):
  bluetooth:Adding driver and quirk defs for multi-role LE
  bluetooth:allow scatternet connections if supported.
  bluetooth:btusb: Adding support for LE scatternet to Jfp and ThP

 drivers/bluetooth/btusb.c   | 11 ++++++++---
 include/net/bluetooth/hci.h |  9 +++++++++
 net/bluetooth/hci_event.c   |  4 +++-
 3 files changed, 20 insertions(+), 4 deletions(-)

-- 
2.26.1.301.g55bc3eb7cb9-goog

[PATCH v3 1/3] bluetooth:Adding driver and quirk defs for multi-role LE

[Alain Michaud]

This change adds the relevant driver and quirk to allow drivers to
report the le_states as being trustworthy.

This has historically been disabled as controllers did not reliably
support this. In particular, this will be used to relax this condition
for controllers that have been well tested and reliable.

	/* Most controller will fail if we try to create new connections
	 * while we have an existing one in slave role.
	 */
	if (hdev->conn_hash.le_num_slave > 0)
		return NULL;

Signed-off-by: Alain Michaud <alainm@chromium.org>
---

 drivers/bluetooth/btusb.c   | 2 +-
 include/net/bluetooth/hci.h | 9 +++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 3bdec42c9612..dd27e28d4601 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -58,7 +58,7 @@ static struct usb_driver btusb_driver;
 #define BTUSB_CW6622		0x100000
 #define BTUSB_MEDIATEK		0x200000
 #define BTUSB_WIDEBAND_SPEECH	0x400000
-
+#define BTUSB_VALID_LE_STATES   0x800000
 static const struct usb_device_id btusb_table[] = {
 	/* Generic Bluetooth USB device */
 	{ USB_DEVICE_INFO(0xe0, 0x01, 0x01) },
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 5f60e135aeb6..25c2e5ee81dc 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -214,6 +214,15 @@ enum {
 	 * This quirk must be set before hci_register_dev is called.
 	 */
 	HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED,
+
+	/* When this quirk is set, the controller has validated that
+	 * LE states reported through the HCI_LE_READ_SUPPORTED_STATES are
+	 * valid.  This mechanism is necessary as many controllers have
+	 * been seen has having trouble initiating a connectable
+	 * advertisement despite the state combination being reported as
+	 * supported.
+	 */
+	HCI_QUIRK_VALID_LE_STATES,
 };
 
 /* HCI device flags */
-- 
2.26.1.301.g55bc3eb7cb9-goog

[PATCH v3 2/3] bluetooth:allow scatternet connections if supported.

[Alain Michaud]

This change allows scatternet connections to be created if the
controller reports support and the HCI_QUIRK_VALID_LE_STATES indicates
that the reported LE states can be trusted.

Signed-off-by: Alain Michaud <alainm@chromium.org>
---

 net/bluetooth/hci_event.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 0a591be8b0ae..34cd98a1d370 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -5245,7 +5245,9 @@ static struct hci_conn *check_pending_le_conn(struct hci_dev *hdev,
 	/* Most controller will fail if we try to create new connections
 	 * while we have an existing one in slave role.
 	 */
-	if (hdev->conn_hash.le_num_slave > 0)
+	if (hdev->conn_hash.le_num_slave > 0 &&
+	    (hdev->quirks & HCI_QUIRK_VALID_LE_STATES) == 0 ||
+	     !(hdev->le_states[3] & 0x10))
 		return NULL;
 
 	/* If we're not connectable only connect devices that we have in
-- 
2.26.1.301.g55bc3eb7cb9-goog

[PATCH v3 3/3] bluetooth:btusb: Adding support for LE scatternet to Jfp and ThP

[Alain Michaud]

This change adds support for LE scatternet connections to Intel's JfP
and ThP controllers.

Signed-off-by: Alain Michaud <alainm@chromium.org>
---

 drivers/bluetooth/btusb.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index dd27e28d4601..d801c332b85a 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -335,7 +335,8 @@ static const struct usb_device_id blacklist_table[] = {
 
 	/* Intel Bluetooth devices */
 	{ USB_DEVICE(0x8087, 0x0025), .driver_info = BTUSB_INTEL_NEW |
-						     BTUSB_WIDEBAND_SPEECH },
+						     BTUSB_WIDEBAND_SPEECH |
+						     BTUSB_VALID_LE_STATES },
 	{ USB_DEVICE(0x8087, 0x0026), .driver_info = BTUSB_INTEL_NEW |
 						     BTUSB_WIDEBAND_SPEECH },
 	{ USB_DEVICE(0x8087, 0x0029), .driver_info = BTUSB_INTEL_NEW |
@@ -348,7 +349,8 @@ static const struct usb_device_id blacklist_table[] = {
 	{ USB_DEVICE(0x8087, 0x0aa7), .driver_info = BTUSB_INTEL |
 						     BTUSB_WIDEBAND_SPEECH },
 	{ USB_DEVICE(0x8087, 0x0aaa), .driver_info = BTUSB_INTEL_NEW |
-						     BTUSB_WIDEBAND_SPEECH },
+						     BTUSB_WIDEBAND_SPEECH |
+						     BTUSB_VALID_LE_STATES },
 
 	/* Other Intel Bluetooth devices */
 	{ USB_VENDOR_AND_INTERFACE_INFO(0x8087, 0xe0, 0x01, 0x01),
@@ -3877,6 +3879,9 @@ static int btusb_probe(struct usb_interface *intf,
 	if (id->driver_info & BTUSB_WIDEBAND_SPEECH)
 		set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, &hdev->quirks);
 
+	if (id->driver_info & BTUSB_VALID_LE_STATES)
+		set_bit(HCI_QUIRK_VALID_LE_STATES, &hdev->quirks);
+
 	if (id->driver_info & BTUSB_DIGIANSWER) {
 		data->cmdreq_type = USB_TYPE_VENDOR;
 		set_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
-- 
2.26.1.301.g55bc3eb7cb9-goog

Re: [PATCH v3 2/3] bluetooth:allow scatternet connections if supported.

[Marcel Holtmann]

Hi Alain,

> This change allows scatternet connections to be created if the
> controller reports support and the HCI_QUIRK_VALID_LE_STATES indicates
> that the reported LE states can be trusted.
> 
> Signed-off-by: Alain Michaud <alainm@chromium.org>
> ---
> 
> net/bluetooth/hci_event.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 0a591be8b0ae..34cd98a1d370 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -5245,7 +5245,9 @@ static struct hci_conn *check_pending_le_conn(struct hci_dev *hdev,
> 	/* Most controller will fail if we try to create new connections
> 	 * while we have an existing one in slave role.
> 	 */
> -	if (hdev->conn_hash.le_num_slave > 0)
> +	if (hdev->conn_hash.le_num_slave > 0 &&
> +	    (hdev->quirks & HCI_QUIRK_VALID_LE_STATES) == 0 ||

please use test_bit() here.

> +	     !(hdev->le_states[3] & 0x10))
> 		return NULL;

Regards

Marcel

Re: [PATCH v3 2/3] bluetooth:allow scatternet connections if supported.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5093 bytes --]

Hi Alain,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bluetooth-next/master]
[also build test WARNING on v5.7-rc2 next-20200422]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Alain-Michaud/bluetooth-Adding-driver-and-quirk-defs-for-multi-role-LE/20200423-083451
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: c6x-allyesconfig (attached as .config)
compiler: c6x-elf-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=c6x 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   net/bluetooth/hci_event.c: In function 'check_pending_le_conn':
>> net/bluetooth/hci_event.c:5291:39: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
    5291 |  if (hdev->conn_hash.le_num_slave > 0 &&
         |      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
    5292 |      (hdev->quirks & HCI_QUIRK_VALID_LE_STATES) == 0 ||
         |      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +5291 net/bluetooth/hci_event.c

  5270	
  5271	/* This function requires the caller holds hdev->lock */
  5272	static struct hci_conn *check_pending_le_conn(struct hci_dev *hdev,
  5273						      bdaddr_t *addr,
  5274						      u8 addr_type, u8 adv_type,
  5275						      bdaddr_t *direct_rpa)
  5276	{
  5277		struct hci_conn *conn;
  5278		struct hci_conn_params *params;
  5279	
  5280		/* If the event is not connectable don't proceed further */
  5281		if (adv_type != LE_ADV_IND && adv_type != LE_ADV_DIRECT_IND)
  5282			return NULL;
  5283	
  5284		/* Ignore if the device is blocked */
  5285		if (hci_bdaddr_list_lookup(&hdev->blacklist, addr, addr_type))
  5286			return NULL;
  5287	
  5288		/* Most controller will fail if we try to create new connections
  5289		 * while we have an existing one in slave role.
  5290		 */
> 5291		if (hdev->conn_hash.le_num_slave > 0 &&
  5292		    (hdev->quirks & HCI_QUIRK_VALID_LE_STATES) == 0 ||
  5293		     !(hdev->le_states[3] & 0x10))
  5294			return NULL;
  5295	
  5296		/* If we're not connectable only connect devices that we have in
  5297		 * our pend_le_conns list.
  5298		 */
  5299		params = hci_pend_le_action_lookup(&hdev->pend_le_conns, addr,
  5300						   addr_type);
  5301		if (!params)
  5302			return NULL;
  5303	
  5304		if (!params->explicit_connect) {
  5305			switch (params->auto_connect) {
  5306			case HCI_AUTO_CONN_DIRECT:
  5307				/* Only devices advertising with ADV_DIRECT_IND are
  5308				 * triggering a connection attempt. This is allowing
  5309				 * incoming connections from slave devices.
  5310				 */
  5311				if (adv_type != LE_ADV_DIRECT_IND)
  5312					return NULL;
  5313				break;
  5314			case HCI_AUTO_CONN_ALWAYS:
  5315				/* Devices advertising with ADV_IND or ADV_DIRECT_IND
  5316				 * are triggering a connection attempt. This means
  5317				 * that incoming connections from slave device are
  5318				 * accepted and also outgoing connections to slave
  5319				 * devices are established when found.
  5320				 */
  5321				break;
  5322			default:
  5323				return NULL;
  5324			}
  5325		}
  5326	
  5327		conn = hci_connect_le(hdev, addr, addr_type, BT_SECURITY_LOW,
  5328				      HCI_LE_AUTOCONN_TIMEOUT, HCI_ROLE_MASTER,
  5329				      direct_rpa);
  5330		if (!IS_ERR(conn)) {
  5331			/* If HCI_AUTO_CONN_EXPLICIT is set, conn is already owned
  5332			 * by higher layer that tried to connect, if no then
  5333			 * store the pointer since we don't really have any
  5334			 * other owner of the object besides the params that
  5335			 * triggered it. This way we can abort the connection if
  5336			 * the parameters get removed and keep the reference
  5337			 * count consistent once the connection is established.
  5338			 */
  5339	
  5340			if (!params->explicit_connect)
  5341				params->conn = hci_conn_get(conn);
  5342	
  5343			return conn;
  5344		}
  5345	
  5346		switch (PTR_ERR(conn)) {
  5347		case -EBUSY:
  5348			/* If hci_connect() returns -EBUSY it means there is already
  5349			 * an LE connection attempt going on. Since controllers don't
  5350			 * support more than one connection attempt at the time, we
  5351			 * don't consider this an error case.
  5352			 */
  5353			break;
  5354		default:
  5355			BT_DBG("Failed to connect: err %ld", PTR_ERR(conn));
  5356			return NULL;
  5357		}
  5358	
  5359		return NULL;
  5360	}
  5361	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 52562 bytes --]

Re: [PATCH v3 2/3] bluetooth:allow scatternet connections if supported.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5225 bytes --]

Hi Alain,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bluetooth-next/master]
[also build test WARNING on v5.7-rc2 next-20200422]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Alain-Michaud/bluetooth-Adding-driver-and-quirk-defs-for-multi-role-LE/20200423-083451
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: c6x-allyesconfig (attached as .config)
compiler: c6x-elf-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=c6x 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   net/bluetooth/hci_event.c: In function 'check_pending_le_conn':
>> net/bluetooth/hci_event.c:5291:39: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
    5291 |  if (hdev->conn_hash.le_num_slave > 0 &&
         |      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
    5292 |      (hdev->quirks & HCI_QUIRK_VALID_LE_STATES) == 0 ||
         |      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +5291 net/bluetooth/hci_event.c

  5270	
  5271	/* This function requires the caller holds hdev->lock */
  5272	static struct hci_conn *check_pending_le_conn(struct hci_dev *hdev,
  5273						      bdaddr_t *addr,
  5274						      u8 addr_type, u8 adv_type,
  5275						      bdaddr_t *direct_rpa)
  5276	{
  5277		struct hci_conn *conn;
  5278		struct hci_conn_params *params;
  5279	
  5280		/* If the event is not connectable don't proceed further */
  5281		if (adv_type != LE_ADV_IND && adv_type != LE_ADV_DIRECT_IND)
  5282			return NULL;
  5283	
  5284		/* Ignore if the device is blocked */
  5285		if (hci_bdaddr_list_lookup(&hdev->blacklist, addr, addr_type))
  5286			return NULL;
  5287	
  5288		/* Most controller will fail if we try to create new connections
  5289		 * while we have an existing one in slave role.
  5290		 */
> 5291		if (hdev->conn_hash.le_num_slave > 0 &&
  5292		    (hdev->quirks & HCI_QUIRK_VALID_LE_STATES) == 0 ||
  5293		     !(hdev->le_states[3] & 0x10))
  5294			return NULL;
  5295	
  5296		/* If we're not connectable only connect devices that we have in
  5297		 * our pend_le_conns list.
  5298		 */
  5299		params = hci_pend_le_action_lookup(&hdev->pend_le_conns, addr,
  5300						   addr_type);
  5301		if (!params)
  5302			return NULL;
  5303	
  5304		if (!params->explicit_connect) {
  5305			switch (params->auto_connect) {
  5306			case HCI_AUTO_CONN_DIRECT:
  5307				/* Only devices advertising with ADV_DIRECT_IND are
  5308				 * triggering a connection attempt. This is allowing
  5309				 * incoming connections from slave devices.
  5310				 */
  5311				if (adv_type != LE_ADV_DIRECT_IND)
  5312					return NULL;
  5313				break;
  5314			case HCI_AUTO_CONN_ALWAYS:
  5315				/* Devices advertising with ADV_IND or ADV_DIRECT_IND
  5316				 * are triggering a connection attempt. This means
  5317				 * that incoming connections from slave device are
  5318				 * accepted and also outgoing connections to slave
  5319				 * devices are established when found.
  5320				 */
  5321				break;
  5322			default:
  5323				return NULL;
  5324			}
  5325		}
  5326	
  5327		conn = hci_connect_le(hdev, addr, addr_type, BT_SECURITY_LOW,
  5328				      HCI_LE_AUTOCONN_TIMEOUT, HCI_ROLE_MASTER,
  5329				      direct_rpa);
  5330		if (!IS_ERR(conn)) {
  5331			/* If HCI_AUTO_CONN_EXPLICIT is set, conn is already owned
  5332			 * by higher layer that tried to connect, if no then
  5333			 * store the pointer since we don't really have any
  5334			 * other owner of the object besides the params that
  5335			 * triggered it. This way we can abort the connection if
  5336			 * the parameters get removed and keep the reference
  5337			 * count consistent once the connection is established.
  5338			 */
  5339	
  5340			if (!params->explicit_connect)
  5341				params->conn = hci_conn_get(conn);
  5342	
  5343			return conn;
  5344		}
  5345	
  5346		switch (PTR_ERR(conn)) {
  5347		case -EBUSY:
  5348			/* If hci_connect() returns -EBUSY it means there is already
  5349			 * an LE connection attempt going on. Since controllers don't
  5350			 * support more than one connection attempt at the time, we
  5351			 * don't consider this an error case.
  5352			 */
  5353			break;
  5354		default:
  5355			BT_DBG("Failed to connect: err %ld", PTR_ERR(conn));
  5356			return NULL;
  5357		}
  5358	
  5359		return NULL;
  5360	}
  5361	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 52562 bytes --]

Re: [PATCH v3 0/3] bluetooth:Adding driver and quirk defs for multi-role LE

[Alain Michaud]

Friendly ping on these series.


On Thu, Apr 23, 2020 at 10:43 AM Alain Michaud <alainm@chromium.org> wrote:
>
> This series adds BTUSB and quirk support for the driver to confirm that
> the reported LE_states can be trusted.  The quirk will be used to
> gradually roll out the feature to supported controllers without
> affecting the stability of other controllers.  If all controllers FWs
> are fixed or else validated, we can consider removing the quirk over
> time.
>
>
> Alain Michaud (3):
>   bluetooth:Adding driver and quirk defs for multi-role LE
>   bluetooth:allow scatternet connections if supported.
>   bluetooth:btusb: Adding support for LE scatternet to Jfp and ThP
>
>  drivers/bluetooth/btusb.c   | 11 ++++++++---
>  include/net/bluetooth/hci.h |  9 +++++++++
>  net/bluetooth/hci_event.c   |  4 +++-
>  3 files changed, 20 insertions(+), 4 deletions(-)
>
> --
> 2.26.1.301.g55bc3eb7cb9-goog
>

[PATCH v3 0/3] bluetooth:Adding driver and quirk defs for multi-role LE

[Alain Michaud]

This series adds BTUSB and quirk support for the driver to confirm that
the reported LE_states can be trusted.  The quirk will be used to
gradually roll out the feature to supported controllers without
affecting the stability of other controllers.  If all controllers FWs
are fixed or else validated, we can consider removing the quirk over
time.


Alain Michaud (3):
  bluetooth:Adding driver and quirk defs for multi-role LE
  bluetooth:allow scatternet connections if supported.
  bluetooth:btusb: Adding support for LE scatternet to Jfp and ThP

 drivers/bluetooth/btusb.c   | 11 ++++++++---
 include/net/bluetooth/hci.h |  9 +++++++++
 net/bluetooth/hci_event.c   |  4 +++-
 3 files changed, 20 insertions(+), 4 deletions(-)

-- 
2.26.1.301.g55bc3eb7cb9-goog