[PATCH v4 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR)

[PATCH v4 0/2] Add API to retrieve the Bluetooth Device Address (BD_ADDR)

[Matthias Kaehlcke]

On some systems the Bluetooth Device Address (BD_ADDR) isn't stored
on the Bluetooth chip itself. One way to configure the address is
through the device tree. The btqcomsmd driver is an example, it can
read the address from the DT property 'local-bd-address'. It is also
planned to extend the hci_qca driver to support setting the BD address
through the DT.

To avoid redundant open-coded reading of 'local-bd-address' and error
handling this series adds an API to retrieve the BD address of a device
and adapts the btqcomsmd driver to use this API.

Matthias Kaehlcke (2):
  Bluetooth: Add device_get_bd_address()
  Bluetooth: btqcomsmd: Get the BD address with device_get_bd_address()

 drivers/bluetooth/btqcomsmd.c     |  4 +---
 include/net/bluetooth/bluetooth.h |  2 ++
 net/bluetooth/lib.c               | 34 +++++++++++++++++++++++++++++++
 3 files changed, 37 insertions(+), 3 deletions(-)

-- 
2.19.0.605.g01d371f741-goog

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

[Matthias Kaehlcke]

Provide an API for Bluetooth drivers to retrieve the Bluetooth Device
address (BD_ADDR) for a device. If the firmware node of the device
has a property 'local-bd-address' the BD address is read from this
property.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Changes in v4:
- move code from driver/base/property.c to net/bluetooth/lib.c
- undo move of bdaddr_t declaration
- merge fwnode_get_bd_address() into device_get_bd_address(). as of now
  the function is not needed, it can be created later if necessary
- minor improvements suggested by Sakari
- updated commit message
- added 'Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>' tag

Changes in v3:
- move definition of bdaddr_t to types.h to avoid include of
  bluetooth.h from property.h
- add stubs for the new functions

Changes in v2:
- use bdaddr_t instead of byte pointer + len
- use EXPORT_SYMBOL_GPL for the new functions instead of EXPORT_SYMBOL
- put new functions inside #if IS_ENABLED(CONFIG_BT)
- some new line juggling in property.h
- added 'Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>' tag
---
 include/net/bluetooth/bluetooth.h |  2 ++
 net/bluetooth/lib.c               | 34 +++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index ec9d6bc65855..6c4cecfda816 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -413,4 +413,6 @@ void mgmt_exit(void);
 
 void bt_sock_reclassify_lock(struct sock *sk, int proto);
 
+int device_get_bd_address(struct device *dev, bdaddr_t *bd_addr);
+
 #endif /* __BLUETOOTH_H */
diff --git a/net/bluetooth/lib.c b/net/bluetooth/lib.c
index 63e65d9b4b24..78a58ea586c6 100644
--- a/net/bluetooth/lib.c
+++ b/net/bluetooth/lib.c
@@ -26,7 +26,10 @@
 
 #define pr_fmt(fmt) "Bluetooth: " fmt
 
+#include <linux/etherdevice.h>
 #include <linux/export.h>
+#include <linux/fwnode.h>
+#include <linux/property.h>
 
 #include <net/bluetooth/bluetooth.h>
 
@@ -198,3 +201,34 @@ void bt_err_ratelimited(const char *format, ...)
 	va_end(args);
 }
 EXPORT_SYMBOL(bt_err_ratelimited);
