linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Balakrishna Godavarthi <bgodavar@codeaurora.org>
To: Matthias Kaehlcke <mka@chromium.org>
Cc: Marcel Holtmann <marcel@holtmann.org>,
	Johan Hedberg <johan.hedberg@gmail.com>,
	"David S . Miller" <davem@davemloft.net>,
	Loic Poulain <loic.poulain@linaro.org>,
	linux-bluetooth@vger.kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Brian Norris <briannorris@chromium.org>,
	Dmitry Grinberg <dmitrygr@google.com>,
	hemantg@codeaurora.org
Subject: Re: [PATCH 1/2] Bluetooth: Add quirk for reading BD_ADDR from fwnode property
Date: Fri, 26 Oct 2018 10:31:37 +0530	[thread overview]
Message-ID: <7462a1b91c84454290eb09ff33bee8ee@codeaurora.org> (raw)
In-Reply-To: <4041ef05cdd70d28d665d3288c4d4c43@codeaurora.org>

Hi Matthias,

I missed to add a point here.

On 2018-10-25 20:06, Balakrishna Godavarthi wrote:
> On 2018-10-25 05:51, Matthias Kaehlcke wrote:
>> Add HCI_QUIRK_USE_BDADDR_PROPERTY to allow controllers to retrieve
>> the public Bluetooth address from the firmware node property
>> 'local-bd-address'. If quirk is set and the property does not exist
>> or is invalid the controller is marked as unconfigured.
>> 
>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>> hci_dev_get_bd_addr_from_property() currently assumes that the
>> firmware node with 'local-bd-address' is from hdev->dev.parent, not
>> sure if this universally true. However if it is true for existing
>> device that might use this interface we can assume this for now
>> (unless there is a clear solution now), and cross the bridge of
>> finding an alternative when we actually encounter the situation.
>> One option could be to look for the first parent that has a fwnode.
>> ---
>>  include/net/bluetooth/hci.h | 12 +++++++++++
>>  net/bluetooth/hci_core.c    | 42 
>> +++++++++++++++++++++++++++++++++++++
>>  net/bluetooth/mgmt.c        |  6 ++++--
>>  3 files changed, 58 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>> index cdd9f1fe7cfa..a5d748099752 100644
>> --- a/include/net/bluetooth/hci.h
>> +++ b/include/net/bluetooth/hci.h
>> @@ -158,6 +158,18 @@ enum {
>>  	 */
>>  	HCI_QUIRK_INVALID_BDADDR,
>> 
>> +	/* When this quirk is set, the public Bluetooth address
>> +	 * initially reported by HCI Read BD Address command
>> +	 * is considered invalid. The public BD Address can be
>> +	 * specified in the fwnode property 'local-bd-address'.
>> +	 * If this property does not exist or is invalid controller
>> +	 * configuration is required before this device can be used.
>> +	 *
>> +	 * This quirk can be set before hci_register_dev is called or
>> +	 * during the hdev->setup vendor callback.
>> +	 */
>> +	HCI_QUIRK_USE_BDADDR_PROPERTY,
>> +
>>  	/* When this quirk is set, the duplicate filtering during
>>  	 * scanning is based on Bluetooth devices addresses. To allow
>>  	 * RSSI based updates, restart scanning if needed.
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 74b29c7d841c..97214262c4fb 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -30,6 +30,7 @@
>>  #include <linux/rfkill.h>
>>  #include <linux/debugfs.h>
>>  #include <linux/crypto.h>
>> +#include <linux/property.h>
>>  #include <asm/unaligned.h>
>> 
>>  #include <net/bluetooth/bluetooth.h>
>> @@ -1355,9 +1356,40 @@ int hci_inquiry(void __user *arg)
>>  	return err;
>>  }
>> 
>> +/**
>> + * hci_dev_get_bd_addr_from_property - Get the Bluetooth Device 
>> Address
>> + *				       (BD_ADDR) for a HCI device from
>> + *				       a firmware node property.
>> + * @hdev:	The HCI device
>> + *
>> + * Search the firmware node 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.
>> + */
>> +static int hci_dev_get_bd_addr_from_property(struct hci_dev *hdev)
>> +{
>> +	struct fwnode_handle *fwnode = dev_fwnode(hdev->dev.parent);
>> +	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 (!bacmp(&ba, BDADDR_ANY))
>> +		return -ENODATA;
>> +
>> +	hdev->public_addr = ba;
>> +
>> +	return 0;
>> +}
>> +
>>  static int hci_dev_do_open(struct hci_dev *hdev)
>>  {
>>  	int ret = 0;
>> +	bool bd_addr_set = false;
>> 
>>  	BT_DBG("%s %p", hdev->name, hdev);
>> 
>> @@ -1422,6 +1454,16 @@ static int hci_dev_do_open(struct hci_dev 
>> *hdev)
>>  		if (hdev->setup)
>>  			ret = hdev->setup(hdev);
>> 
>> +		if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) {
>> +			if (!hci_dev_get_bd_addr_from_property(hdev))
>> +				if (hdev->set_bdaddr &&
>> +				    !hdev->set_bdaddr(hdev, &hdev->public_addr))
>> +					bd_addr_set = true;

Can we check the return status of hdev->setup() before calling 
hdev->set_bdaddr().
some vendors assign hdev->set_baddr helper before calling hdev->setup().
https://elixir.bootlin.com/linux/v4.19-rc8/source/drivers/bluetooth/btqcomsmd.c#L194
There will no use in calling hdev->set_baddr() if hdev->setup() fails.

>> +
>> +			if (!bd_addr_set)
>> +				hci_dev_set_flag(hdev, HCI_UNCONFIGURED);
>> +		}
>> +
>>  		/* The transport driver can set these quirks before
>>  		 * creating the HCI device or in its setup callback.
>>  		 *
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index 3bdc8f3ca259..3d9edb752403 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -551,7 +551,8 @@ static bool is_configured(struct hci_dev *hdev)
>>  	    !hci_dev_test_flag(hdev, HCI_EXT_CONFIGURED))
>>  		return false;
>> 
>> -	if (test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks) &&
>> +	if ((test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks) ||
>> +	     test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) &&
>>  	    !bacmp(&hdev->public_addr, BDADDR_ANY))
>>  		return false;
>> 
>> @@ -566,7 +567,8 @@ static __le32 get_missing_options(struct hci_dev 
>> *hdev)
>>  	    !hci_dev_test_flag(hdev, HCI_EXT_CONFIGURED))
>>  		options |= MGMT_OPTION_EXTERNAL_CONFIG;
>> 
>> -	if (test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks) &&
>> +	if ((test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks) ||
>> +	     test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) &&
>>  	    !bacmp(&hdev->public_addr, BDADDR_ANY))
>>  		options |= MGMT_OPTION_PUBLIC_ADDRESS;
> 
> Looks fine to me.
> 
> Reviewed-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>

-- 
Regards
Balakrishna.

  reply	other threads:[~2018-10-26  5:01 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-25  0:21 [PATCH 0/2] " Matthias Kaehlcke
2018-10-25  0:21 ` [PATCH 1/2] Bluetooth: " Matthias Kaehlcke
2018-10-25 14:36   ` Balakrishna Godavarthi
2018-10-26  5:01     ` Balakrishna Godavarthi [this message]
2018-10-26 17:58       ` Matthias Kaehlcke
2018-10-25  0:21 ` [PATCH 2/2] Bluetooth: btqcomsmd: use HCI_QUIRK_USE_BDADDR_PROPERTY Matthias Kaehlcke
2018-10-25 14:40   ` Balakrishna Godavarthi
2018-10-25 19:03     ` Matthias Kaehlcke

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7462a1b91c84454290eb09ff33bee8ee@codeaurora.org \
    --to=bgodavar@codeaurora.org \
    --cc=briannorris@chromium.org \
    --cc=davem@davemloft.net \
    --cc=dmitrygr@google.com \
    --cc=hemantg@codeaurora.org \
    --cc=johan.hedberg@gmail.com \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=loic.poulain@linaro.org \
    --cc=marcel@holtmann.org \
    --cc=mka@chromium.org \
    --cc=netdev@vger.kernel.org \
    --subject='Re: [PATCH 1/2] Bluetooth: Add quirk for reading BD_ADDR from fwnode property' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).