[PATCH 0/7] Bluetooth: make the Amlogic W1 (W155S2) usable

[PATCH 0/7] Bluetooth: make the Amlogic W1 (W155S2) usable

[christian.koever-draxl]

From: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

The W1 combo chip in an S905W2 TV box could not be powered on by
bluetoothd and did not survive warm reboots. This series:

 1-2: adds two quirks, for controllers that advertise Secure Connections
      but reject enabling it, and for controllers that hang on Write
      Current IAC LAP (sent when the adapter becomes discoverable);
 3:   lets hci_aml cope with a controller that still runs firmware from
      a previous boot (the enable line does not reset it), retries lost
      firmware segments, waits long enough for the firmware to start,
      and keeps the controller powered across down/up;
 4-5: takes a stable address from local-bd-address, as the controller
      keeps none across a power cycle;
 6-7: adds amlogic,shared-antenna for boards with one antenna for Wi-Fi
      and Bluetooth (about 15 dB better reception there).

Tested: cold boot, warm reboot, rfkill and hciconfig down/up cycles,
classic keyboard pairing and reconnects, LE central and peripheral.

Tool use (Documentation/process/generated-content.rst):
This work was done with the help of an AI coding assistant, Claude
(Anthropic), used through Claude Code over many sessions. It did a large
part of the hands-on work: it read the vendor kernel and the firmware
interfaces, probed registers on the board, drafted the code and the
changelogs, and ran the builds, checkpatch, dt_binding_check and the
tests on the hardware. I directed the work, reviewed every change and
the final patches, and take responsibility for them. There is no single
prompt to quote; the sessions were of the form "find out why X fails on
this board, fix it, test it on the box".

Tested on an Amediatech X98Q TV box (Amlogic S905W2, S4 family, 2 GiB),
vendor U-Boot and BL31/OP-TEE, Linux 7.3 plus this work.

Christian Stefan Kövér-Draxl (7):
  Bluetooth: add a quirk for controllers that cannot enable Secure
    Connections
  Bluetooth: add a quirk for controllers that hang on Write Current IAC
    LAP
  Bluetooth: hci_aml: cope with a controller that is already running
  dt-bindings: net: bluetooth: amlogic,w155s2-bt: allow local-bd-address
  Bluetooth: hci_aml: take the address from the firmware description
  dt-bindings: net: bluetooth: amlogic,w155s2-bt: add
    amlogic,shared-antenna
  Bluetooth: hci_aml: configure the RF for a shared antenna

 .../net/bluetooth/amlogic,w155s2-bt.yaml      |  11 +
 drivers/bluetooth/hci_aml.c                   | 197 ++++++++++++++++--
 include/net/bluetooth/hci.h                   |  32 +++
 net/bluetooth/hci_sync.c                      |  16 +-
 4 files changed, 231 insertions(+), 25 deletions(-)


base-commit: 7be4bc52fd9e0e55dc9a2e0232b89427d2c4b614
-- 
2.56.0

[PATCH 1/7] Bluetooth: add a quirk for controllers that cannot enable Secure Connections

[christian.koever-draxl]

From: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

The Amlogic W1 (W155S2) lists BR/EDR Secure Connections among its
extended features and then fails HCI_OP_WRITE_SC_SUPPORT with an error.
That failure is fatal to hci_dev_open_sync(), so the adapter can never
be powered on: every mgmt Set Powered returns Invalid Parameters and
bluetoothd gives up on the controller.

Add HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT for controllers whose feature bit
cannot be taken at face value, skip the command in both places it is
issued, and set the quirk in hci_aml.

Signed-off-by: Christian Stefan Kövér-Draxl <ckoever@outlook.de>
---
 drivers/bluetooth/hci_aml.c |  5 +++++
 include/net/bluetooth/hci.h | 14 ++++++++++++++
 net/bluetooth/hci_sync.c    |  8 ++++++--
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_aml.c b/drivers/bluetooth/hci_aml.c
index 067fbf278b44..990e701da4be 100644
--- a/drivers/bluetooth/hci_aml.c
+++ b/drivers/bluetooth/hci_aml.c
@@ -598,6 +598,11 @@ static int aml_setup(struct hci_uart *hu)
 	/* Setup bdaddr */
 	hdev->set_bdaddr = aml_set_bdaddr;
 
