Hi Marcel, On Tue, Oct 16, 2018 at 08:52:07AM +0200, Marcel Holtmann wrote: > 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. Agreed, it would be better if this can be handled in the Bluetooth core. > 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. Yes, the DT approach relies on the bootloader which isn't ideal. Part of the problem is that AFAIK there is currently no standard way for storing/retrieving persistent, board specific values like BD ADDR, so different custom mechanisms are used, which tend to be incompatible with each other (e.g. Chrome OS uses VPD: https://chromium.googlesource.com/chromiumos/platform/vpd/) Using the bootloader & DT is a pragmatic approach, since the DT is well established and the bootloader often already has DT awareness. That said I agree that a common solution would make our lives easier. > 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. Thanks for proposing this generic alternative solution and providing details! I'm not really experienced with hacking the Bluetooth core and don't understand your proposal entirely: I get the part of setting the quirk in the driver, checking for it in hci_dev_do_open(), reading the address from 'local-bd-address', setting it with hdev->bd_addr and marking the device as unconfigured if the address is empty/unavailable. I interpret that you suggest that 'local-bd-address' should only be used if user space doesn't provide a BD_ADDR. It is not evident to me where a user space provided address is set, in any case it doesn't seem to be in hci_dev_do_open(), my uneducated guess is that the address is set with the management command SET_PUBLIC_ADDRESS. Could you clarify on this? I also wonder how to identify the DT node corresponding to an HCI device, for hci_qca it's the node of hdev->dev.parent, but I'm not sure if that is universally true. If it isn't looking for the first parent with a DT node could be an option. Thanks Matthias
