Re: [PATCH v9 08/10] Bluetooth: hci_sync: Add NVMEM-backed BD address retrieval

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Fetching NVMEM cells asynchronously in `hci_dev_setup_sync()` bypasses the kernel's probe deferral mechanism, causing non-deterministic hardware initialization failures.
--

--- Patch [8]: [PATCH v9 08/10] Bluetooth: hci_sync: Add NVMEM-backed BD address retrieval ---
commit 72e4410658718fb2db8f711db8f8fe59033dd650
Author: Loic Poulain <loic.poulain@oss.qualcomm.com>

Bluetooth: hci_sync: Add NVMEM-backed BD address retrieval

This commit adds support for retrieving the Bluetooth BD address from an
NVMEM cell when an in-chip address is not present. It introduces two new
quirks to indicate when to use the NVMEM address and its endianness.

> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index c0b1fc293b496..ff395bf14dbc3 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
[ ... ]
> @@ -3638,6 +3639,39 @@ int hci_powered_update_sync(struct hci_dev *hdev)
>  	return 0;
>  }
>  
> +/**
> + * hci_dev_get_bd_addr_from_nvmem - Get the Bluetooth Device Address
> + *				    (BD_ADDR) for a HCI device from
> + *				    an NVMEM cell.
> + * @hdev:	The HCI device
> + *
> + * Search for 'local-bd-address' NVMEM cell in the device firmware node.
> + *
> + * All-zero BD addresses are rejected (unprovisioned).
> + *
> + * Return: 0 on success, or a negative error code on failure.
> + */
> +static int hci_dev_get_bd_addr_from_nvmem(struct hci_dev *hdev)
> +{
> +	struct device_node *np = dev_of_node(hdev->dev.parent);
> +	u8 ba[sizeof(bdaddr_t)];
> +	int err;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	err = of_get_nvmem_eui48(np, "local-bd-address", ba);
> +	if (err)
> +		return err;

[Severity: Medium]
Will this return value be handled correctly if the NVMEM provider has not
probed yet?

When of_get_nvmem_eui48() returns -EPROBE_DEFER, the error is correctly
passed up from this function. However, looking at the caller below, it seems
this might not trigger probe deferral as expected.

> +
> +	if (hci_test_quirk(hdev, HCI_QUIRK_BDADDR_NVMEM_BE))
> +		baswap(&hdev->public_addr, (bdaddr_t *)ba);
> +	else
> +		bacpy(&hdev->public_addr, (bdaddr_t *)ba);
> +
> +	return 0;
> +}
[ ... ]
> @@ -5117,12 +5151,17 @@ static int hci_dev_setup_sync(struct hci_dev *hdev)
[ ... ]
>  	if (!ret) {
>  		if (hci_test_quirk(hdev, HCI_QUIRK_USE_BDADDR_PROPERTY) &&
>  		    !bacmp(&hdev->public_addr, BDADDR_ANY))
>  			hci_dev_get_bd_addr_from_property(hdev);
>  
> +		if (hci_test_quirk(hdev, HCI_QUIRK_USE_BDADDR_NVMEM) &&
> +		    !bacmp(&hdev->public_addr, BDADDR_ANY))
> +			hci_dev_get_bd_addr_from_nvmem(hdev);

[Severity: Medium]
Does this silently swallow initialization failures?

The return value from hci_dev_get_bd_addr_from_nvmem() is completely ignored
here. If the NVMEM provider is not yet available, the -EPROBE_DEFER error
is lost and the Bluetooth controller might be left in an unconfigured state.

Additionally, since hci_dev_setup_sync() often executes asynchronously outside
of the standard driver probe sequence, would returning the error here even
properly trigger the kernel probe deferral mechanism?

Should the NVMEM cell be fetched synchronously during the initial device probe
sequence instead of waiting for the setup synchronization phase?

> +
>  		if (invalid_bdaddr && bacmp(&hdev->public_addr, BDADDR_ANY) &&
>  		    hdev->set_bdaddr) {
>  			ret = hdev->set_bdaddr(hdev, &hdev->public_addr);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-block-as-nvmem-v9-0-f72935817dbf@oss.qualcomm.com?part=8