Re: [PATCH v4 1/2] Bluetooth: Add device_get_bd_address()

[Marcel Holtmann <marcel@holtmann.org>]

Hi Matthias,

>>>>>> void bt_sock_reclassify_lock(struct sock *sk, int proto);
>>>>>> 
>>>>>> +int device_get_bd_address(struct device *dev, bdaddr_t *bd_addr);
>>>>> 
>>>>> Maybe change the API name to start with bt_ and get rid of device_?
>>>> 
>>>> device_ indicates that we get the BD_ADDR for a 'struct device' and
>>>> not for e.g. a 'struct fwnode_handle'.
>>>> 
>>>> Anyway with this version of the patch fwnode_get_bd_address() has been
>>>> scrapped and it might never be introduced again, so I'm open to change
>>>> the name to bt_ if there is a general preference for it.
>>> 
>>> Marcel, can you live with this being added to the Bluetooth code base
>>> instead of property? Also if you'd prefer the function to be named
>>> bt_get_bd_address() let me know.
>> 
>> explain to me again why this is useful?
> 
> The official binding for providing the BD_ADDR through the device tree
> is the property 'local-bd-address'. device_get_bd_address() provides a
> common API to retrieve the BD_ADDR instead of requiring BT drivers to
> use the lower level fwnode_property_read_u8_array(). It also avoids
> repeating the check for an all zeroes BD_ADDR in multiple drivers.
> 
>> I am failing to see the benefit if this is not part of the property.h API.
> 
> My understanding is that the intention of property.h it to provide an
> API for common property types used by drivers from different
> subsystems, hence the implementation 'lives' in drivers/base.
> Obtaining the BD_ADDR is clearly limited to the Bluetooth subsystem,
> and drivers/base doesn't seem to be the right place for it. It's true,
> device_get_mac_address() lives in the common property code, but that
> doesn't necessarily mean it really should be there and we should do
> the same. I agree with Sakari that the the approach taken by V4L2
> (drivers/media/v4l2-core/v4l2-fwnode.c) seems more appropriate.
> 
> That said I wouldn't raise opposition if the maintainers of
> drivers/base agreed to add device_get_mac_address() there, however so
> far several recent authors of property.[ch] have raised objections.

so if this is not in drivers/base/ then what is the point in making each driver do this? If it is a common property, then it can be well handled in the Bluetooth core when setting up the hardware.

This whole BD_ADDR via DT is stupid anyway. Just so that is clear up-front. It has been a total hack and fully relies on boot loaders doing too much magic and then using DT to hide this magic. The BD_ADDR is required to be unique and that means no user will ever create a DT with that set. The boot loader always has to read some magic value and then convert it and merge it into the actual DT provided to the kernel. The clean part would be just to have proper APIs to read the memory of the persistent / programmed BD_ADDR and then access that.

That all said, we have hdev->set_bdaddr address and the HCI_QUIRK_INVALID_BDADDR to mark the controller as not fully set up. And then actually user space can deal with getting the correct address and providing it. The code is already there that handles all of this if the BD_ADDR comes from user space. Actually hacking this into the driver and doing that in the hdev->setup callback is quirky to begin with. A user space provided address will just overwrite that.

If you really want to make this generic, then introduce HCI_QUIRK_USE_BDADDR_PROPERTY that a driver can set and then do that all in hci_dev_do_open() so that if no user space provided BD_ADDR is available, it is read from local-bt-address property and if that is not available or empty, then mark the the device as unconfigured.

I am intentionally saying unconfigured when you set HCI_QUIRK_USE_BDADDR_PROPERTY since I assume that the logic that we have behind HCI_QUIRK_INVALID_BDADDR is implied and whatever address comes back via Read_BD_Address is invalid. Otherwise this hardware should not set HCI_QUIRK_USE_BDADDR_PROPERTY at all.


Regards

Marcel