Several drivers have the same (and incorrect) code in their
_remove() handler.
Coalesce this into a shared function.
Signed-off-by: Ian Molton <[email protected]>
---
drivers/bluetooth/hci_serdev.c | 12 ++++++++++++
drivers/bluetooth/hci_uart.h | 1 +
2 files changed, 13 insertions(+)
diff --git a/drivers/bluetooth/hci_serdev.c b/drivers/bluetooth/hci_serdev.c
index aea930101dd2..abc70b5e5647 100644
--- a/drivers/bluetooth/hci_serdev.c
+++ b/drivers/bluetooth/hci_serdev.c
@@ -354,3 +354,15 @@ int hci_uart_register_device(struct hci_uart *hu,
return err;
}
EXPORT_SYMBOL_GPL(hci_uart_register_device);
+
+void hci_uart_unregister_device(struct hci_uart *hu) {
+ struct hci_dev *hdev = hu->hdev;
+
+ hci_unregister_dev(hdev);
+ hci_free_dev(hdev);
+
+ cancel_work_sync(&hu->write_work);
+
+ hu->proto->close(hu);
+}
+EXPORT_SYMBOL_GPL(hci_uart_unregister_device);
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index c6e9e1cf63f8..d9cd95d81149 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -112,6 +112,7 @@ struct hci_uart {
int hci_uart_register_proto(const struct hci_uart_proto *p);
int hci_uart_unregister_proto(const struct hci_uart_proto *p);
int hci_uart_register_device(struct hci_uart *hu, const struct hci_uart_proto *p);
+void hci_uart_unregister_device(struct hci_uart *hu);
int hci_uart_tx_wakeup(struct hci_uart *hu);
int hci_uart_init_ready(struct hci_uart *hu);
--
2.11.0
On 09/07/17 19:01, Marcel Holtmann wrote:
>> Is it intended behaviour that you cannot change the address via btmgmt
>> if the device started with a valid bdaddr? obviously this patch is
>> incorrect, but without it, btmgmt public-addr <foo> fails.
>>
>> OTOH, bdaddr does not fail - which seems inconsistent to me. Surely
>> either both should work, or both fail?
>
> you can change them, but the you need a btmgmt power off first. Changing the address while the controller powered on is not possible. You need to reset the controller to have the changes take affect.
Aha, thats the magic sauce :) Thanks.
>> eg. - there is *no* legitimate reason for userspace to send a firmware
>> load command to the chip via my driver, since its already handled by the
>> kernels firmware mechanism - attempting to do so should fail, IMO
>
> Yes. These are legacy UART drivers. We try to phase these out.
I could look into the feasibility of filtering the command stream if you
are interested in a patch to that effect?
-Ian
Hi Ian,
>>> +#define BDADDR_BCM43430A0 (&(bdaddr_t) {{0xf8, 0x1a, 0x08, 0xca, 0x25, 0x00}})
>>
>> isn’t this a valid address? I do not see the point here. At least to me this is not one of the Broadcom default addresses that you get when no EEPROM address is configured.
>
> Yeah, this one was sent out by mistake - thats why there are only 3
> patches in the later series you've applied - I managed to test it on
> another device and it turns out that the original code I had was
> changing the bdaddr for no reason - these addresses are valid, as you say.
>
> I have no idea why this used to be set up this way, for some reason the
> init scripts for the device would set the BDADDR up to == the MAC of the
> Ethernet port. I assumed the devices were coming with invalid MACs but I
> was wrong.
>
> Is it intended behaviour that you cannot change the address via btmgmt
> if the device started with a valid bdaddr? obviously this patch is
> incorrect, but without it, btmgmt public-addr <foo> fails.
>
> OTOH, bdaddr does not fail - which seems inconsistent to me. Surely
> either both should work, or both fail?
you can change them, but the you need a btmgmt power off first. Changing the address while the controller powered on is not possible. You need to reset the controller to have the changes take affect.
> This brings up a secondary issue - currently, some drivers *need* help
> from userspace to get some things done, but it seems like it would be a
> good idea if we filtered the command stream such that the internal state
> of the device cannot be messed with from userspace, except via approved
> APIs?
>
> eg. - there is *no* legitimate reason for userspace to send a firmware
> load command to the chip via my driver, since its already handled by the
> kernels firmware mechanism - attempting to do so should fail, IMO
Yes. These are legacy UART drivers. We try to phase these out.
Regards
Marcel
Resent [to list only] because Thunderbird insists on converting UTF8
into top bit set ASCII.
On 08/07/17 18:40, Marcel Holtmann wrote:
> Hi Ian,
>
>> +#define BDADDR_BCM43430A0 (&(bdaddr_t) {{0xf8, 0x1a, 0x08, 0xca,
>> 0x25, 0x00}})
>
> isn't this a valid address? I do not see the point here. At least to
> me this is not one of the Broadcom default addresses that you get
> when no EEPROM address is configured.
Yeah, this one was sent out by mistake - thats why there are only 3
patches in the later series you've applied - I managed to test it on
another device and it turns out that the original code I had was
changing the bdaddr for no reason - these addresses are valid, as you
say.
I have no idea why this used to be set up this way, for some reason
the init scripts for the device would set the BDADDR up to == the MAC
of the Ethernet port. I assumed the devices were coming with invalid
MACs but I was wrong.
Is it intended behaviour that you cannot change the address via
btmgmt if the device started with a valid bdaddr? obviously this
patch is incorrect, but without it, btmgmt public-addr <foo> fails.
OTOH, bdaddr does not fail - which seems inconsistent to me. Surely
either both should work, or both fail?
This brings up a secondary issue - currently, some drivers *need*
help from userspace to get some things done, but it seems like it
would be a good idea if we filtered the command stream such that the
internal state of the device cannot be messed with from userspace,
except via approved APIs?
eg. - there is *no* legitimate reason for userspace to send a
firmware load command to the chip via my driver, since its already
handled by the kernels firmware mechanism - attempting to do so
should fail, IMO
-Ian
On 09/07/17 09:44, Ian Molton wrote:
> On 08/07/17 18:40, Marcel Holtmann wrote:
>> Hi Ian,
>>
>>> +#define BDADDR_BCM43430A0 (&(bdaddr_t) {{0xf8, 0x1a, 0x08, 0xca, 0x25, 0x00}})
>>
>> isn’t this a valid address? I do not see the point here. At least to me this is not one of the Broadcom default addresses that you get when no EEPROM address is configured.
>
> Yeah, this one was sent out by mistake - thats why there are only 3
> patches in the later series you've applied - I managed to test it on
> another device and it turns out that the original code I had was
> changing the bdaddr for no reason - these addresses are valid, as you say.
>
> I have no idea why this used to be set up this way, for some reason the
> init scripts for the device would set the BDADDR up to == the MAC of the
> Ethernet port. I assumed the devices were coming with invalid MACs but I
> was wrong.
>
> Is it intended behaviour that you cannot change the address via btmgmt
> if the device started with a valid bdaddr? obviously this patch is
> incorrect, but without it, btmgmt public-addr <foo> fails.
>
> OTOH, bdaddr does not fail - which seems inconsistent to me. Surely
> either both should work, or both fail?
>
> This brings up a secondary issue - currently, some drivers *need* help
> from userspace to get some things done, but it seems like it would be a
> good idea if we filtered the command stream such that the internal state
> of the device cannot be messed with from userspace, except via approved
> APIs?
>
> eg. - there is *no* legitimate reason for userspace to send a firmware
> load command to the chip via my driver, since its already handled by the
> kernels firmware mechanism - attempting to do so should fail, IMO
>
> -Ian
>
On 08/07/17 18:40, Marcel Holtmann wrote:
> Hi Ian,
>
>> +#define BDADDR_BCM43430A0 (&(bdaddr_t) {{0xf8, 0x1a, 0x08, 0xca, 0x25, 0x00}})
>
> isn’t this a valid address? I do not see the point here. At least to me this is not one of the Broadcom default addresses that you get when no EEPROM address is configured.
Yeah, this one was sent out by mistake - thats why there are only 3
patches in the later series you've applied - I managed to test it on
another device and it turns out that the original code I had was
changing the bdaddr for no reason - these addresses are valid, as you say.
I have no idea why this used to be set up this way, for some reason the
init scripts for the device would set the BDADDR up to == the MAC of the
Ethernet port. I assumed the devices were coming with invalid MACs but I
was wrong.
Is it intended behaviour that you cannot change the address via btmgmt
if the device started with a valid bdaddr? obviously this patch is
incorrect, but without it, btmgmt public-addr <foo> fails.
OTOH, bdaddr does not fail - which seems inconsistent to me. Surely
either both should work, or both fail?
This brings up a secondary issue - currently, some drivers *need* help
from userspace to get some things done, but it seems like it would be a
good idea if we filtered the command stream such that the internal state
of the device cannot be messed with from userspace, except via approved
APIs?
eg. - there is *no* legitimate reason for userspace to send a firmware
load command to the chip via my driver, since its already handled by the
kernels firmware mechanism - attempting to do so should fail, IMO
-Ian
Hi Ian,
> This patch adds the default BDADDR for Broadcom BCM43430A0
> Bluetooth devices.
> ---
> drivers/bluetooth/btbcm.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
> index 9ab6cfbb831d..28eac429a2ab 100644
> --- a/drivers/bluetooth/btbcm.c
> +++ b/drivers/bluetooth/btbcm.c
> @@ -35,6 +35,7 @@
> #define BDADDR_BCM20702A0 (&(bdaddr_t) {{0x00, 0xa0, 0x02, 0x70, 0x20, 0x00}})
> #define BDADDR_BCM4324B3 (&(bdaddr_t) {{0x00, 0x00, 0x00, 0xb3, 0x24, 0x43}})
> #define BDADDR_BCM4330B1 (&(bdaddr_t) {{0x00, 0x00, 0x00, 0xb1, 0x30, 0x43}})
> +#define BDADDR_BCM43430A0 (&(bdaddr_t) {{0xf8, 0x1a, 0x08, 0xca, 0x25, 0x00}})
isn’t this a valid address? I do not see the point here. At least to me this is not one of the Broadcom default addresses that you get when no EEPROM address is configured.
Regards
Marcel
Sorry for the noise - ignore this series
git send-email ignored my --compose switch :(
-Ian
This patch adds the default BDADDR for Broadcom BCM43430A0
Bluetooth devices.
---
drivers/bluetooth/btbcm.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
index 9ab6cfbb831d..28eac429a2ab 100644
--- a/drivers/bluetooth/btbcm.c
+++ b/drivers/bluetooth/btbcm.c
@@ -35,6 +35,7 @@
#define BDADDR_BCM20702A0 (&(bdaddr_t) {{0x00, 0xa0, 0x02, 0x70, 0x20, 0x00}})
#define BDADDR_BCM4324B3 (&(bdaddr_t) {{0x00, 0x00, 0x00, 0xb3, 0x24, 0x43}})
#define BDADDR_BCM4330B1 (&(bdaddr_t) {{0x00, 0x00, 0x00, 0xb1, 0x30, 0x43}})
+#define BDADDR_BCM43430A0 (&(bdaddr_t) {{0xf8, 0x1a, 0x08, 0xca, 0x25, 0x00}})
int btbcm_check_bdaddr(struct hci_dev *hdev)
{
@@ -70,10 +71,14 @@ int btbcm_check_bdaddr(struct hci_dev *hdev)
*
* The address 43:30:B1:00:00:00 indicates a BCM4330B1 controller
* with waiting for configuration state.
+ *
+ * The address 00:25:CA:08:1A:F8 indicates a BCM43430A0 controller
+ * with no configured address.
*/
if (!bacmp(&bda->bdaddr, BDADDR_BCM20702A0) ||
!bacmp(&bda->bdaddr, BDADDR_BCM4324B3) ||
- !bacmp(&bda->bdaddr, BDADDR_BCM4330B1)) {
+ !bacmp(&bda->bdaddr, BDADDR_BCM4330B1) ||
+ !bacmp(&bda->bdaddr, BDADDR_BCM43430A0)) {
BT_INFO("%s: BCM: Using default device address (%pMR)",
hdev->name, &bda->bdaddr);
set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
--
2.11.0
Convert hci_ll to use hci_uart_unregister_device().
This simplifies the _remove() handler as well as fixes a
potential race condition on unload.
Signed-off-by: Ian Molton <[email protected]>
---
drivers/bluetooth/hci_ll.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/bluetooth/hci_ll.c b/drivers/bluetooth/hci_ll.c
index c982943f0747..1b898445a0b8 100644
--- a/drivers/bluetooth/hci_ll.c
+++ b/drivers/bluetooth/hci_ll.c
@@ -742,14 +742,8 @@ static int hci_ti_probe(struct serdev_device *serdev)
static void hci_ti_remove(struct serdev_device *serdev)
{
struct ll_device *lldev = serdev_device_get_drvdata(serdev);
- struct hci_uart *hu = &lldev->hu;
- struct hci_dev *hdev = hu->hdev;
- cancel_work_sync(&hu->write_work);
-
- hci_unregister_dev(hdev);
- hci_free_dev(hdev);
- hu->proto->close(hu);
+ hci_uart_unregister_device(&lldev->hu);
}
static const struct of_device_id hci_ti_of_match[] = {
--
2.11.0
Simplify _remove() path for hci_nokia.c
Signed-off-by: Ian Molton <[email protected]>
---
drivers/bluetooth/hci_nokia.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/bluetooth/hci_nokia.c b/drivers/bluetooth/hci_nokia.c
index 6dbb1f6ff6bd..3539fd03f47e 100644
--- a/drivers/bluetooth/hci_nokia.c
+++ b/drivers/bluetooth/hci_nokia.c
@@ -767,16 +767,8 @@ static int nokia_bluetooth_serdev_probe(struct serdev_device *serdev)
static void nokia_bluetooth_serdev_remove(struct serdev_device *serdev)
{
struct nokia_bt_dev *btdev = serdev_device_get_drvdata(serdev);
- struct hci_uart *hu = &btdev->hu;
- struct hci_dev *hdev = hu->hdev;
-
- hci_unregister_dev(hdev);
- hci_free_dev(hdev);
-
- cancel_work_sync(&hu->write_work);
-
- hu->proto->close(hu);
+ hci_uart_unregister_device(&btdev->hu);
}
static int nokia_bluetooth_runtime_suspend(struct device *dev)
--
2.11.0