| Message ID | 20230424133542.14383-2-johan+linaro@kernel.org |
|---|---|
| State | Accepted |
| Commit | 92776a7e761e46859fac424ff70200c13d01f9c1 |
| Headers | show |
| Series | Bluetooth: fix bdaddr quirks | expand |
Hi Johan, On Mon, Apr 24, 2023 at 6:35 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > Devices that lack persistent storage for the device address can indicate > this by setting the HCI_QUIRK_INVALID_BDADDR which causes the controller > to be marked as unconfigured until user space has set a valid address. > > Once configured, the device address must be set on every setup for > controllers with HCI_QUIRK_NON_PERSISTENT_SETUP to avoid marking the > controller as unconfigured and requiring the address to be set again. > > Fixes: 740011cfe948 ("Bluetooth: Add new quirk for non-persistent setup settings") > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > net/bluetooth/hci_sync.c | 28 +++++++++++----------------- > 1 file changed, 11 insertions(+), 17 deletions(-) > > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > index a8785126df75..f45598b5a532 100644 > --- a/net/bluetooth/hci_sync.c > +++ b/net/bluetooth/hci_sync.c > @@ -4573,23 +4573,17 @@ static int hci_dev_setup_sync(struct hci_dev *hdev) > invalid_bdaddr = test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks); > > if (!ret) { > - if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) { > - if (!bacmp(&hdev->public_addr, BDADDR_ANY)) > - hci_dev_get_bd_addr_from_property(hdev); > - > - if (bacmp(&hdev->public_addr, BDADDR_ANY) && > - hdev->set_bdaddr) { > - ret = hdev->set_bdaddr(hdev, > - &hdev->public_addr); > - > - /* If setting of the BD_ADDR from the device > - * property succeeds, then treat the address > - * as valid even if the invalid BD_ADDR > - * quirk indicates otherwise. > - */ > - if (!ret) > - invalid_bdaddr = false; > - } > + if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks) && > + !bacmp(&hdev->public_addr, BDADDR_ANY)) > + hci_dev_get_bd_addr_from_property(hdev); > + > + if ((invalid_bdaddr || > + test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) && > + bacmp(&hdev->public_addr, BDADDR_ANY) && > + hdev->set_bdaddr) { > + ret = hdev->set_bdaddr(hdev, &hdev->public_addr); > + if (!ret) > + invalid_bdaddr = false; I'd keep the original comments since it appears you haven't changed its logic with respect to invalid_bdaddr. > } > } > > -- > 2.39.2 >
On Tue, May 02, 2023 at 04:44:48PM -0700, Luiz Augusto von Dentz wrote: > Hi Johan, > > On Mon, Apr 24, 2023 at 6:35 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > > > Devices that lack persistent storage for the device address can indicate > > this by setting the HCI_QUIRK_INVALID_BDADDR which causes the controller > > to be marked as unconfigured until user space has set a valid address. > > > > Once configured, the device address must be set on every setup for > > controllers with HCI_QUIRK_NON_PERSISTENT_SETUP to avoid marking the > > controller as unconfigured and requiring the address to be set again. > > > > Fixes: 740011cfe948 ("Bluetooth: Add new quirk for non-persistent setup settings") > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > --- > > net/bluetooth/hci_sync.c | 28 +++++++++++----------------- > > 1 file changed, 11 insertions(+), 17 deletions(-) > > > > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > > index a8785126df75..f45598b5a532 100644 > > --- a/net/bluetooth/hci_sync.c > > +++ b/net/bluetooth/hci_sync.c > > @@ -4573,23 +4573,17 @@ static int hci_dev_setup_sync(struct hci_dev *hdev) > > invalid_bdaddr = test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks); > > > > if (!ret) { > > - if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) { > > - if (!bacmp(&hdev->public_addr, BDADDR_ANY)) > > - hci_dev_get_bd_addr_from_property(hdev); > > - > > - if (bacmp(&hdev->public_addr, BDADDR_ANY) && > > - hdev->set_bdaddr) { > > - ret = hdev->set_bdaddr(hdev, > > - &hdev->public_addr); > > - > > - /* If setting of the BD_ADDR from the device > > - * property succeeds, then treat the address > > - * as valid even if the invalid BD_ADDR > > - * quirk indicates otherwise. > > - */ > > - if (!ret) > > - invalid_bdaddr = false; > > - } > > + if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks) && > > + !bacmp(&hdev->public_addr, BDADDR_ANY)) > > + hci_dev_get_bd_addr_from_property(hdev); > > + > > + if ((invalid_bdaddr || > > + test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) && > > + bacmp(&hdev->public_addr, BDADDR_ANY) && > > + hdev->set_bdaddr) { > > + ret = hdev->set_bdaddr(hdev, &hdev->public_addr); > > + if (!ret) > > + invalid_bdaddr = false; > > I'd keep the original comments since it appears you haven't changed > its logic with respect to invalid_bdaddr. The comment no longer applies as set_bdaddr can now be called also when there is no "device property" holding the address (e.g. when one has been set using btmgmt and HCI_QUIRK_NON_PERSISTENT_SETUP is set). And obviously we shouldn't mark the controller as unconfigured after we've successfully set an address. Johan
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c index a8785126df75..f45598b5a532 100644 --- a/net/bluetooth/hci_sync.c +++ b/net/bluetooth/hci_sync.c @@ -4573,23 +4573,17 @@ static int hci_dev_setup_sync(struct hci_dev *hdev) invalid_bdaddr = test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks); if (!ret) { - if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) { - if (!bacmp(&hdev->public_addr, BDADDR_ANY)) - hci_dev_get_bd_addr_from_property(hdev); - - if (bacmp(&hdev->public_addr, BDADDR_ANY) && - hdev->set_bdaddr) { - ret = hdev->set_bdaddr(hdev, - &hdev->public_addr); - - /* If setting of the BD_ADDR from the device - * property succeeds, then treat the address - * as valid even if the invalid BD_ADDR - * quirk indicates otherwise. - */ - if (!ret) - invalid_bdaddr = false; - } + if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks) && + !bacmp(&hdev->public_addr, BDADDR_ANY)) + hci_dev_get_bd_addr_from_property(hdev); + + if ((invalid_bdaddr || + test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) && + bacmp(&hdev->public_addr, BDADDR_ANY) && + hdev->set_bdaddr) { + ret = hdev->set_bdaddr(hdev, &hdev->public_addr); + if (!ret) + invalid_bdaddr = false; } }
Devices that lack persistent storage for the device address can indicate this by setting the HCI_QUIRK_INVALID_BDADDR which causes the controller to be marked as unconfigured until user space has set a valid address. Once configured, the device address must be set on every setup for controllers with HCI_QUIRK_NON_PERSISTENT_SETUP to avoid marking the controller as unconfigured and requiring the address to be set again. Fixes: 740011cfe948 ("Bluetooth: Add new quirk for non-persistent setup settings") Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- net/bluetooth/hci_sync.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-)