+	/* The controller lists BR/EDR Secure Connections among its extended
+	 * features and then rejects the command that turns host support on.
+	 */
+	hci_set_quirk(hdev, HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT);
+
 	err = aml_power_on(amldev);
 	if (err)
 		return err;
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 1641d879dbda..bcbe76c84d1b 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -375,6 +375,20 @@ enum {
 	 */
 	HCI_QUIRK_BROKEN_READ_PAGE_SCAN_TYPE,
 
+	/*
+	 * When this quirk is set, the HCI_OP_WRITE_SC_SUPPORT command is
+	 * not supported.
+	 *
+	 * This quirk is intended for controllers that list BR/EDR Secure
+	 * Connections among their extended features and then reject the
+	 * command that turns host support on. Without it the failure takes
+	 * the whole of hci_dev_open_sync() with it and the controller can
+	 * never be powered on.
+	 *
+	 * This quirk must be set before hci_register_dev is called.
+	 */
+	HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT,
+
 	__HCI_NUM_QUIRKS,
 };
 
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index a92a81846e9d..f1dfef81ca96 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -3419,7 +3419,8 @@ int hci_write_sc_support_sync(struct hci_dev *hdev, u8 val)
 {
 	int err;
 
-	if (!bredr_sc_enabled(hdev) || lmp_host_sc_capable(hdev))
+	if (!bredr_sc_enabled(hdev) || lmp_host_sc_capable(hdev) ||
+	    hci_test_quirk(hdev, HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT))
 		return 0;
 
 	err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_SC_SUPPORT,
@@ -5040,7 +5041,8 @@ static int hci_write_sc_support_1_sync(struct hci_dev *hdev)
 	u8 support = 0x01;
 
 	if (!hci_dev_test_flag(hdev, HCI_SSP_ENABLED) ||
-	    !bredr_sc_enabled(hdev))
+	    !bredr_sc_enabled(hdev) ||
+	    hci_test_quirk(hdev, HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT))
 		return 0;
 
 	return __hci_cmd_sync_status(hdev, HCI_OP_WRITE_SC_SUPPORT,
@@ -5254,6 +5256,8 @@ static const struct {
 			 "HCI WRITE AUTH PAYLOAD TIMEOUT command leads "
 			 "to unexpected SMP errors when pairing "
 			 "and will not be used."),
+	HCI_QUIRK_BROKEN(WRITE_SC_SUPPORT,
+			 "HCI BR/EDR Secure Connections is advertised, but cannot be enabled."),
 	HCI_QUIRK_BROKEN(LE_CODED,
 			 "HCI LE Coded PHY feature bit is set, "
 			 "but its usage is not supported.")
-- 
2.56.0

[PATCH 2/7] Bluetooth: add a quirk for controllers that hang on Write Current IAC LAP

[christian.koever-draxl]

From: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

Some controllers list Write Current IAC LAP among their supported
commands and answer Read Current IAC LAP, but stop responding to HCI
altogether as soon as the write is issued, even when the value written
is the general inquiry access code the controller already reports.
Nothing short of a power cycle recovers the part.

The write is only sent on the way to becoming discoverable, so such a
controller dies exactly when something asks it to become visible, and
the failure looks like a dead radio rather than a refused command.

Add HCI_QUIRK_BROKEN_WRITE_CURRENT_IAC_LAP and skip the write when it
is set. This costs limited discoverable mode, which needs the LIAC
programmed. General discoverable mode is unaffected, as the controller
already holds the GIAC.

Seen on the Amlogic W1 (W155S2); set the quirk in hci_aml.

Signed-off-by: Christian Stefan Kövér-Draxl <ckoever@outlook.de>
---
 drivers/bluetooth/hci_aml.c |  6 ++++++
 include/net/bluetooth/hci.h | 18 ++++++++++++++++++
 net/bluetooth/hci_sync.c    |  8 ++++++++
 3 files changed, 32 insertions(+)

diff --git a/drivers/bluetooth/hci_aml.c b/drivers/bluetooth/hci_aml.c
index 990e701da4be..5979b114f2e4 100644
--- a/drivers/bluetooth/hci_aml.c
+++ b/drivers/bluetooth/hci_aml.c
@@ -603,6 +603,12 @@ static int aml_setup(struct hci_uart *hu)
 	 */
 	hci_set_quirk(hdev, HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT);
 
+	/* Write Current IAC LAP is advertised, but the controller stops
+	 * answering HCI as soon as it is sent, until the combo chip is power
+	 * cycled. It is sent when the adapter is made discoverable.
+	 */
+	hci_set_quirk(hdev, HCI_QUIRK_BROKEN_WRITE_CURRENT_IAC_LAP);
+
 	err = aml_power_on(amldev);
 	if (err)
 		return err;
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index bcbe76c84d1b..a59f48f387ac 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -389,6 +389,24 @@ enum {
 	 */
 	HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT,
 
+	/* When this quirk is set, HCI_OP_WRITE_CURRENT_IAC_LAP is never sent.
+	 *
+	 * Some controllers list the command among the ones they support, and
+	 * answer Read Current IAC LAP quite happily, but wedge and stop
+	 * responding to anything at all the moment the write is issued - even
+	 * when the value being written is the general inquiry access code the
+	 * controller already reports. Going discoverable is what triggers the
+	 * write, so on such a controller the adapter dies precisely when it is
+	 * asked to become visible.
+	 *
+	 * Skipping the write costs limited discoverable mode, which needs the
+	 * LIAC programmed. General discoverable mode is unaffected: the GIAC
+	 * is what these controllers already have.
+	 *
+	 * This quirk must be set before hci_register_dev is called.
+	 */
+	HCI_QUIRK_BROKEN_WRITE_CURRENT_IAC_LAP,
+
 	__HCI_NUM_QUIRKS,
 };
 
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index f1dfef81ca96..92c442910d6a 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -5258,6 +5258,8 @@ static const struct {
 			 "and will not be used."),
 	HCI_QUIRK_BROKEN(WRITE_SC_SUPPORT,
 			 "HCI BR/EDR Secure Connections is advertised, but cannot be enabled."),
+	HCI_QUIRK_BROKEN(WRITE_CURRENT_IAC_LAP,
+			 "HCI Write Current IAC LAP is advertised, but hangs the controller."),
 	HCI_QUIRK_BROKEN(LE_CODED,
 			 "HCI LE Coded PHY feature bit is set, "
 			 "but its usage is not supported.")
@@ -6127,6 +6129,12 @@ static int hci_write_iac_sync(struct hci_dev *hdev)
 	if (!hci_dev_test_flag(hdev, HCI_DISCOVERABLE))
 		return 0;
 
+	/* Controllers that wedge on the write are left alone. They already
+	 * hold the GIAC, so general discoverable mode works without it.
+	 */
+	if (hci_test_quirk(hdev, HCI_QUIRK_BROKEN_WRITE_CURRENT_IAC_LAP))
+		return 0;
+
 	memset(&cp, 0, sizeof(cp));
 
 	if (hci_dev_test_flag(hdev, HCI_LIMITED_DISCOVERABLE)) {
-- 
2.56.0

[PATCH 3/7] Bluetooth: hci_aml: cope with a controller that is already running

[christian.koever-draxl]

From: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

Several things stop the W1 (W155S2) in an S905W2 TV box from completing
setup reliably:

 - Dropping the enable line does not reset this controller. It keeps
   running whatever firmware it was last given, at the baud rate it was
   last told to use, so a warm reboot or a driver reload finds an HCI
   device at 4 Mbps that no longer understands TCI commands, and the
   download dies on the first TCI write. Read Local Version tells the
   two cases apart, as the ROM does not answer it, so probe for it and
   skip the download when firmware is already running.

 - Talking to a running controller first means sending it a TCI command
   at the ROM's 115200 baud, which arrives as noise at 4 Mbps. The
   firmware takes it for the start of a packet and swallows the next
   command as its payload. Retry the probe; every attempt completes
   more of the imagined packet until the controller listens again.

 - A single unanswered firmware segment aborted the bring-up. Segments
   are plain writes to controller RAM, so resync and send it again.

 - The fixed 60 ms wait after starting the firmware is too short for
   W1, which takes up to half a second. A version request sent too early
   is lost and blocks the command queue for HCI_CMD_TIMEOUT, so wait
   600 ms, then poll the firmware version until it answers.

 - Closing the HCI device dropped the enable line. Setup, which powers
   the controller up, only runs once per registration, and the dropped
   enable line leaves the controller unreachable until the combo chip
   is power cycled, so the adapter was lost after a single down/up
   cycle. Keep it powered in close and power it off on remove instead,
   tracking the state so remove and shutdown stay balanced. Rebinding
   the driver still loses the controller, as the hardware gives no way
   to reset it short of a power cycle.

Tested on an S905W2 board: setup succeeds on cold boot and warm reboot,
and the adapter survives down/up cycles.

Signed-off-by: Christian Stefan Kövér-Draxl <ckoever@outlook.de>
---
 drivers/bluetooth/hci_aml.c | 186 ++++++++++++++++++++++++++++++------
 1 file changed, 158 insertions(+), 28 deletions(-)

diff --git a/drivers/bluetooth/hci_aml.c b/drivers/bluetooth/hci_aml.c
index 5979b114f2e4..8561618867e8 100644
--- a/drivers/bluetooth/hci_aml.c
+++ b/drivers/bluetooth/hci_aml.c
@@ -24,6 +24,29 @@
 
 #define AML_FIRMWARE_OPERATION_SIZE		(248)
 #define AML_FIRMWARE_MAX_SIZE			(512 * 1024)
+#define AML_FIRMWARE_SEGMENT_RETRIES		(1)
+
+/* Probing for a controller that already runs firmware is expected to time
+ * out on a freshly powered one, so do not wait HCI_INIT_TIMEOUT for it.
+ */
+#define AML_PROBE_TIMEOUT			msecs_to_jiffies(500)
+
+/* Long enough for the controller to drop a partial frame received at the
+ * wrong rate.
+ */
+#define AML_RESYNC_MS				(100)
+#define AML_RESYNC_RETRIES			(4)
+
+/* An HCI command has an 8 bit length, so this always completes a partial one */
+#define AML_RESYNC_CHUNKS_CMD			(2)
+
+/* W1 takes up to half a second to start its firmware, and a version request
+ * sent before that is lost and blocks the command queue for HCI_CMD_TIMEOUT,
+ * so wait a little longer than that before asking, then keep asking.
+ */
+#define AML_STARTUP_DELAY_MS			(600)
+#define AML_STARTUP_POLL_MS			(60)
+#define AML_STARTUP_RETRIES			(5)
 
 /* TCI command */
 #define AML_TCI_CMD_READ			0xFEF0
@@ -100,6 +123,7 @@ struct aml_serdev {
 	struct clk *lpo_clk;
 	const struct aml_device_data *aml_dev_data;
 	const char *firmware_name;
+	bool powered;
 };
 
 struct aml_data {
@@ -214,11 +238,33 @@ static int aml_send_firmware_segment(struct hci_dev *hdev,
 			     op_addr, (u32 *)seg, seg_size);
 }
 
-static int aml_send_firmware(struct hci_dev *hdev, u8 fw_type,
-			     u8 *fw, u32 fw_size, u32 offset)
+static const u8 aml_resync_pattern[256];
+
+/* A command that goes unanswered has usually been half received: the
+ * controller took what did arrive for the start of a packet and waits for
+ * the rest, so every following command is swallowed as its payload. Send
+ * enough bytes to complete whatever it is reassembling. Zero is not a valid
+ * H4 packet type, so what is left over afterwards is discarded.
+ */
+static void aml_resync(struct hci_uart *hu, unsigned int chunks)
+{
+	unsigned int i;
+
+	for (i = 0; i < chunks; i++)
+		serdev_device_write(hu->serdev, aml_resync_pattern,
+				    sizeof(aml_resync_pattern),
+				    MAX_SCHEDULE_TIMEOUT);
+
+	serdev_device_wait_until_sent(hu->serdev, 0);
+}
+
+static int aml_send_firmware(struct hci_dev *hdev, struct hci_uart *hu,
+			     u8 fw_type, u8 *fw, u32 fw_size, u32 offset)
 {
 	u32 seg_size = 0;
 	u32 seg_off = 0;
+	int err = 0;
+	int retry;
 
 	if (fw_size > AML_FIRMWARE_MAX_SIZE) {
 		bt_dev_err(hdev,
@@ -229,8 +275,22 @@ static int aml_send_firmware(struct hci_dev *hdev, u8 fw_type,
 	while (fw_size > 0) {
 		seg_size = (fw_size > AML_FIRMWARE_OPERATION_SIZE) ?
 			   AML_FIRMWARE_OPERATION_SIZE : fw_size;
-		if (aml_send_firmware_segment(hdev, fw_type, (fw + seg_off),
-					      seg_size, offset)) {
+		/* A segment is a plain write to controller RAM, so one that
+		 * goes unanswered can simply be sent again, after resyncing
+		 * the controller that is still waiting for its end.
+		 */
+		for (retry = 0; retry <= AML_FIRMWARE_SEGMENT_RETRIES; retry++) {
+			err = aml_send_firmware_segment(hdev, fw_type,
+							fw + seg_off,
+							seg_size, offset);
+			if (!err)
+				break;
+
+			bt_dev_warn(hdev, "Retrying firmware segment, type: %d, offset: 0x%x",
+				    fw_type, offset);
+			aml_resync(hu, AML_RESYNC_CHUNKS_CMD);
+		}
+		if (err) {
 			bt_dev_err(hdev, "Failed send firmware, type: %d, offset: 0x%x",
 			       fw_type, offset);
 			return -EINVAL;
@@ -242,6 +302,35 @@ static int aml_send_firmware(struct hci_dev *hdev, u8 fw_type,
 	return 0;
 }
 
+/* Dropping the enable line does not reset these controllers: they keep
+ * running the firmware they were last given, at the baud rate they were last
+ * told to use. A warm reboot or a driver reload therefore finds an HCI device
+ * at the operational rate that no longer understands TCI commands. Read Local
+ * Version tells the two cases apart: the ROM does not answer it, running
+ * firmware does.
+ */
+static bool aml_fw_already_running(struct hci_dev *hdev, struct hci_uart *hu)
+{
+	struct sk_buff *skb;
+	int i;
+
+	serdev_device_set_baudrate(hu->serdev, hu->proto->oper_speed);
+
+	for (i = 0; i < AML_RESYNC_RETRIES; i++) {
+		skb = __hci_cmd_sync_ev(hdev, HCI_OP_READ_LOCAL_VERSION, 0,
+					NULL, HCI_EV_CMD_COMPLETE,
+					AML_PROBE_TIMEOUT);
+		if (!IS_ERR(skb)) {
+			kfree_skb(skb);
+			return true;
+		}
+	}
+
+	serdev_device_set_baudrate(hu->serdev, hu->proto->init_speed);
+
+	return false;
+}
+
 static int aml_download_firmware(struct hci_dev *hdev, const char *fw_name)
 {
 	struct hci_uart *hu = hci_get_drvdata(hdev);
@@ -301,7 +390,7 @@ static int aml_download_firmware(struct hci_dev *hdev, const char *fw_name)
 	iccm_start = (u8 *)(firmware->data) + sizeof(struct aml_fw_len)
 			+ amldev->aml_dev_data->iccm_offset;
 	iccm_len = fw_len->iccm_len - amldev->aml_dev_data->iccm_offset;
-	ret = aml_send_firmware(hdev, FW_ICCM, iccm_start, iccm_len,
+	ret = aml_send_firmware(hdev, hu, FW_ICCM, iccm_start, iccm_len,
 				amldev->aml_dev_data->iccm_offset);
 	if (ret) {
 		bt_dev_err(hdev, "Failed to send FW_ICCM (%d)", ret);
@@ -311,7 +400,7 @@ static int aml_download_firmware(struct hci_dev *hdev, const char *fw_name)
 	/* Download DCCM */
 	dccm_start = (u8 *)(firmware->data) + sizeof(struct aml_fw_len) + fw_len->iccm_len;
 	dccm_len = fw_len->dccm_len;
-	ret = aml_send_firmware(hdev, FW_DCCM, dccm_start, dccm_len,
+	ret = aml_send_firmware(hdev, hu, FW_DCCM, dccm_start, dccm_len,
 				amldev->aml_dev_data->dccm_offset);
 	if (ret) {
 		bt_dev_err(hdev, "Failed to send FW_DCCM (%d)", ret);
@@ -508,6 +597,7 @@ static int aml_power_on(struct aml_serdev *amldev)
 	}
 
 	gpiod_set_value_cansleep(amldev->bt_en_gpio, 1);
+	amldev->powered = true;
 
 	/* Wait 20ms for bluetooth controller power on */
 	msleep(20);
@@ -516,6 +606,10 @@ static int aml_power_on(struct aml_serdev *amldev)
 
 static int aml_power_off(struct aml_serdev *amldev)
 {
+	if (!amldev->powered)
+		return 0;
+
+	amldev->powered = false;
 	gpiod_set_value_cansleep(amldev->bt_en_gpio, 0);
 
 	clk_disable_unprepare(amldev->lpo_clk);
@@ -568,7 +662,6 @@ static int aml_open(struct hci_uart *hu)
 
 static int aml_close(struct hci_uart *hu)
 {
-	struct aml_serdev *amldev = serdev_device_get_drvdata(hu->serdev);
 	struct aml_data *aml_data = hu->priv;
 
 	skb_queue_purge(&aml_data->txq);
@@ -577,7 +670,13 @@ static int aml_close(struct hci_uart *hu)
 
 	hu->priv = NULL;
 
-	return aml_power_off(amldev);
+	/* The controller is left powered. Setup, which powers it on, only
+	 * runs once per registration, and dropping the enable line does not
+	 * reset it but leaves it unreachable until the combo chip is power
+	 * cycled, so powering off here would lose the controller on the next
+	 * open. It is powered off on remove and shutdown.
+	 */
+	return 0;
 }
 
 static int aml_flush(struct hci_uart *hu)
@@ -589,6 +688,55 @@ static int aml_flush(struct hci_uart *hu)
 	return 0;
 }
 
+/* Everything between powering the controller up and a usable HCI device */
+static int aml_setup_controller(struct hci_uart *hu, struct aml_serdev *amldev,
+				struct hci_dev *hdev)
+{
+	int err;
+	int i;
+
+	/* Try the fresh controller first: a ROM that has just been probed at
+	 * the wrong rate swallows the command that follows.
+	 */
+	err = aml_set_baudrate(hu, hu->proto->oper_speed);
+	if (err) {
+		msleep(AML_RESYNC_MS);
+
+		if (!aml_fw_already_running(hdev, hu))
+			return err;
+
+		bt_dev_info(hdev, "Controller is already running firmware");
+		return 0;
+	}
+
+	err = aml_download_firmware(hdev, amldev->firmware_name);
+	if (err)
+		return err;
+
+	err = aml_config_rf(hdev, amldev->aml_dev_data->is_coex);
+	if (err)
+		return err;
+
+	err = aml_start_chip(hdev);
+	if (err)
+		return err;
+
+	/* There is nothing to poll but the version command itself, and the
+	 * time to start varies: W1 takes up to half a second.
+	 */
+	msleep(AML_STARTUP_DELAY_MS);
+	for (i = 0; i < AML_STARTUP_RETRIES; i++) {
+		if (i)
+			msleep(AML_STARTUP_POLL_MS);
+
+		err = aml_dump_fw_version(hdev);
+		if (!err)
+			break;
+	}
+
+	return err;
+}
+
 static int aml_setup(struct hci_uart *hu)
 {
 	struct aml_serdev *amldev = serdev_device_get_drvdata(hu->serdev);
@@ -613,26 +761,7 @@ static int aml_setup(struct hci_uart *hu)
 	if (err)
 		return err;
 
-	err = aml_set_baudrate(hu, amldev->serdev_hu.proto->oper_speed);
-	if (err)
-		return err;
-
-	err = aml_download_firmware(hdev, amldev->firmware_name);
-	if (err)
-		return err;
-
-	err = aml_config_rf(hdev, amldev->aml_dev_data->is_coex);
-	if (err)
-		return err;
-
-	err = aml_start_chip(hdev);
-	if (err)
-		return err;
-
-	/* Wait 60ms for controller startup */
-	msleep(60);
-
-	err = aml_dump_fw_version(hdev);
+	err = aml_setup_controller(hu, amldev, hdev);
 	if (err)
 		return err;
 
@@ -730,6 +859,7 @@ static void aml_serdev_remove(struct serdev_device *serdev)
 	struct aml_serdev *amldev = serdev_device_get_drvdata(serdev);
 
 	hci_uart_unregister_device(&amldev->serdev_hu);
+	aml_power_off(amldev);
 }
 
 static void aml_serdev_shutdown(struct serdev_device *serdev)
-- 
2.56.0

[PATCH 4/7] dt-bindings: net: bluetooth: amlogic,w155s2-bt: allow local-bd-address

[christian.koever-draxl]

From: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

These controllers report a fixed default address and keep nothing
across a power cycle, so a board needs a way to give them a stable one.
Reference the common Bluetooth controller schema and allow
local-bd-address.

Signed-off-by: Christian Stefan Kövér-Draxl <ckoever@outlook.de>
---
 .../devicetree/bindings/net/bluetooth/amlogic,w155s2-bt.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/bluetooth/amlogic,w155s2-bt.yaml b/Documentation/devicetree/bindings/net/bluetooth/amlogic,w155s2-bt.yaml
index 6fd7557039d2..50291a438ce2 100644
--- a/Documentation/devicetree/bindings/net/bluetooth/amlogic,w155s2-bt.yaml
+++ b/Documentation/devicetree/bindings/net/bluetooth/amlogic,w155s2-bt.yaml
@@ -15,6 +15,9 @@ description:
 maintainers:
   - Yang Li <yang.li@amlogic.com>
 
+allOf:
+  - $ref: /schemas/net/bluetooth/bluetooth-controller.yaml#
+
 properties:
   compatible:
     oneOf:
@@ -41,6 +44,8 @@ properties:
     maxItems: 1
     description: specify the path of firmware bin to load
 
+  local-bd-address: true
+
 required:
   - compatible
   - clocks
-- 
2.56.0

[PATCH 5/7] Bluetooth: hci_aml: take the address from the firmware description

[christian.koever-draxl]

From: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

When the controller reports the Amlogic default address the driver
marks it invalid, which leaves userspace to configure one before the
adapter can be used. The controller does not keep an address across a
power cycle, so there is nowhere else a stable one can come from.

Set HCI_QUIRK_USE_BDADDR_PROPERTY as well, so that a local-bd-address
property, typically filled in by the bootloader, is used when present.

Signed-off-by: Christian Stefan Kövér-Draxl <ckoever@outlook.de>
---
 drivers/bluetooth/hci_aml.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/bluetooth/hci_aml.c b/drivers/bluetooth/hci_aml.c
index 8561618867e8..d1b83f7bd388 100644
--- a/drivers/bluetooth/hci_aml.c
+++ b/drivers/bluetooth/hci_aml.c
@@ -528,6 +528,10 @@ static int aml_check_bdaddr(struct hci_dev *hdev)
 	if (!bacmp(&paddr->bdaddr, AML_BDADDR_DEFAULT)) {
 		bt_dev_info(hdev, "amlbt using default bdaddr (%pM)", &paddr->bdaddr);
 		hci_set_quirk(hdev, HCI_QUIRK_INVALID_BDADDR);
+		/* Nothing is kept across a power cycle either, so let the
+		 * firmware description provide an address.
+		 */
+		hci_set_quirk(hdev, HCI_QUIRK_USE_BDADDR_PROPERTY);
 	}
 
 exit:
-- 
2.56.0

[PATCH 6/7] dt-bindings: net: bluetooth: amlogic,w155s2-bt: add amlogic,shared-antenna

[christian.koever-draxl]

From: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

Some boards wire Bluetooth and Wi-Fi of the combo chip to a single
antenna. The Bluetooth RF has to be configured for that, which the
driver currently only does for the W265S2-class coexistence parts.
Add a property for boards to describe it.

Signed-off-by: Christian Stefan Kövér-Draxl <ckoever@outlook.de>
---
 .../bindings/net/bluetooth/amlogic,w155s2-bt.yaml           | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/bluetooth/amlogic,w155s2-bt.yaml b/Documentation/devicetree/bindings/net/bluetooth/amlogic,w155s2-bt.yaml
index 50291a438ce2..aff8c2a54cf8 100644
--- a/Documentation/devicetree/bindings/net/bluetooth/amlogic,w155s2-bt.yaml
+++ b/Documentation/devicetree/bindings/net/bluetooth/amlogic,w155s2-bt.yaml
@@ -46,6 +46,12 @@ properties:
 
   local-bd-address: true
 
+  amlogic,shared-antenna:
+    type: boolean
+    description:
+      Bluetooth and Wi-Fi share a single antenna. The controller has to be
+      configured for it, otherwise its receive path is severely attenuated.
+
 required:
   - compatible
   - clocks
-- 
2.56.0

[PATCH 7/7] Bluetooth: hci_aml: configure the RF for a shared antenna

[christian.koever-draxl]

From: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

The RF configuration selects the single antenna setting only for parts
flagged as coexistence capable. On an S905W2 TV box with a W155S2-class
W1 chip, Bluetooth and Wi-Fi share one antenna; with the default dual
antenna setting the received signal is about 15 dB weaker.

Use the single antenna setting when the board sets
amlogic,shared-antenna.

Signed-off-by: Christian Stefan Kövér-Draxl <ckoever@outlook.de>
---
 drivers/bluetooth/hci_aml.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_aml.c b/drivers/bluetooth/hci_aml.c
index d1b83f7bd388..dc797ab665b9 100644
--- a/drivers/bluetooth/hci_aml.c
+++ b/drivers/bluetooth/hci_aml.c
@@ -123,6 +123,7 @@ struct aml_serdev {
 	struct clk *lpo_clk;
 	const struct aml_device_data *aml_dev_data;
 	const char *firmware_name;
+	bool shared_antenna;
 	bool powered;
 };
 
@@ -569,6 +570,9 @@ static int aml_parse_dt(struct aml_serdev *amldev)
 		return -ENODEV;
 	}
 
+	amldev->shared_antenna = device_property_read_bool(pdev,
+							   "amlogic,shared-antenna");
+
 	amldev->bt_supply = devm_regulator_get(pdev, "vddio");
 	if (IS_ERR(amldev->bt_supply)) {
 		dev_err(pdev, "Failed to acquire regulator");
@@ -696,6 +700,8 @@ static int aml_flush(struct hci_uart *hu)
 static int aml_setup_controller(struct hci_uart *hu, struct aml_serdev *amldev,
 				struct hci_dev *hdev)
 {
+	bool single_antenna = amldev->aml_dev_data->is_coex ||
+			      amldev->shared_antenna;
 	int err;
 	int i;
 
@@ -717,7 +723,7 @@ static int aml_setup_controller(struct hci_uart *hu, struct aml_serdev *amldev,
 	if (err)
 		return err;
 
-	err = aml_config_rf(hdev, amldev->aml_dev_data->is_coex);
+	err = aml_config_rf(hdev, single_antenna);
 	if (err)
 		return err;
 
-- 
2.56.0

Re: [PATCH 1/7] Bluetooth: add a quirk for controllers that cannot enable Secure Connections

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Bluetooth: unmasked lmp_sc_capable() with HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT
- [Low] Bluetooth: inaccurate comment for HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT in hci.h
--

commit 300bedb0d4e07bfd158958e39e058f268b697a0b
Author: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

Bluetooth: add a quirk for controllers that cannot enable Secure Connections

This commit introduces HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT to skip the
HCI_OP_WRITE_SC_SUPPORT command on controllers that advertise BR/EDR Secure
Connections in their extended features but reject enabling host support. It
also sets this quirk in the Amlogic Bluetooth driver (hci_aml).

> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 1641d879dbda9..bcbe76c84d1b5 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -375,6 +375,20 @@ enum {
>  	 */
>  	HCI_QUIRK_BROKEN_READ_PAGE_SCAN_TYPE,
>  
> +	/*
> +	 * When this quirk is set, the HCI_OP_WRITE_SC_SUPPORT command is
> +	 * not supported.
> +	 *
> +	 * This quirk is intended for controllers that list BR/EDR Secure
> +	 * Connections among their extended features and then reject the
> +	 * command that turns host support on. Without it the failure takes
> +	 * the whole of hci_dev_open_sync() with it and the controller can
> +	 * never be powered on.
> +	 *
> +	 * This quirk must be set before hci_register_dev is called.
> +	 */
> +	HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT,

[Severity: Low]
Is the comment stating that this quirk must be set before hci_register_dev
is called accurate?

In drivers/bluetooth/hci_aml.c, aml_serdev_probe() registers the HCI device
first via:

  aml_serdev_probe()
    -> hci_uart_register_device()
      -> hci_uart_register_device_priv()
        -> hci_register_dev()

and aml_setup() sets HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT later inside the
hdev->setup callback during hci_dev_open_sync() -> hci_dev_init_sync() ->
hci_dev_setup_sync().

Should this comment note that the quirk can also be set in the hdev->setup
vendor callback?

> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index a92a81846e9df..f1dfef81ca961 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -3419,7 +3419,8 @@ int hci_write_sc_support_sync(struct hci_dev *hdev, u8 val)
>  {
>  	int err;
>  
> -	if (!bredr_sc_enabled(hdev) || lmp_host_sc_capable(hdev))
> +	if (!bredr_sc_enabled(hdev) || lmp_host_sc_capable(hdev) ||
> +	    hci_test_quirk(hdev, HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT))
>  		return 0;
>  
>  	err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_SC_SUPPORT,
> @@ -5040,7 +5041,8 @@ static int hci_write_sc_support_1_sync(struct hci_dev *hdev)
>  	u8 support = 0x01;
>  
>  	if (!hci_dev_test_flag(hdev, HCI_SSP_ENABLED) ||
> -	    !bredr_sc_enabled(hdev))
> +	    !bredr_sc_enabled(hdev) ||
> +	    hci_test_quirk(hdev, HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT))
>  		return 0;

[Severity: High]
When HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT is set, skipping
HCI_OP_WRITE_SC_SUPPORT in hci_write_sc_support_sync() and
hci_write_sc_support_1_sync() leaves LMP_HOST_SC unset on the controller,
while lmp_sc_capable(hdev) and bredr_sc_enabled(hdev) in
include/net/bluetooth/hci_core.h still evaluate to true:

  #define lmp_sc_capable(dev)     ((dev)->features[2][1] & LMP_SC)
  ...
  #define bredr_sc_enabled(dev)   (lmp_sc_capable(dev) && \
                                   hci_dev_test_flag(dev, HCI_SC_ENABLED))

When userspace (such as bluetoothd) issues MGMT_OP_SET_SECURE_CONN,
set_secure_conn() or set_secure_conn_sync() in net/bluetooth/mgmt.c sets
HCI_SC_ENABLED:

  set_secure_conn_sync()
    hci_dev_set_flag(hdev, HCI_SC_ENABLED);
    return hci_write_sc_support_sync(hdev, val);

Because hci_write_sc_support_sync() returns 0 under the quirk,
set_secure_conn_complete() keeps HCI_SC_ENABLED (and HCI_SC_ONLY if
requested) set, leaving bredr_sc_enabled(hdev) true even though BR/EDR
Secure Connections was never enabled on the controller.

Won't this cause callers of lmp_sc_capable() and bredr_sc_enabled() to
misbehave on BR/EDR links? For example:

- In net/bluetooth/mgmt.c, read_local_oob_data_sync(),
  read_local_oob_data_complete(), and read_local_oob_ext_data_complete()
  check bredr_sc_enabled(hdev) and issue HCI_OP_READ_LOCAL_OOB_EXT_DATA
  instead of HCI_OP_READ_LOCAL_OOB_DATA.

- In net/bluetooth/hci_event.c, bredr_oob_data_present() and
  hci_remote_oob_data_request_evt() check bredr_sc_enabled(hdev) and report
  P-256 OOB data or send HCI_OP_REMOTE_OOB_EXT_DATA_REPLY instead of
  HCI_OP_REMOTE_OOB_DATA_REPLY.

- In net/bluetooth/smp.c and net/bluetooth/l2cap_core.c, smp_register() and
  l2cap_conn_add() check bredr_sc_enabled(hdev) and advertise
  L2CAP_FC_SMP_BREDR to remote BR/EDR peers. When a peer initiates SMP
  pairing / Cross-Transport Key Derivation over BR/EDR,
  smp_cmd_pairing_req() rejects it with SMP_CROSS_TRANSP_NOT_ALLOWED
  because HCI_CONN_AES_CCM is not set on the link, and smp_recv_cb()
  disconnects the ACL connection with HCI_ERROR_AUTH_FAILURE.

- In net/bluetooth/mgmt.c and net/bluetooth/hci_debugfs.c,
  get_supported_settings(), set_secure_conn(), and
  hci_debugfs_create_bredr() check lmp_sc_capable(hdev) and treat BR/EDR
  Secure Connections as supported.

Should lmp_sc_capable(dev) in include/net/bluetooth/hci_core.h instead mask
out the feature bit when the quirk is set (similar to how le_coded_capable()
checks HCI_QUIRK_BROKEN_LE_CODED), so that both lmp_sc_capable() and
bredr_sc_enabled() evaluate to false across the stack while still allowing
HCI_SC_ENABLED for LE Secure Connections?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025192338.209574-1-christian.koever-draxl@student.uibk.ac.at?part=1

Re: [PATCH 3/7] Bluetooth: hci_aml: cope with a controller that is already running

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Bluetooth: hci_aml: aml_fw_already_running() timeout stalls retries
- [High] Bluetooth: hci_aml: aml_resync() hangs in serdev_device_write()
- [Medium] Bluetooth: hci_aml: unbalanced power state in aml_setup() and aml_power_on()
--

commit 603cd1d2f458c40daa60bef1f089a3a6efc3085b
Author: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

Bluetooth: hci_aml: cope with a controller that is already running

This commit updates the Amlogic Bluetooth HCI UART driver to detect
already-running controller firmware via HCI_OP_READ_LOCAL_VERSION, retry
unanswered firmware download segments after sending UART resync bytes,
increase firmware startup polling delays, and keep the controller powered
across HCI close until driver removal or shutdown.

> diff --git a/drivers/bluetooth/hci_aml.c b/drivers/bluetooth/hci_aml.c
> index 5979b114f2e4f..8561618867e83 100644
> --- a/drivers/bluetooth/hci_aml.c
> +++ b/drivers/bluetooth/hci_aml.c

[ ... ]

> @@ -214,11 +238,33 @@ static int aml_send_firmware_segment(struct hci_dev *hdev,
>  			     op_addr, (u32 *)seg, seg_size);
>  }
>  
> -static int aml_send_firmware(struct hci_dev *hdev, u8 fw_type,
> -			     u8 *fw, u32 fw_size, u32 offset)
> +static const u8 aml_resync_pattern[256];
> +
> +/* A command that goes unanswered has usually been half received: the
> + * controller took what did arrive for the start of a packet and waits for
> + * the rest, so every following command is swallowed as its payload. Send
> + * enough bytes to complete whatever it is reassembling. Zero is not a valid
> + * H4 packet type, so what is left over afterwards is discarded.
> + */
> +static void aml_resync(struct hci_uart *hu, unsigned int chunks)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < chunks; i++)
> +		serdev_device_write(hu->serdev, aml_resync_pattern,
> +				    sizeof(aml_resync_pattern),
> +				    MAX_SCHEDULE_TIMEOUT);
> +
> +	serdev_device_wait_until_sent(hu->serdev, 0);
> +}

[Severity: High]
Can calling serdev_device_write() with MAX_SCHEDULE_TIMEOUT in aml_resync()
hang the hdev->req_workqueue worker thread when a firmware segment retry is
triggered via aml_setup() -> aml_setup_controller() ->
aml_download_firmware() -> aml_send_firmware() -> aml_resync()?

In drivers/tty/serdev/core.c:serdev_device_write(), if the controller's
write_buf() callback accepts fewer than count bytes, it waits for
serdev->write_comp:

drivers/tty/serdev/core.c:serdev_device_write() {
	mutex_lock(&serdev->write_lock);
	do {
		reinit_completion(&serdev->write_comp);

		ret = ctrl->ops->write_buf(ctrl, buf, count);
		if (ret < 0)
			break;

		written += ret;
		buf += ret;
		count -= ret;

		if (count == 0)
			break;

		timeout = wait_for_completion_interruptible_timeout(&serdev->write_comp,
								    timeout);
	} while (timeout > 0);
	mutex_unlock(&serdev->write_lock);
}

serdev_device_write() requires the serdev client's write_wakeup callback to
call serdev_device_write_wakeup() in order to complete serdev->write_comp.
However, hci_serdev registers hci_uart_write_wakeup() in
drivers/bluetooth/hci_serdev.c, which only calls hci_uart_tx_wakeup(hu) and
never calls serdev_device_write_wakeup(serdev):

drivers/bluetooth/hci_serdev.c:hci_uart_write_wakeup() {
	...
	if (test_bit(HCI_UART_PROTO_READY, &hu->flags))
		hci_uart_tx_wakeup(hu);
}

Because aml_resync() runs on a kernel workqueue thread with
MAX_SCHEDULE_TIMEOUT and serdev->write_comp is never completed, wouldn't any
partial write in serdev_device_write() block forever while holding
serdev->write_lock, as well as bypass hu->write_work without synchronizing
with any in-flight hu->tx_skb?

[ ... ]

> @@ -242,6 +302,35 @@ static int aml_send_firmware(struct hci_dev *hdev, u8 fw_type,
>  	return 0;
>  }
>  
> +/* Dropping the enable line does not reset these controllers: they keep
> + * running the firmware they were last given, at the baud rate they were last
> + * told to use. A warm reboot or a driver reload therefore finds an HCI device
> + * at the operational rate that no longer understands TCI commands. Read Local
> + * Version tells the two cases apart: the ROM does not answer it, running
> + * firmware does.
> + */
> +static bool aml_fw_already_running(struct hci_dev *hdev, struct hci_uart *hu)
> +{
> +	struct sk_buff *skb;
> +	int i;
> +
> +	serdev_device_set_baudrate(hu->serdev, hu->proto->oper_speed);
> +
> +	for (i = 0; i < AML_RESYNC_RETRIES; i++) {
> +		skb = __hci_cmd_sync_ev(hdev, HCI_OP_READ_LOCAL_VERSION, 0,
> +					NULL, HCI_EV_CMD_COMPLETE,
> +					AML_PROBE_TIMEOUT);
> +		if (!IS_ERR(skb)) {
> +			kfree_skb(skb);
> +			return true;
> +		}
> +	}
> +
> +	serdev_device_set_baudrate(hu->serdev, hu->proto->init_speed);
> +
> +	return false;
> +}

[Severity: High]
When the first __hci_cmd_sync_ev() call in aml_fw_already_running() times out
after AML_PROBE_TIMEOUT (500 ms), are the remaining retry iterations actually
transmitted over the UART?

On the first iteration (i = 0), hci_cmd_work() dequeues the command SKB,
calls hci_send_cmd_sync() which decrements hdev->cmd_cnt to 0, and arms
hdev->cmd_timer for HCI_CMD_TIMEOUT (2000 ms):

net/bluetooth/hci_core.c:hci_cmd_work() {
	if (atomic_read(&hdev->cmd_cnt)) {
		skb = skb_dequeue(&hdev->cmd_q);
		if (!skb)
			return;

		err = hci_send_cmd_sync(hdev, skb);
		if (err)
			return;

		rcu_read_lock();
		if (test_bit(HCI_RESET, &hdev->flags) ||
		    hci_dev_test_flag(hdev, HCI_CMD_DRAIN_WORKQUEUE))
			cancel_delayed_work(&hdev->cmd_timer);
		else
			queue_delayed_work(hdev->workqueue, &hdev->cmd_timer,
					   HCI_CMD_TIMEOUT);
		rcu_read_unlock();
	}
}

When the controller does not reply within 500 ms,
wait_event_interruptible_timeout() in
net/bluetooth/hci_sync.c:__hci_cmd_sync_sk() returns -ETIMEDOUT without
resetting hdev->cmd_cnt or canceling hdev->cmd_timer.

On iterations i = 1, 2, and 3, __hci_cmd_sync_ev() queues three more
HCI_OP_READ_LOCAL_VERSION SKBs onto hdev->cmd_q and schedules hdev->cmd_work,
but hci_cmd_work() sees atomic_read(&hdev->cmd_cnt) == 0 and returns without
sending them. At 2000 ms (4 * 500 ms == HCI_CMD_TIMEOUT), hci_cmd_timeout()
fires, cancels the final wait, sets hdev->cmd_cnt back to 1, and schedules
hdev->cmd_work:

net/bluetooth/hci_core.c:hci_cmd_timeout() {
	...
	atomic_set(&hdev->cmd_cnt, 1);
	queue_work(hdev->workqueue, &hdev->cmd_work);
}

Doesn't this prevent retries 1..3 from ever being sent on the wire while
probing at oper_speed, and leave three unsent HCI_OP_READ_LOCAL_VERSION SKBs
in hdev->cmd_q that get transmitted asynchronously after
aml_fw_already_running() has already restored init_speed and returned false?

[ ... ]

> @@ -508,6 +597,7 @@ static int aml_power_on(struct aml_serdev *amldev)
>  	}
>  
>  	gpiod_set_value_cansleep(amldev->bt_en_gpio, 1);
> +	amldev->powered = true;
>  
>  	/* Wait 20ms for bluetooth controller power on */
>  	msleep(20);
> @@ -516,6 +606,10 @@ static int aml_power_on(struct aml_serdev *amldev)
>  
>  static int aml_power_off(struct aml_serdev *amldev)
>  {
> +	if (!amldev->powered)
> +		return 0;
> +
> +	amldev->powered = false;
>  	gpiod_set_value_cansleep(amldev->bt_en_gpio, 0);
>  
>  	clk_disable_unprepare(amldev->lpo_clk);
> @@ -568,7 +662,6 @@ static int aml_open(struct hci_uart *hu)
>  
>  static int aml_close(struct hci_uart *hu)
>  {
> -	struct aml_serdev *amldev = serdev_device_get_drvdata(hu->serdev);
>  	struct aml_data *aml_data = hu->priv;
>  
>  	skb_queue_purge(&aml_data->txq);
> @@ -577,7 +670,13 @@ static int aml_close(struct hci_uart *hu)
>  
>  	hu->priv = NULL;
>  
> -	return aml_power_off(amldev);
> +	/* The controller is left powered. Setup, which powers it on, only
> +	 * runs once per registration, and dropping the enable line does not
> +	 * reset it but leaves it unreachable until the combo chip is power
> +	 * cycled, so powering off here would lose the controller on the next
> +	 * open. It is powered off on remove and shutdown.
> +	 */
> +	return 0;
>  }

[Severity: Medium]
What happens to the enable counts on amldev->bt_supply and amldev->lpo_clk if
aml_setup() fails after aml_power_on(amldev) succeeds, and the HCI device is
subsequently opened again?

In drivers/bluetooth/hci_aml.c:aml_setup():

	err = aml_power_on(amldev);
	if (err)
		return err;

	err = aml_setup_controller(hu, amldev, hdev);
	if (err)
		return err;

Previously, if hci_dev_init_sync() failed during hci_dev_open_sync(), the
error path called hdev->close(hdev) -> aml_close() -> aml_power_off(amldev).
With aml_power_off() removed from aml_close() and not called on error in
aml_setup(), a failure in aml_setup_controller(), aml_send_reset(),
aml_check_bdaddr(), or later in hci_dev_init_sync() leaves amldev->powered
set to true while HCI_SETUP remains set on hdev.

If another open attempt is made (for example via hciconfig up or btmgmt power
on), hci_dev_setup_sync() invokes aml_setup() -> aml_power_on(amldev) again.
Because aml_power_on() does not check amldev->powered before calling
regulator_enable(amldev->bt_supply) and clk_prepare_enable(amldev->lpo_clk),
won't their enable counts be incremented a second time while aml_power_off()
only disables amldev->lpo_clk and amldev->bt_supply once on remove or
shutdown?

Also, unlike aml_serdev_remove() which calls hci_uart_unregister_device()
first, aml_serdev_shutdown() calls aml_power_off(amldev) directly; can that
race with asynchronous aml_setup() work running on hdev->req_workqueue?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025192338.209574-1-christian.koever-draxl@student.uibk.ac.at?part=3

RE: Bluetooth: make the Amlogic W1 (W155S2) usable

[bluez.test.bot]

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

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/series/1193155/

---Test result---

Test Summary:
CheckPatch                    PASS      4.94 seconds
VerifyFixes                   PASS      0.09 seconds
VerifySignedoff               PASS      0.09 seconds
GitLint                       FAIL      1.87 seconds
SubjectPrefix                 FAIL      0.60 seconds
BuildKernel                   PASS      22.55 seconds
CheckAllWarning               PASS      25.46 seconds
CheckSparse                   PASS      28.35 seconds
BuildKernel32                 PASS      22.61 seconds
CheckKernelLLVM               PASS      26.51 seconds
TestRunnerSetup               PASS      520.03 seconds
TestRunner_l2cap-tester       PASS      13.41 seconds
TestRunner_iso-tester         PASS      9.52 seconds
TestRunner_bnep-tester        PASS      2.98 seconds
TestRunner_mgmt-tester        PASS      83.76 seconds
TestRunner_rfcomm-tester      PASS      4.09 seconds
TestRunner_sco-tester         PASS      5.12 seconds
TestRunner_ioctl-tester       PASS      4.23 seconds
TestRunner_mesh-tester        PASS      5.10 seconds
TestRunner_smp-tester         PASS      4.02 seconds
TestRunner_userchan-tester    PASS      2.85 seconds
TestRunner_6lowpan-tester     PASS      4.13 seconds
IncrementalBuild              PASS      42.45 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[1/7] Bluetooth: add a quirk for controllers that cannot enable Secure Connections

1: T1 Title exceeds max length (82>80): "[1/7] Bluetooth: add a quirk for controllers that cannot enable Secure Connections"
##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject
"Bluetooth: " prefix is not specified in the subject


https://github.com/bluez/bluetooth-next/pull/1002

---
Regards,
Linux Bluetooth