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

[sashiko-bot@kernel.org]

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