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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Reading the NVMEM cell asynchronously in `hci_dev_setup_sync` defeats `-EPROBE_DEFER` dependency management, potentially leaving the device permanently unconfigured.
--

--- Patch [8]: [PATCH v10 08/10] Bluetooth: hci_sync: Add NVMEM-backed BD address retrieval ---
commit da997894066d6b3f7aceb681048c25113a6d107e
Author: Loic Poulain <[email protected]>

Bluetooth: hci_sync: Add NVMEM-backed BD address retrieval

This commit adds support for reading the Bluetooth BD address from a
'local-bd-address' NVMEM cell, similar to Ethernet or WiFi MAC addresses.
It also adds quirks to indicate if a device should use this method and
whether the address is stored in big-endian order.

> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index c8d14128c363d..8132a1487ac25 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c

[ ... ]

> @@ -3701,6 +3702,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;

[ ... ]

> @@ -5180,12 +5214,17 @@ static int hci_dev_setup_sync(struct hci_dev *hdev)
>  	 * its setup callback.
>  	 */
>  	invalid_bdaddr = hci_test_quirk(hdev, HCI_QUIRK_INVALID_BDADDR) ||
> -			 hci_test_quirk(hdev, HCI_QUIRK_USE_BDADDR_PROPERTY);
> +			 hci_test_quirk(hdev, HCI_QUIRK_USE_BDADDR_PROPERTY) ||
> +			 hci_test_quirk(hdev, HCI_QUIRK_USE_BDADDR_NVMEM);
>  	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: High]
Does ignoring the return value here defeat probe deferral?

Since hci_dev_setup_sync() runs during device open rather than synchronously
in the driver's probe() function, any -EPROBE_DEFER error returned by
of_get_nvmem_eui48() inside hci_dev_get_bd_addr_from_nvmem() cannot be
propagated to the driver core's deferral queue.

Additionally, because the return value is completely dropped, does this
silently ignore permanent hardware errors like -EIO?

If the NVMEM cell is missing, not fully bound yet, or fails to read, it
appears the device could silently fail to acquire its BD_ADDR and
permanently boot in an unconfigured state without any log message.

> +
>  		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/[email protected]?part=8