Re: [PATCH v6 02/11] Bluetooth: hci_qca: Rename 'power_ctrl_enabled' to 'bt_en_available'

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Renaming 'power_ctrl_enabled' to 'bt_en_available' introduces conceptually flawed and misleading variable semantics that contradict the hardware configuration paths.
--

commit 17c23b3051787bb7fc513728ad005355fa649bd0
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
Subject: Bluetooth: hci_qca: Rename 'power_ctrl_enabled' to 'bt_en_available'

This commit renames the 'power_ctrl_enabled' flag to 'bt_en_available' in
an effort to clarify the availability of the BT_EN GPIO in the devicetree
and avoid naming confusion with the new pwrctrl framework.

> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index c8ea5507bdc2c..f7a20138cb742 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -2396,7 +2396,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  	struct hci_dev *hdev;
>  	const struct qca_device_data *data;
>  	int err;
> -	bool power_ctrl_enabled = true;
> +	bool bt_en_available = true;

[Severity: Medium]
Does renaming this variable to bt_en_available introduce a semantic
contradiction? For devices like QCA_WCN3990 that use UART power pulses
rather than a dedicated BT_EN GPIO, this variable remains true throughout
qca_serdev_probe() even when qcadev->bt_en is NULL.

[ ... ]

> @@ -2504,7 +2504,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  		    (data->soc_type == QCA_WCN6750 ||
>  		     data->soc_type == QCA_WCN6855 ||
>  		     data->soc_type == QCA_WCN7850))
> -			power_ctrl_enabled = false;
> +			bt_en_available = false;

[Severity: Medium]
For SoCs outside of this specific list (like QCA_WCN3990), bt_en_available
remains true here even if qcadev->bt_en is NULL, which contradicts the
variable's new name.

[ ... ]

> @@ -2560,7 +2560,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  
>  	hdev = qcadev->serdev_hu.hdev;
>  
> -	if (power_ctrl_enabled) {
> +	if (bt_en_available) {
>  		hci_set_quirk(hdev, HCI_QUIRK_NON_PERSISTENT_SETUP);
>  		hdev->shutdown = qca_hci_shutdown;
>  	}

[Severity: Medium]
By renaming the flag to bt_en_available, does this falsely imply the GPIO
is actively present? 

The commit message states the flag is used to indicate the availability of
the BT_EN GPIO. However, the flag actually seems to track whether the driver
manages the controller's power state via any mechanism (including UART
in-band pulses or pwrseq). 

Since this flag correctly remains true for SoCs where power control relies
on UART pulses so the shutdown callback is registered (even though
qcadev->bt_en is NULL), could this semantic mismatch lead to a NULL pointer
dereference if a future developer relies on bt_en_available being true as a
safe guarantee to dereference qcadev->bt_en?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-monza-wireless-v6-0-d8c5042b3efd@oss.qualcomm.com?part=2