Re: [PATCH bluetooth-next v5 3/3] Bluetooth: btaic: Add AIC8800D80 SDIO loader and UART transport

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] bluetooth: btaic: forcible override of SDIO CCCR IEN corrupts bus state
- [Medium] bluetooth: btaic: permanent TX queue wedge on UART transmission error
--

commit 0b3f4967899ed87ce6d0daef5c0ff2b9f67d8faf
Author: Yanli Yang <yanli.yang@bedmex.com>

Bluetooth: btaic: Add AIC8800D80 SDIO loader and UART transport

This commit adds a driver for the AIC8800D80 combination chip. It uses
SDIO function 1 for Bluetooth firmware loading and a serdev UART for H4
HCI traffic. Firmware loading and patching are synchronized with UART
initialization via a reference-counted boot state.

> diff --git a/drivers/bluetooth/btaic_sdio.c b/drivers/bluetooth/btaic_sdio.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/bluetooth/btaic_sdio.c
[ ... ]
> +static int aic_bt_sdio_hw_init(struct aic_bt_sdio *btdev)
> +{
> +	struct sdio_func *func = btdev->func;
> +	struct mmc_host *host = func->card->host;
> +	u8 io_control;
> +	int err;
[ ... ]
> +	err = sdio_claim_irq(func, aic_bt_sdio_irq);
> +	if (err)
> +		goto disable_func;
> +	btdev->irq_claimed = true;
> +
> +	sdio_f0_writeb(func, 0x07, 0x04, &err);

[Severity: High]
Does this hardcoded write to the CCCR IEN register (0x04) bypass the MMC 
core's interrupt tracking? 

Since sdio_claim_irq() was already called to properly enable the master and 
Function 1 interrupts, writing 0x07 forcibly enables the Function 2 (WiFi) 
interrupt outside of the MMC core's knowledge. Could this lead to unhandled 
interrupt storms or disrupt the WiFi function if its driver is not yet ready?

> +	if (err)
> +		goto release_irq;
> +
> +	sdio_writeb(func, 0x07, AIC_SDIO_INTR_ENABLE, &err);
> +	if (err)
> +		goto release_irq;
> +
> +	sdio_release_host(func);
> +	return 0;
[ ... ]
> diff --git a/drivers/bluetooth/btaic_uart.c b/drivers/bluetooth/btaic_uart.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/bluetooth/btaic_uart.c
[ ... ]
> +static void aic_bt_tx_work(struct work_struct *work)
> +{
> +	struct aic_bt_uart *uart = container_of(work, struct aic_bt_uart,
> +						 tx_work);
> +
> +	for (;;) {
> +		struct sk_buff *skb;
> +
> +		clear_bit(AIC_BT_TX_WAKEUP, &uart->tx_state);
> +
> +		while ((skb = skb_dequeue(&uart->txq))) {
> +			int len;
> +
> +			len = serdev_device_write_buf(uart->serdev, skb->data,
> +						      skb->len);
> +			if (len <= 0) {
> +				skb_queue_head(&uart->txq, skb);
> +				if (len < 0)
> +					bt_dev_err(uart->hdev,
> +						   "UART transmit failed (%d)", len);
> +				break;
> +			}

[Severity: Medium]
Will this create a permanent wedge in the TX queue on a transmission error?

If serdev_device_write_buf() returns a negative error code (such as -EIO 
or -ENODEV) due to local hardware or bus errors, the driver treats it 
identically to a full buffer by pushing the exact same skb back to the 
head of uart->txq.

On the next TX wakeup, it appears it will dequeue this same packet, 
fail again, and requeue it indefinitely, which would effectively block 
all subsequent outbound Bluetooth traffic rather than gracefully dropping 
the failed packet.

> +
> +			uart->hdev->stat.byte_tx += len;
> +			skb_pull(skb, len);
> +			if (skb->len) {
> +				skb_queue_head(&uart->txq, skb);
> +				break;
> +			}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790235697.git.yanli.yang@bedmex.com?part=3