+
+/**
+ * device_get_bd_address - Get the Bluetooth Device Address (BD_ADDR) for a
+ *                         given device
+ * @dev:	Pointer to the device
+ * @bd_addr:	Pointer to struct to store the BD address in
+ *
+ * Search the firmware node of the device for 'local-bd-address'.
+ *
+ * All-zero BD addresses are rejected, because those could be properties
+ * that exist in the firmware tables, but were not updated by the firmware. For
+ * example, the DTS could define 'local-bd-address', with zero BD addresses.
+ */
+int device_get_bd_address(struct device *dev, bdaddr_t *bd_addr)
+{
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
+	bdaddr_t ba;
+	int ret;
+
+	ret = fwnode_property_read_u8_array(fwnode, "local-bd-address",
+					    (u8 *)&ba, sizeof(ba));
+	if (ret < 0)
+		return ret;
+	if (is_zero_ether_addr((u8 *)&ba))
+		return -ENODATA;
+
+	*bd_addr = ba;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(device_get_bd_address);
-- 
2.19.0.605.g01d371f741-goog

[PATCH v4 2/2] Bluetooth: btqcomsmd: Get the BD address with device_get_bd_address()

[Matthias Kaehlcke]

Use the new API to get the BD address instead of reading it directly
from the device tree.

Also remove an unncessary pair of braces in the same area of code.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Changes in v4:
- added 'Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>' tag

Changes in v3:
- added 'Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>' tag

Changes in v2:
- pass bdaddr_t instead of byte pointer + len
---
 drivers/bluetooth/btqcomsmd.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/bluetooth/btqcomsmd.c b/drivers/bluetooth/btqcomsmd.c
index 7df3eed1ef5e..ff74d2c46991 100644
--- a/drivers/bluetooth/btqcomsmd.c
+++ b/drivers/bluetooth/btqcomsmd.c
@@ -172,11 +172,9 @@ static int btqcomsmd_probe(struct platform_device *pdev)
 	/* The local-bd-address property is usually injected by the
 	 * bootloader which has access to the allocated BD address.
 	 */
-	if (!of_property_read_u8_array(pdev->dev.of_node, "local-bd-address",
-				       (u8 *)&btq->bdaddr, sizeof(bdaddr_t))) {
+	if (!device_get_bd_address(&pdev->dev, &btq->bdaddr))
 		dev_info(&pdev->dev, "BD address %pMR retrieved from device-tree",
 			 &btq->bdaddr);
-	}
 
 	hdev = hci_alloc_dev();
 	if (!hdev)
-- 
2.19.0.605.g01d371f741-goog

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

[Sakari Ailus]

On Wed, Sep 26, 2018 at 05:48:09PM -0700, Matthias Kaehlcke wrote:
> Provide an API for Bluetooth drivers to retrieve the Bluetooth Device
> address (BD_ADDR) for a device. If the firmware node of the device
> has a property 'local-bd-address' the BD address is read from this
> property.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Changes in v4:
> - move code from driver/base/property.c to net/bluetooth/lib.c
> - undo move of bdaddr_t declaration
> - merge fwnode_get_bd_address() into device_get_bd_address(). as of now
>   the function is not needed, it can be created later if necessary
> - minor improvements suggested by Sakari
> - updated commit message
> - added 'Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>' tag
> 
> Changes in v3:
> - move definition of bdaddr_t to types.h to avoid include of
>   bluetooth.h from property.h
> - add stubs for the new functions
> 
> Changes in v2:
> - use bdaddr_t instead of byte pointer + len
> - use EXPORT_SYMBOL_GPL for the new functions instead of EXPORT_SYMBOL
> - put new functions inside #if IS_ENABLED(CONFIG_BT)
> - some new line juggling in property.h
> - added 'Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>' tag
> ---
>  include/net/bluetooth/bluetooth.h |  2 ++
>  net/bluetooth/lib.c               | 34 +++++++++++++++++++++++++++++++

Yeah, this seems like a better place for this stuff indeed.

Thanks!

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

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

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-09-27 06:18, Matthias Kaehlcke wrote:
> Provide an API for Bluetooth drivers to retrieve the Bluetooth Device
> address (BD_ADDR) for a device. If the firmware node of the device
> has a property 'local-bd-address' the BD address is read from this
> property.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Changes in v4:
> - move code from driver/base/property.c to net/bluetooth/lib.c
> - undo move of bdaddr_t declaration
> - merge fwnode_get_bd_address() into device_get_bd_address(). as of now
>   the function is not needed, it can be created later if necessary
> - minor improvements suggested by Sakari
> - updated commit message
> - added 'Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>' tag
> 
> Changes in v3:
> - move definition of bdaddr_t to types.h to avoid include of
>   bluetooth.h from property.h
> - add stubs for the new functions
> 
> Changes in v2:
> - use bdaddr_t instead of byte pointer + len
> - use EXPORT_SYMBOL_GPL for the new functions instead of EXPORT_SYMBOL
> - put new functions inside #if IS_ENABLED(CONFIG_BT)
> - some new line juggling in property.h
> - added 'Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>' tag
> ---
>  include/net/bluetooth/bluetooth.h |  2 ++
>  net/bluetooth/lib.c               | 34 +++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/include/net/bluetooth/bluetooth.h
> b/include/net/bluetooth/bluetooth.h
> index ec9d6bc65855..6c4cecfda816 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -413,4 +413,6 @@ void mgmt_exit(void);
> 
>  void bt_sock_reclassify_lock(struct sock *sk, int proto);
> 
> +int device_get_bd_address(struct device *dev, bdaddr_t *bd_addr);
> +
>  #endif /* __BLUETOOTH_H */
> diff --git a/net/bluetooth/lib.c b/net/bluetooth/lib.c
> index 63e65d9b4b24..78a58ea586c6 100644
> --- a/net/bluetooth/lib.c
> +++ b/net/bluetooth/lib.c
> @@ -26,7 +26,10 @@
> 
>  #define pr_fmt(fmt) "Bluetooth: " fmt
> 
> +#include <linux/etherdevice.h>
>  #include <linux/export.h>
> +#include <linux/fwnode.h>
> +#include <linux/property.h>
> 
>  #include <net/bluetooth/bluetooth.h>
> 
> @@ -198,3 +201,34 @@ void bt_err_ratelimited(const char *format, ...)
>  	va_end(args);
>  }
>  EXPORT_SYMBOL(bt_err_ratelimited);
> +
> +/**
> + * device_get_bd_address - Get the Bluetooth Device Address (BD_ADDR) 
> for a
> + *                         given device
> + * @dev:	Pointer to the device
> + * @bd_addr:	Pointer to struct to store the BD address in
> + *
> + * Search the firmware node of the device for 'local-bd-address'.
> + *
> + * All-zero BD addresses are rejected, because those could be 
> properties
> + * that exist in the firmware tables, but were not updated by the 
> firmware. For
> + * example, the DTS could define 'local-bd-address', with zero BD 
> addresses.
> + */
> +int device_get_bd_address(struct device *dev, bdaddr_t *bd_addr)
> +{
> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
> +	bdaddr_t ba;
> +	int ret;
> +
> +	ret = fwnode_property_read_u8_array(fwnode, "local-bd-address",
> +					    (u8 *)&ba, sizeof(ba));
> +	if (ret < 0)
> +		return ret;
> +	if (is_zero_ether_addr((u8 *)&ba))
> +		return -ENODATA;
> +
> +	*bd_addr = ba;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(device_get_bd_address);

Reviewed-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>

-- 
Regards
Balakrishna.

Re: [PATCH v4 2/2] Bluetooth: btqcomsmd: Get the BD address with device_get_bd_address()

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-09-27 06:18, Matthias Kaehlcke wrote:
> Use the new API to get the BD address instead of reading it directly
> from the device tree.
> 
> Also remove an unncessary pair of braces in the same area of code.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Changes in v4:
> - added 'Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>' tag
> 
> Changes in v3:
> - added 'Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>' tag
> 
> Changes in v2:
> - pass bdaddr_t instead of byte pointer + len
> ---
>  drivers/bluetooth/btqcomsmd.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqcomsmd.c 
> b/drivers/bluetooth/btqcomsmd.c
> index 7df3eed1ef5e..ff74d2c46991 100644
> --- a/drivers/bluetooth/btqcomsmd.c
> +++ b/drivers/bluetooth/btqcomsmd.c
> @@ -172,11 +172,9 @@ static int btqcomsmd_probe(struct platform_device 
> *pdev)
>  	/* The local-bd-address property is usually injected by the
>  	 * bootloader which has access to the allocated BD address.
>  	 */
> -	if (!of_property_read_u8_array(pdev->dev.of_node, "local-bd-address",
> -				       (u8 *)&btq->bdaddr, sizeof(bdaddr_t))) {
> +	if (!device_get_bd_address(&pdev->dev, &btq->bdaddr))
>  		dev_info(&pdev->dev, "BD address %pMR retrieved from device-tree",
>  			 &btq->bdaddr);
> -	}
> 
>  	hdev = hci_alloc_dev();
>  	if (!hdev)

Reviewed-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>

-- 
Regards
Balakrishna.

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

[Sinan Kaya]

On 9/27/2018 12:41 PM, Balakrishna Godavarthi wrote:
>   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_?

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

[Matthias Kaehlcke]

On Thu, Sep 27, 2018 at 12:47:06PM -0400, Sinan Kaya wrote:
> On 9/27/2018 12:41 PM, Balakrishna Godavarthi wrote:
> >   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.

Cheers

Matthias

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

[Matthias Kaehlcke]

On Thu, Sep 27, 2018 at 10:13:05AM -0700, Matthias Kaehlcke wrote:
> On Thu, Sep 27, 2018 at 12:47:06PM -0400, Sinan Kaya wrote:
> > On 9/27/2018 12:41 PM, Balakrishna Godavarthi wrote:
> > >   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.

Cheers

Matthias

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

[Matthias Kaehlcke]

Hi Marcel,

please let me know if any changes are needed to get this patch applied
to bluetooth-next.

Thanks

Matthias

On Thu, Oct 04, 2018 at 10:33:38AM -0700, Matthias Kaehlcke wrote:
> On Thu, Sep 27, 2018 at 10:13:05AM -0700, Matthias Kaehlcke wrote:
> > On Thu, Sep 27, 2018 at 12:47:06PM -0400, Sinan Kaya wrote:
> > > On 9/27/2018 12:41 PM, Balakrishna Godavarthi wrote:
> > > >   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.
> 
> Cheers
> 
> Matthias

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

[Marcel Holtmann]

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? I am failing to see the benefit if this is not part of the property.h API.

Regards

Marcel

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

[Matthias Kaehlcke]

On Mon, Oct 15, 2018 at 08:06:02PM +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.

Thanks

Matthias

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

[Marcel Holtmann]

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

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

[Matthias Kaehlcke]

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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2212 bytes --]

Hi!

> >> 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.

Can you wrap lines at around 72 characters in the emails?

We do ethernet addresses via DT. I don't know if doing that in
bootloader is a hack or not, but if bootloader is already doing that
for ethernet, bluettoth address in DT kind of makes sense.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]