Hi Tomas,
> From: Gregory Paskar <gregory.paskar@xxxxxxxxx>
>
> A Bluetooth device experiencing hardware failure may issue
> a HARDWARE_ERROR hci event. The reaction to this event is device
> reset flow implemented in following sequence.
>
> 1. Notify: HCI_DEV_DOWN
> 2. Reinitialize internal structures.
> 3. Call driver flush function
> 4. Send HCI reset request to the device.
> 5. Send HCI init sequence reset to the device.
> 6. Notify HCI_DEV_UP.
>
> Signed-off-by: Gregory Paskar <gregory.paskar@xxxxxxxxx>
> Signed-off-by: Tomas Winkler <tomas.winkler@xxxxxxxxx>
> Reviewed-by: Ron Rindjunsky <ron.rindjunsky@xxxxxxxxx>
> ---
> V2:
> 1. Fix email typo
> 2. Fix style issues
> V3:
> 1. align with naming convention hw_err_work -> work_hw_err
> 2. use per controller workqueue
>
> include/net/bluetooth/hci.h | 5 ++++
> include/net/bluetooth/hci_core.h | 2 +
> net/bluetooth/hci_core.c | 47 +++++++++++++++++++++++++++++++++++++-
> net/bluetooth/hci_event.c | 3 ++
> 4 files changed, 56 insertions(+), 1 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index fc0c502..3f986e7 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -695,6 +695,11 @@ struct hci_ev_cmd_status {
> __le16 opcode;
> } __attribute__ ((packed));
>
> +#define HCI_EV_HARDWARE_ERROR 0x10
> +struct hci_ev_hw_error {
> + __u8 hw_code;
> +} __attribute__ ((packed));
> +
So the rule is that the HCI_EV and hci_ev struct names are matching up.
It should be matching what we have in the library (not a hard
requirement though):
#define EVT_HARDWARE_ERROR 0x10
typedef struct {
uint8_t code;
} __attribute__ ((packed)) evt_hardware_error;
So just spell it out hardware_error. This means it is also code and not
hw_code inside the struct.
> #define HCI_EV_ROLE_CHANGE 0x12
> struct hci_ev_role_change {
> __u8 status;
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index e17849e..1c9601d 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -113,6 +113,8 @@ struct hci_dev {
> struct tasklet_struct rx_task;
> struct tasklet_struct tx_task;
>
> + struct work_struct work_hw_err;
> +
I would prefer if you just call it reset_work actually. I know that we
are acting on a hardware error, but the action that we are performing is
actually a full reset. It might be useful for other parts of the stack
as well.
> struct sk_buff_head rx_q;
> struct sk_buff_head raw_q;
> struct sk_buff_head cmd_q;
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 8b4baac..c5a43e3 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -839,6 +839,51 @@ static int hci_rfkill_set_block(void *data, bool blocked)
> return 0;
> }
>
> +static void hci_device_hw_error(struct hci_dev *hdev)
> +{
> + hci_req_lock(hdev);
> +
> + /* notify: going down */
> + clear_bit(HCI_UP, &hdev->flags);
> + hci_notify(hdev, HCI_DEV_DOWN);
> +
> + tasklet_disable(&hdev->tx_task);
> + skb_queue_purge(&hdev->rx_q);
> + skb_queue_purge(&hdev->cmd_q);
> +
> + hci_dev_lock_bh(hdev);
> + inquiry_cache_flush(hdev);
> + hci_conn_hash_flush(hdev);
> + hci_dev_unlock_bh(hdev);
> +
> + if (hdev->flush)
> + hdev->flush(hdev);
> +
> + /* reset device */
> + atomic_set(&hdev->cmd_cnt, 1);
> + hdev->acl_cnt = 0;
> + hdev->sco_cnt = 0;
> + hci_reset_req(hdev, 0);
> +
> + /* init device */
> + atomic_set(&hdev->cmd_cnt, 1);
> + hci_init_req(hdev, 0);
> +
> + tasklet_enable(&hdev->tx_task);
> +
> + /* notify: going back up */
> + set_bit(HCI_UP, &hdev->flags);
> + hci_notify(hdev, HCI_DEV_UP);
> +
> + hci_req_unlock(hdev);
> +}
> +
> +static void hci_work_hw_err(struct work_struct *ws)
> +{
> + struct hci_dev *hdev = container_of(ws, struct hci_dev, work_hw_err);
> + hci_device_hw_error(hdev);
> +}
> +
Please name it hci_reset_work to match it properly. And then put
everything in the same function. No need to really split it here.
What I would prefer if we can split out the open/close code and re-use
it it here.
> static const struct rfkill_ops hci_rfkill_ops = {
> .set_block = hci_rfkill_set_block,
> };
> @@ -908,7 +953,7 @@ int hci_register_dev(struct hci_dev *hdev)
> tasklet_init(&hdev->cmd_task, hci_cmd_task,(unsigned long) hdev);
> tasklet_init(&hdev->rx_task, hci_rx_task, (unsigned long) hdev);
> tasklet_init(&hdev->tx_task, hci_tx_task, (unsigned long) hdev);
> -
> + INIT_WORK(&hdev->work_hw_err, hci_work_hw_err);
> skb_queue_head_init(&hdev->rx_q);
> skb_queue_head_init(&hdev->cmd_q);
> skb_queue_head_init(&hdev->raw_q);
I am missing the cancel_work_sync() call in hci_unregister_dev().
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 6c57fc7..6bcf3e9 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1955,6 +1955,9 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
> case HCI_EV_REMOTE_HOST_FEATURES:
> hci_remote_host_features_evt(hdev, skb);
> break;
> + case HCI_EV_HARDWARE_ERROR:
> + queue_work(hdev->workqueue, &hdev->work_hw_err);
> + break;
>
> default:
> BT_DBG("%s event 0x%x", hdev->name, event);
Since it seems you have the only hardware where this can be re-produced,
it would be great if you include some hcidump -X -V extracts inside the
commit message.
Regards
Marcel
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
