linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Matthias Kaehlcke <mka@chromium.org>
To: Balakrishna Godavarthi <bgodavar@codeaurora.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:58:16 -0700	[thread overview]
Message-ID: <20181026175816.GC22824@google.com> (raw)
In-Reply-To: <7462a1b91c84454290eb09ff33bee8ee@codeaurora.org>

On Fri, Oct 26, 2018 at 10:31:37AM +0530, Balakrishna Godavarthi wrote:
> 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.

Thanks for pointing this out, I'll add the check.

This is more a question for Marcel: independently from this change I
wonder how robust the error flow in this function is. Is there are
reason to not bail out directly when a seemingly vital function like
->setup() fails, and instead continue and potentially overwrite the
error code? And there are other similar patterns in hci_dev_do_open().

Bailing out would certainly add a bit more code and probably gotos to
a cleanup section (currently in the else branch at the bottom of the
function), but might improve readability and robustness (I don't claim
there is an actual problem, but overwriting the error code seems
brittle).

Cheers

Matthias

  reply	other threads:[~2018-10-26 17:58 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
2018-10-26 17:58       ` Matthias Kaehlcke [this message]
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=20181026175816.GC22824@google.com \
    --to=mka@chromium.org \
    --cc=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=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).