Re: [bluetooth-next 1/1] bluetooth: handle device reset event

[Marcel Holtmann <marcel@holtmann.org>]

Hi Tomas,

> From: Gregory Paskar <greogry.paskar@intel.com>
> 
> 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.

since Bluetooth is a brand name, I prefer you write it properly inside
the commit message or comments. The B is uppercase, everything else is
lowercase.

> 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 <greogry.paskar@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> Reviewed-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
> ---
>  include/net/bluetooth/hci.h      |    5 ++++
>  include/net/bluetooth/hci_core.h |    2 +
>  net/bluetooth/hci_core.c         |   40 +++++++++++++++++++++++++++++++++++++-
>  net/bluetooth/hci_event.c        |    3 ++
>  4 files changed, 49 insertions(+), 1 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index ed3aea1..cd23eb4 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -691,6 +691,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));
> +
>  #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 7b86094..d2b1cba 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -110,6 +110,8 @@ struct hci_dev {
>  	struct tasklet_struct	rx_task;
>  	struct tasklet_struct	tx_task;
>  
> +	struct work_struct 	hw_err_work;
> +
>  	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 94ba349..6002439 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -834,6 +834,44 @@ 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);
> +	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);
> +	atomic_set(&hdev->cmd_cnt, 1);
> +	hdev->acl_cnt = 0;
> +	hdev->sco_cnt = 0;
> +	hci_reset_req(hdev, 0);
> +	atomic_set(&hdev->cmd_cnt, 1);
> +	hci_init_req(hdev, 0);
> +	tasklet_enable(&hdev->tx_task);
> +	set_bit(HCI_UP, &hdev->flags);
> +	hci_notify(hdev, HCI_DEV_UP);
> +	hci_req_unlock(hdev);
> +}

This is a big block of commands. Almost impossible for anybody to follow
easily. You need to split them into small logical chunks and separate
them by empty lines.

Also I think there could be more unification with other init and reset
code inside hci_core.c.

> +
> +

I don't know where you get the double empty lines between functions
from, but I don't remember anything inside net/bluetooth/ doing it. And
even if, then that is an oversight.

> +static void hci_hw_err_work(struct work_struct *ws)
> +{
> +	struct hci_dev *hdev;
> +	hdev = container_of(ws, struct hci_dev, hw_err_work);

You could assign it directly at variable declaration.

> +	if (!hdev) {
> +		BT_DBG("%s: hci_hw_error_worker: hdev = NULL ", hdev->name);
> +		return;
> +	}

Can it really happen that hdev is NULL? Even if hdev goes away in
between, the pointer should be not NULL, it should be actually invalid
and cause more harm.

> +	hci_device_hw_error(hdev);
> +}
> +
>  static const struct rfkill_ops hci_rfkill_ops = {
>  	.set_block = hci_rfkill_set_block,
>  };
> @@ -903,7 +941,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->hw_err_work, hci_hw_err_work);
>  	skb_queue_head_init(&hdev->rx_q);
>  	skb_queue_head_init(&hdev->cmd_q);
>  	skb_queue_head_init(&hdev->raw_q);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 28517ba..e8f48cc 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1953,6 +1953,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:
> +		schedule_work(&hdev->hw_err_work);
> +		break;

The more I think about this, the more I feel like we should be
processing the HCI event in a work queue anyway. The sysfs integration
needs it and it is not a performance critical path anyway. The ACL and
SCO/eSCO packets are the only ones where processing in a tasklet really
matters.

Regards

Marcel