| Message ID | 20180101204217.26165-9-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi Martin, > The three-wire (H5) protocol is the only protocol which uses > HCI_UART_INIT_PENDING. > Unfortunately the benefits of using this flag are currently unknown. It > was added in commit 9f2aee848fe6 ("Bluetooth: Add delayed init sequence > support for UART controllers"). In my experiments (with the > "rtk_hciattach" tool - a customized version of hciattach for Realtek > chipsets) I started the tool before and after this patch while the > Bluetooth chipset was disabled (by pulling it's enable GPIO LOW). In > both cases hci0 was not created - thus HCI_UART_INIT_PENDING is not > required in that case. > > Removing this code also has another benefit: hci_serdev.c does not > support the delayed initialization / registration. Thus the protocol > implementation (hci_h5) never receives any data with this check still > in place. For the H5 protocol this means that the initialization never > completes (because the sync response never arrives). Even if the > initialization would succeed later on the drivers would call > hci_uart_init_ready() which schedules the registration which is > currently not implemented by hci_serdev.c. > > Removing the HCI_UART_INIT_PENDING check makes the code easier to read > and also fixes the initalization of devices (implemented with the serdev > library) which use the H5 protocol. > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> I really like Johan to ack this one, but generally I am fine with removing unneeded code. We might also want to look at hciattach to btattach code and make sure it gets removed there as well. I am not even sure anybody used hciattach with H:5 ever. > --- > drivers/bluetooth/hci_h5.c | 3 --- > drivers/bluetooth/hci_ldisc.c | 38 -------------------------------------- > drivers/bluetooth/hci_serdev.c | 3 --- > drivers/bluetooth/hci_uart.h | 4 +--- > 4 files changed, 1 insertion(+), 47 deletions(-) > > diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c > index 6a8d0d06aba7..6cfc2f276250 100644 > --- a/drivers/bluetooth/hci_h5.c > +++ b/drivers/bluetooth/hci_h5.c > @@ -210,8 +210,6 @@ static int h5_open(struct hci_uart *hu) > > h5->tx_win = H5_TX_WIN_MAX; > > - set_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags); > - > /* Send initial sync request */ > h5_link_control(hu, sync, sizeof(sync)); > mod_timer(&h5->timer, jiffies + H5_SYNC_TIMEOUT); > @@ -316,7 +314,6 @@ static void h5_handle_internal_rx(struct hci_uart *hu) > h5->tx_win = (data[2] & 0x07); > BT_DBG("Three-wire init complete. tx_win %u", h5->tx_win); > h5->state = H5_ACTIVE; > - hci_uart_init_ready(hu); > return; > } else if (memcmp(data, sleep_req, 2) == 0) { > BT_DBG("Peer went to sleep"); > diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c > index c823914b3a80..5dd3e1bebfe4 100644 > --- a/drivers/bluetooth/hci_ldisc.c > +++ b/drivers/bluetooth/hci_ldisc.c > @@ -195,39 +195,6 @@ static void hci_uart_write_work(struct work_struct *work) > clear_bit(HCI_UART_SENDING, &hu->tx_state); > } > > -static void hci_uart_init_work(struct work_struct *work) > -{ > - struct hci_uart *hu = container_of(work, struct hci_uart, init_ready); > - int err; > - struct hci_dev *hdev; > - > - if (!test_and_clear_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags)) > - return; > - > - err = hci_register_dev(hu->hdev); > - if (err < 0) { > - BT_ERR("Can't register HCI device"); > - hdev = hu->hdev; > - hu->hdev = NULL; > - hci_free_dev(hdev); > - clear_bit(HCI_UART_PROTO_READY, &hu->flags); > - hu->proto->close(hu); > - return; > - } > - > - set_bit(HCI_UART_REGISTERED, &hu->flags); > -} > - > -int hci_uart_init_ready(struct hci_uart *hu) > -{ > - if (!test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags)) > - return -EALREADY; > - > - schedule_work(&hu->init_ready); > - > - return 0; > -} > - > /* ------- Interface to HCI layer ------ */ > /* Initialize device */ > static int hci_uart_open(struct hci_dev *hdev) > @@ -490,7 +457,6 @@ static int hci_uart_tty_open(struct tty_struct *tty) > hu->alignment = 1; > hu->padding = 0; > > - INIT_WORK(&hu->init_ready, hci_uart_init_work); > INIT_WORK(&hu->write_work, hci_uart_write_work); > > percpu_init_rwsem(&hu->proto_lock); > @@ -653,9 +619,6 @@ static int hci_uart_register_dev(struct hci_uart *hu) > else > hdev->dev_type = HCI_PRIMARY; > > - if (test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags)) > - return 0; > - > if (hci_register_dev(hdev) < 0) { > BT_ERR("Can't register HCI device"); > hu->hdev = NULL; > @@ -699,7 +662,6 @@ static int hci_uart_set_flags(struct hci_uart *hu, unsigned long flags) > unsigned long valid_flags = BIT(HCI_UART_RAW_DEVICE) | > BIT(HCI_UART_RESET_ON_INIT) | > BIT(HCI_UART_CREATE_AMP) | > - BIT(HCI_UART_INIT_PENDING) | > BIT(HCI_UART_EXT_CONFIG) | > BIT(HCI_UART_VND_DETECT); > > diff --git a/drivers/bluetooth/hci_serdev.c b/drivers/bluetooth/hci_serdev.c > index e0e6461b9200..fe67eb6d4278 100644 > --- a/drivers/bluetooth/hci_serdev.c > +++ b/drivers/bluetooth/hci_serdev.c > @@ -333,9 +333,6 @@ int hci_uart_register_device(struct hci_uart *hu, > else > hdev->dev_type = HCI_PRIMARY; > > - if (test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags)) > - return 0; > - > if (hci_register_dev(hdev) < 0) { > BT_ERR("Can't register HCI device"); > err = -ENODEV; > diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h > index 66e8c68e4607..47e755ff4092 100644 > --- a/drivers/bluetooth/hci_uart.h > +++ b/drivers/bluetooth/hci_uart.h > @@ -53,7 +53,7 @@ > #define HCI_UART_RAW_DEVICE 0 > #define HCI_UART_RESET_ON_INIT 1 > #define HCI_UART_CREATE_AMP 2 > -#define HCI_UART_INIT_PENDING 3 > +/* 3 is unused - was HCI_UART_INIT_PENDING */ #define HCI_UART_INIT_PENDING 3 /* unused */ I prefer it this way since it is easier on the eyes. > #define HCI_UART_EXT_CONFIG 4 > #define HCI_UART_VND_DETECT 5 > > @@ -83,7 +83,6 @@ struct hci_uart { > unsigned long flags; > unsigned long hdev_flags; > > - struct work_struct init_ready; > struct work_struct write_work; > > const struct hci_uart_proto *proto; > @@ -115,7 +114,6 @@ 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); > void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed); > void hci_uart_set_flow_control(struct hci_uart *hu, bool enable); > void hci_uart_set_speeds(struct hci_uart *hu, unsigned int init_speed, Regards Marcel
Hi Marcel, thank you for looking into this latest version! On Tue, Jan 2, 2018 at 12:04 PM, Marcel Holtmann <marcel@holtmann.org> wrote: > Hi Martin, > >> The three-wire (H5) protocol is the only protocol which uses >> HCI_UART_INIT_PENDING. >> Unfortunately the benefits of using this flag are currently unknown. It >> was added in commit 9f2aee848fe6 ("Bluetooth: Add delayed init sequence >> support for UART controllers"). In my experiments (with the >> "rtk_hciattach" tool - a customized version of hciattach for Realtek >> chipsets) I started the tool before and after this patch while the >> Bluetooth chipset was disabled (by pulling it's enable GPIO LOW). In >> both cases hci0 was not created - thus HCI_UART_INIT_PENDING is not >> required in that case. >> >> Removing this code also has another benefit: hci_serdev.c does not >> support the delayed initialization / registration. Thus the protocol >> implementation (hci_h5) never receives any data with this check still >> in place. For the H5 protocol this means that the initialization never >> completes (because the sync response never arrives). Even if the >> initialization would succeed later on the drivers would call >> hci_uart_init_ready() which schedules the registration which is >> currently not implemented by hci_serdev.c. >> >> Removing the HCI_UART_INIT_PENDING check makes the code easier to read >> and also fixes the initalization of devices (implemented with the serdev >> library) which use the H5 protocol. >> >> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > I really like Johan to ack this one, but generally I am fine with removing unneeded code. I would also like as many ACKs/Tested-by/Reviewed-by as possible since this is all new code for me (so it's easy for me to make mistakes)! > We might also want to look at hciattach to btattach code and make sure it gets removed there as well. I am not even sure anybody used hciattach with H:5 ever. a quick glance shows that it's defined in bluez.git but never used there (which is good in this case) >> --- >> drivers/bluetooth/hci_h5.c | 3 --- >> drivers/bluetooth/hci_ldisc.c | 38 -------------------------------------- >> drivers/bluetooth/hci_serdev.c | 3 --- >> drivers/bluetooth/hci_uart.h | 4 +--- >> 4 files changed, 1 insertion(+), 47 deletions(-) >> >> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c >> index 6a8d0d06aba7..6cfc2f276250 100644 >> --- a/drivers/bluetooth/hci_h5.c >> +++ b/drivers/bluetooth/hci_h5.c >> @@ -210,8 +210,6 @@ static int h5_open(struct hci_uart *hu) >> >> h5->tx_win = H5_TX_WIN_MAX; >> >> - set_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags); >> - >> /* Send initial sync request */ >> h5_link_control(hu, sync, sizeof(sync)); >> mod_timer(&h5->timer, jiffies + H5_SYNC_TIMEOUT); >> @@ -316,7 +314,6 @@ static void h5_handle_internal_rx(struct hci_uart *hu) >> h5->tx_win = (data[2] & 0x07); >> BT_DBG("Three-wire init complete. tx_win %u", h5->tx_win); >> h5->state = H5_ACTIVE; >> - hci_uart_init_ready(hu); >> return; >> } else if (memcmp(data, sleep_req, 2) == 0) { >> BT_DBG("Peer went to sleep"); >> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c >> index c823914b3a80..5dd3e1bebfe4 100644 >> --- a/drivers/bluetooth/hci_ldisc.c >> +++ b/drivers/bluetooth/hci_ldisc.c >> @@ -195,39 +195,6 @@ static void hci_uart_write_work(struct work_struct *work) >> clear_bit(HCI_UART_SENDING, &hu->tx_state); >> } >> >> -static void hci_uart_init_work(struct work_struct *work) >> -{ >> - struct hci_uart *hu = container_of(work, struct hci_uart, init_ready); >> - int err; >> - struct hci_dev *hdev; >> - >> - if (!test_and_clear_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags)) >> - return; >> - >> - err = hci_register_dev(hu->hdev); >> - if (err < 0) { >> - BT_ERR("Can't register HCI device"); >> - hdev = hu->hdev; >> - hu->hdev = NULL; >> - hci_free_dev(hdev); >> - clear_bit(HCI_UART_PROTO_READY, &hu->flags); >> - hu->proto->close(hu); >> - return; >> - } >> - >> - set_bit(HCI_UART_REGISTERED, &hu->flags); >> -} >> - >> -int hci_uart_init_ready(struct hci_uart *hu) >> -{ >> - if (!test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags)) >> - return -EALREADY; >> - >> - schedule_work(&hu->init_ready); >> - >> - return 0; >> -} >> - >> /* ------- Interface to HCI layer ------ */ >> /* Initialize device */ >> static int hci_uart_open(struct hci_dev *hdev) >> @@ -490,7 +457,6 @@ static int hci_uart_tty_open(struct tty_struct *tty) >> hu->alignment = 1; >> hu->padding = 0; >> >> - INIT_WORK(&hu->init_ready, hci_uart_init_work); >> INIT_WORK(&hu->write_work, hci_uart_write_work); >> >> percpu_init_rwsem(&hu->proto_lock); >> @@ -653,9 +619,6 @@ static int hci_uart_register_dev(struct hci_uart *hu) >> else >> hdev->dev_type = HCI_PRIMARY; >> >> - if (test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags)) >> - return 0; >> - >> if (hci_register_dev(hdev) < 0) { >> BT_ERR("Can't register HCI device"); >> hu->hdev = NULL; >> @@ -699,7 +662,6 @@ static int hci_uart_set_flags(struct hci_uart *hu, unsigned long flags) >> unsigned long valid_flags = BIT(HCI_UART_RAW_DEVICE) | >> BIT(HCI_UART_RESET_ON_INIT) | >> BIT(HCI_UART_CREATE_AMP) | >> - BIT(HCI_UART_INIT_PENDING) | >> BIT(HCI_UART_EXT_CONFIG) | >> BIT(HCI_UART_VND_DETECT); >> >> diff --git a/drivers/bluetooth/hci_serdev.c b/drivers/bluetooth/hci_serdev.c >> index e0e6461b9200..fe67eb6d4278 100644 >> --- a/drivers/bluetooth/hci_serdev.c >> +++ b/drivers/bluetooth/hci_serdev.c >> @@ -333,9 +333,6 @@ int hci_uart_register_device(struct hci_uart *hu, >> else >> hdev->dev_type = HCI_PRIMARY; >> >> - if (test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags)) >> - return 0; >> - >> if (hci_register_dev(hdev) < 0) { >> BT_ERR("Can't register HCI device"); >> err = -ENODEV; >> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h >> index 66e8c68e4607..47e755ff4092 100644 >> --- a/drivers/bluetooth/hci_uart.h >> +++ b/drivers/bluetooth/hci_uart.h >> @@ -53,7 +53,7 @@ >> #define HCI_UART_RAW_DEVICE 0 >> #define HCI_UART_RESET_ON_INIT 1 >> #define HCI_UART_CREATE_AMP 2 >> -#define HCI_UART_INIT_PENDING 3 >> +/* 3 is unused - was HCI_UART_INIT_PENDING */ > > #define HCI_UART_INIT_PENDING 3 /* unused */ > > I prefer it this way since it is easier on the eyes. OK, I'll do it that way in the next version >> #define HCI_UART_EXT_CONFIG 4 >> #define HCI_UART_VND_DETECT 5 >> >> @@ -83,7 +83,6 @@ struct hci_uart { >> unsigned long flags; >> unsigned long hdev_flags; >> >> - struct work_struct init_ready; >> struct work_struct write_work; >> >> const struct hci_uart_proto *proto; >> @@ -115,7 +114,6 @@ 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); >> void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed); >> void hci_uart_set_flow_control(struct hci_uart *hu, bool enable); >> void hci_uart_set_speeds(struct hci_uart *hu, unsigned int init_speed, Regards Martin
Hi Martin, On 2 January 2018 at 22:06, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > Hi Marcel, > > thank you for looking into this latest version! > > On Tue, Jan 2, 2018 at 12:04 PM, Marcel Holtmann <marcel@holtmann.org> wrote: >> Hi Martin, >> >>> The three-wire (H5) protocol is the only protocol which uses >>> HCI_UART_INIT_PENDING. >>> Unfortunately the benefits of using this flag are currently unknown. It >>> was added in commit 9f2aee848fe6 ("Bluetooth: Add delayed init sequence >>> support for UART controllers"). In my experiments (with the >>> "rtk_hciattach" tool - a customized version of hciattach for Realtek >>> chipsets) I started the tool before and after this patch while the >>> Bluetooth chipset was disabled (by pulling it's enable GPIO LOW). In >>> both cases hci0 was not created - thus HCI_UART_INIT_PENDING is not >>> required in that case. >>> >>> Removing this code also has another benefit: hci_serdev.c does not >>> support the delayed initialization / registration. Thus the protocol >>> implementation (hci_h5) never receives any data with this check still >>> in place. For the H5 protocol this means that the initialization never >>> completes (because the sync response never arrives). Even if the >>> initialization would succeed later on the drivers would call >>> hci_uart_init_ready() which schedules the registration which is >>> currently not implemented by hci_serdev.c. >>> >>> Removing the HCI_UART_INIT_PENDING check makes the code easier to read >>> and also fixes the initalization of devices (implemented with the serdev >>> library) which use the H5 protocol. >>> >>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> I think the original goal is to perform H5 init peacefully. The H5 protocol needs to be open in order to send/receive H5 link packets during the H5 initialization/synchronization step. During this stage, driver prevents upper stack to send any HCI packet by delaying the HCI device registration. Regards, Loic
On Mon, Jan 1, 2018 at 2:42 PM, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > The three-wire (H5) protocol is the only protocol which uses > HCI_UART_INIT_PENDING. I think I'd also found that this flag wasn't really needed. > Unfortunately the benefits of using this flag are currently unknown. It > was added in commit 9f2aee848fe6 ("Bluetooth: Add delayed init sequence > support for UART controllers"). In my experiments (with the > "rtk_hciattach" tool - a customized version of hciattach for Realtek > chipsets) I started the tool before and after this patch while the > Bluetooth chipset was disabled (by pulling it's enable GPIO LOW). In > both cases hci0 was not created - thus HCI_UART_INIT_PENDING is not > required in that case. > > Removing this code also has another benefit: hci_serdev.c does not > support the delayed initialization / registration. This statement is misleading. serdev *always* supports async init as it forces async probe of drivers. It doesn't need to support the private workq init mechanism. At least that was my intent. > Thus the protocol > implementation (hci_h5) never receives any data with this check still > in place. For the H5 protocol this means that the initialization never > completes (because the sync response never arrives). Even if the > initialization would succeed later on the drivers would call > hci_uart_init_ready() which schedules the registration which is > currently not implemented by hci_serdev.c. > > Removing the HCI_UART_INIT_PENDING check makes the code easier to read > and also fixes the initalization of devices (implemented with the serdev > library) which use the H5 protocol. > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > drivers/bluetooth/hci_h5.c | 3 --- > drivers/bluetooth/hci_ldisc.c | 38 -------------------------------------- > drivers/bluetooth/hci_serdev.c | 3 --- > drivers/bluetooth/hci_uart.h | 4 +--- > 4 files changed, 1 insertion(+), 47 deletions(-)
Hi Loic, On Wed, Jan 3, 2018 at 6:14 PM, Loic Poulain <loic.poulain@linaro.org> wrote: > Hi Martin, > > On 2 January 2018 at 22:06, Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: >> Hi Marcel, >> >> thank you for looking into this latest version! >> >> On Tue, Jan 2, 2018 at 12:04 PM, Marcel Holtmann <marcel@holtmann.org> wrote: >>> Hi Martin, >>> >>>> The three-wire (H5) protocol is the only protocol which uses >>>> HCI_UART_INIT_PENDING. >>>> Unfortunately the benefits of using this flag are currently unknown. It >>>> was added in commit 9f2aee848fe6 ("Bluetooth: Add delayed init sequence >>>> support for UART controllers"). In my experiments (with the >>>> "rtk_hciattach" tool - a customized version of hciattach for Realtek >>>> chipsets) I started the tool before and after this patch while the >>>> Bluetooth chipset was disabled (by pulling it's enable GPIO LOW). In >>>> both cases hci0 was not created - thus HCI_UART_INIT_PENDING is not >>>> required in that case. >>>> >>>> Removing this code also has another benefit: hci_serdev.c does not >>>> support the delayed initialization / registration. Thus the protocol >>>> implementation (hci_h5) never receives any data with this check still >>>> in place. For the H5 protocol this means that the initialization never >>>> completes (because the sync response never arrives). Even if the >>>> initialization would succeed later on the drivers would call >>>> hci_uart_init_ready() which schedules the registration which is >>>> currently not implemented by hci_serdev.c. >>>> >>>> Removing the HCI_UART_INIT_PENDING check makes the code easier to read >>>> and also fixes the initalization of devices (implemented with the serdev >>>> library) which use the H5 protocol. >>>> >>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > I think the original goal is to perform H5 init peacefully. > The H5 protocol needs to be open in order to send/receive H5 link > packets during the H5 initialization/synchronization step. > During this stage, driver prevents upper stack to send any HCI packet > by delaying the HCI device registration. thank you for this explanation do you have a test-case (manual list of steps, a scripts, etc.) which I can use to compare the old and new behavior? based on your explanation this would mean that we should add HCI_UART_INIT_PENDING support to the serdev code rather than removing it Regards Martin
Hi Rob, On Wed, Jan 3, 2018 at 7:38 PM, Rob Herring <robh+dt@kernel.org> wrote: > On Mon, Jan 1, 2018 at 2:42 PM, Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: >> The three-wire (H5) protocol is the only protocol which uses >> HCI_UART_INIT_PENDING. > > I think I'd also found that this flag wasn't really needed. do you remember how you tested this? did you test it with you new serdev code or with the old hci_ldisc code (with some userspace tool)? >> Unfortunately the benefits of using this flag are currently unknown. It >> was added in commit 9f2aee848fe6 ("Bluetooth: Add delayed init sequence >> support for UART controllers"). In my experiments (with the >> "rtk_hciattach" tool - a customized version of hciattach for Realtek >> chipsets) I started the tool before and after this patch while the >> Bluetooth chipset was disabled (by pulling it's enable GPIO LOW). In >> both cases hci0 was not created - thus HCI_UART_INIT_PENDING is not >> required in that case. >> >> Removing this code also has another benefit: hci_serdev.c does not >> support the delayed initialization / registration. > > This statement is misleading. serdev *always* supports async init as > it forces async probe of drivers. It doesn't need to support the > private workq init mechanism. At least that was my intent. thank you for having a look at this patch! I will update the description in the next version >> Thus the protocol >> implementation (hci_h5) never receives any data with this check still >> in place. For the H5 protocol this means that the initialization never >> completes (because the sync response never arrives). Even if the >> initialization would succeed later on the drivers would call >> hci_uart_init_ready() which schedules the registration which is >> currently not implemented by hci_serdev.c. >> >> Removing the HCI_UART_INIT_PENDING check makes the code easier to read >> and also fixes the initalization of devices (implemented with the serdev >> library) which use the H5 protocol. >> >> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> >> --- >> drivers/bluetooth/hci_h5.c | 3 --- >> drivers/bluetooth/hci_ldisc.c | 38 -------------------------------------- >> drivers/bluetooth/hci_serdev.c | 3 --- >> drivers/bluetooth/hci_uart.h | 4 +--- >> 4 files changed, 1 insertion(+), 47 deletions(-) Regards Martin
diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c index 6a8d0d06aba7..6cfc2f276250 100644 --- a/drivers/bluetooth/hci_h5.c +++ b/drivers/bluetooth/hci_h5.c @@ -210,8 +210,6 @@ static int h5_open(struct hci_uart *hu) h5->tx_win = H5_TX_WIN_MAX; - set_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags); - /* Send initial sync request */ h5_link_control(hu, sync, sizeof(sync)); mod_timer(&h5->timer, jiffies + H5_SYNC_TIMEOUT); @@ -316,7 +314,6 @@ static void h5_handle_internal_rx(struct hci_uart *hu) h5->tx_win = (data[2] & 0x07); BT_DBG("Three-wire init complete. tx_win %u", h5->tx_win); h5->state = H5_ACTIVE; - hci_uart_init_ready(hu); return; } else if (memcmp(data, sleep_req, 2) == 0) { BT_DBG("Peer went to sleep"); diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c index c823914b3a80..5dd3e1bebfe4 100644 --- a/drivers/bluetooth/hci_ldisc.c +++ b/drivers/bluetooth/hci_ldisc.c @@ -195,39 +195,6 @@ static void hci_uart_write_work(struct work_struct *work) clear_bit(HCI_UART_SENDING, &hu->tx_state); } -static void hci_uart_init_work(struct work_struct *work) -{ - struct hci_uart *hu = container_of(work, struct hci_uart, init_ready); - int err; - struct hci_dev *hdev; - - if (!test_and_clear_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags)) - return; - - err = hci_register_dev(hu->hdev); - if (err < 0) { - BT_ERR("Can't register HCI device"); - hdev = hu->hdev; - hu->hdev = NULL; - hci_free_dev(hdev); - clear_bit(HCI_UART_PROTO_READY, &hu->flags); - hu->proto->close(hu); - return; - } - - set_bit(HCI_UART_REGISTERED, &hu->flags); -} - -int hci_uart_init_ready(struct hci_uart *hu) -{ - if (!test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags)) - return -EALREADY; - - schedule_work(&hu->init_ready); - - return 0; -} - /* ------- Interface to HCI layer ------ */ /* Initialize device */ static int hci_uart_open(struct hci_dev *hdev) @@ -490,7 +457,6 @@ static int hci_uart_tty_open(struct tty_struct *tty) hu->alignment = 1; hu->padding = 0; - INIT_WORK(&hu->init_ready, hci_uart_init_work); INIT_WORK(&hu->write_work, hci_uart_write_work); percpu_init_rwsem(&hu->proto_lock); @@ -653,9 +619,6 @@ static int hci_uart_register_dev(struct hci_uart *hu) else hdev->dev_type = HCI_PRIMARY; - if (test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags)) - return 0; - if (hci_register_dev(hdev) < 0) { BT_ERR("Can't register HCI device"); hu->hdev = NULL; @@ -699,7 +662,6 @@ static int hci_uart_set_flags(struct hci_uart *hu, unsigned long flags) unsigned long valid_flags = BIT(HCI_UART_RAW_DEVICE) | BIT(HCI_UART_RESET_ON_INIT) | BIT(HCI_UART_CREATE_AMP) | - BIT(HCI_UART_INIT_PENDING) | BIT(HCI_UART_EXT_CONFIG) | BIT(HCI_UART_VND_DETECT); diff --git a/drivers/bluetooth/hci_serdev.c b/drivers/bluetooth/hci_serdev.c index e0e6461b9200..fe67eb6d4278 100644 --- a/drivers/bluetooth/hci_serdev.c +++ b/drivers/bluetooth/hci_serdev.c @@ -333,9 +333,6 @@ int hci_uart_register_device(struct hci_uart *hu, else hdev->dev_type = HCI_PRIMARY; - if (test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags)) - return 0; - if (hci_register_dev(hdev) < 0) { BT_ERR("Can't register HCI device"); err = -ENODEV; diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h index 66e8c68e4607..47e755ff4092 100644 --- a/drivers/bluetooth/hci_uart.h +++ b/drivers/bluetooth/hci_uart.h @@ -53,7 +53,7 @@ #define HCI_UART_RAW_DEVICE 0 #define HCI_UART_RESET_ON_INIT 1 #define HCI_UART_CREATE_AMP 2 -#define HCI_UART_INIT_PENDING 3 +/* 3 is unused - was HCI_UART_INIT_PENDING */ #define HCI_UART_EXT_CONFIG 4 #define HCI_UART_VND_DETECT 5 @@ -83,7 +83,6 @@ struct hci_uart { unsigned long flags; unsigned long hdev_flags; - struct work_struct init_ready; struct work_struct write_work; const struct hci_uart_proto *proto; @@ -115,7 +114,6 @@ 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); void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed); void hci_uart_set_flow_control(struct hci_uart *hu, bool enable); void hci_uart_set_speeds(struct hci_uart *hu, unsigned int init_speed,
The three-wire (H5) protocol is the only protocol which uses HCI_UART_INIT_PENDING. Unfortunately the benefits of using this flag are currently unknown. It was added in commit 9f2aee848fe6 ("Bluetooth: Add delayed init sequence support for UART controllers"). In my experiments (with the "rtk_hciattach" tool - a customized version of hciattach for Realtek chipsets) I started the tool before and after this patch while the Bluetooth chipset was disabled (by pulling it's enable GPIO LOW). In both cases hci0 was not created - thus HCI_UART_INIT_PENDING is not required in that case. Removing this code also has another benefit: hci_serdev.c does not support the delayed initialization / registration. Thus the protocol implementation (hci_h5) never receives any data with this check still in place. For the H5 protocol this means that the initialization never completes (because the sync response never arrives). Even if the initialization would succeed later on the drivers would call hci_uart_init_ready() which schedules the registration which is currently not implemented by hci_serdev.c. Removing the HCI_UART_INIT_PENDING check makes the code easier to read and also fixes the initalization of devices (implemented with the serdev library) which use the H5 protocol. Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- drivers/bluetooth/hci_h5.c | 3 --- drivers/bluetooth/hci_ldisc.c | 38 -------------------------------------- drivers/bluetooth/hci_serdev.c | 3 --- drivers/bluetooth/hci_uart.h | 4 +--- 4 files changed, 1 insertion(+), 47 deletions(-)