Re: [PATCH] Bluetooth: msft: Extended monitor tracking by address filter

From: Simon Horman <simon.horman@corigine.com>
To: hildawu@realtek.com
Cc: marcel@holtmann.org, johan.hedberg@gmail.com,
	luiz.dentz@gmail.com, davem@davemloft.net, edumazet@google.com,
	kuba@kernel.org, pabeni@redhat.com,
	linux-bluetooth@vger.kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, apusaka@chromium.org,
	mmandlik@google.com, yinghsu@chromium.org, max.chou@realtek.com,
	alex_lu@realsil.com.cn, kidman@realtek.com
Subject: Re: [PATCH] Bluetooth: msft: Extended monitor tracking by address filter
Date: Sat, 18 Mar 2023 15:22:17 +0100	[thread overview]
Message-ID: <ZBXJGQF0IR3udmdQ@corigine.com> (raw)
In-Reply-To: <20230316090729.14572-1-hildawu@realtek.com>

On Thu, Mar 16, 2023 at 05:07:29PM +0800, hildawu@realtek.com wrote:
> From: Hilda Wu <hildawu@realtek.com>
> 
> Since limited tracking device per condition, this feature is to support
> tracking multiple devices concurrently.
> When a pattern monitor detects the device, this feature issues an address
> monitor for tracking that device. Let pattern monitor can keep monitor
> new devices.
> This feature adds an address filter when receiving a LE monitor device
> event which monitor handle is for a pattern, and the controller started
> monitoring the device. And this feature also has cancelled the monitor
> advertisement from address filters when receiving a LE monitor device
> event when the controller stopped monitoring the device specified by an
> address and monitor handle.
> 
> Signed-off-by: Alex Lu <alex_lu@realsil.com.cn>
> Signed-off-by: Hilda Wu <hildawu@realtek.com>
> ---
>  net/bluetooth/msft.c | 538 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 524 insertions(+), 14 deletions(-)
> 
> diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c

...

> @@ -254,6 +305,64 @@ static int msft_le_monitor_advertisement_cb(struct hci_dev *hdev, u16 opcode,
>  	return status;
>  }
>  
> +/* This function requires the caller holds hci_req_sync_lock */
> +static int msft_remove_addr_filters_sync(struct hci_dev *hdev, u8 handle)

This function always returns 0.
And the caller ignores the return value.
So I think it's return type can be changed to void.

> +{
> +	struct msft_monitor_addr_filter_data *address_filter, *n;
> +	struct msft_data *msft = hdev->msft_data;
> +	struct msft_cp_le_cancel_monitor_advertisement cp;
> +	struct sk_buff *skb;
> +	struct list_head head;

Suggestion:

I assume that it is not standard practice for bluetooth code.
But, FWIIW, I do find it significantly easier to read code that
uses reverse xmas tree - longest line to shortest - for local
variable declarations.

In this case, that would be:

	struct msft_monitor_addr_filter_data *address_filter, *n;
	struct msft_cp_le_cancel_monitor_advertisement cp;
	struct msft_data *msft = hdev->msft_data;
	struct list_head head;
	struct sk_buff *skb;

...

> @@ -400,6 +516,9 @@ static int msft_add_monitor_sync(struct hci_dev *hdev,
>  	ptrdiff_t offset = 0;
>  	u8 pattern_count = 0;
>  	struct sk_buff *skb;
> +	int err;
> +	struct msft_monitor_advertisement_handle_data *handle_data;
> +	struct msft_rp_le_monitor_advertisement *rp;

As per the build bot, rp is set but not the value is not used.
I guess rp can be removed entirely.

Link: https://lore.kernel.org/netdev/202303161807.AcfCGsAP-lkp@intel.com/
Link: https://lore.kernel.org/netdev/202303170056.UsZ6RDV4-lkp@intel.com/

>  
>  	if (!msft_monitor_pattern_valid(monitor))
>  		return -EINVAL;
> @@ -436,16 +555,30 @@ static int msft_add_monitor_sync(struct hci_dev *hdev,
>  
>  	skb = __hci_cmd_sync(hdev, hdev->msft_opcode, total_size, cp,
>  			     HCI_CMD_TIMEOUT);
> -	kfree(cp);
>  
>  	if (IS_ERR_OR_NULL(skb)) {
> -		if (!skb)
> -			return -EIO;
> +		kfree(cp);
>  		return PTR_ERR(skb);
>  	}
>  
> -	return msft_le_monitor_advertisement_cb(hdev, hdev->msft_opcode,
> -						monitor, skb);
> +	err = msft_le_monitor_advertisement_cb(hdev, hdev->msft_opcode,
> +					       monitor, skb);
> +	if (!err) {
> +		rp = (struct msft_rp_le_monitor_advertisement *)skb->data;
> +		handle_data = msft_find_handle_data(hdev, monitor->handle,
> +						    true);
> +		if (handle_data) {
> +			handle_data->rssi_high   = cp->rssi_high;
> +			handle_data->rssi_low    = cp->rssi_low;
> +			handle_data->rssi_low_interval    =
> +						cp->rssi_low_interval;
> +			handle_data->rssi_sampling_period =
> +						cp->rssi_sampling_period;
> +		}
> +	}
> +	kfree(cp);
> +
> +	return err;

I think it would be more idiomatic to write the above something like this.
(* completely untested! *)

	err = msft_le_monitor_advertisement_cb(hdev, hdev->msft_opcode,
					       monitor, skb);
	if (err)
		goto out_free;

	handle_data = msft_find_handle_data(hdev, monitor->handle, true);
	if (!handle_data)
		goto out_free;

	handle_data->rssi_high = cp->rssi_high;
	handle_data->rssi_low = cp->rssi_low;
	handle_data->rssi_low_interval = cp->rssi_low_interval;
	handle_data->rssi_sampling_period = cp->rssi_sampling_period;

out_free:
	kfree(cp);
	return err;

>  }
>  
>  /* This function requires the caller holds hci_req_sync_lock */
> @@ -497,6 +630,41 @@ int msft_resume_sync(struct hci_dev *hdev)
>  	return 0;
>  }
>  
> +/* This function requires the caller holds hci_req_sync_lock */
> +static bool msft_address_monitor_assist_realtek(struct hci_dev *hdev)
> +{
> +	struct sk_buff *skb;
> +	struct {
> +		__u8   status;
> +		__u8   chip_id;
> +	} *rp;
> +
> +	skb = __hci_cmd_sync(hdev, 0xfc6f, 0, NULL, HCI_CMD_TIMEOUT);
> +	if (IS_ERR_OR_NULL(skb)) {
> +		bt_dev_err(hdev, "MSFT: Failed to send the cmd 0xfc6f");
> +		return false;

This seems like an error case that should propagate.

> +	}
> +
> +	rp = (void *)skb->data;
> +	if (skb->len < sizeof(*rp) || rp->status) {
> +		kfree_skb(skb);
> +		return false;

Ditto.

Also, probably this warrant's a cleanup path.

	if (cond) {
		err = -EINVAL;
		goto out_free;
	}
	...

out_free:
	kfree(cp);
	return err;

> +	}
> +
> +	/* RTL8822C chip id: 13
> +	 * RTL8852A chip id: 18
> +	 * RTL8852C chip id: 25
> +	 */
> +	if (rp->chip_id == 13 || rp->chip_id == 18 || rp->chip_id == 25) {
> +		kfree_skb(skb);
> +		return true;

This could also leverage a label such as 'out_free'.

> +	}
> +
> +	kfree_skb(skb);
> +
> +	return false;
> +}
> +
>  /* This function requires the caller holds hci_req_sync_lock */
>  void msft_do_open(struct hci_dev *hdev)
>  {
> @@ -518,6 +686,10 @@ void msft_do_open(struct hci_dev *hdev)
>  	msft->evt_prefix_len = 0;
>  	msft->features = 0;
>  
> +	if (hdev->manufacturer == 0x005d)

Perhaps 0x005d could be a #define to make it clearer what it means.

> +		msft->addr_monitor_assist =
> +			msft_address_monitor_assist_realtek(hdev);
> +
>  	if (!read_supported_features(hdev, msft)) {
>  		hdev->msft_data = NULL;
>  		kfree(msft);

...

> @@ -645,12 +881,237 @@ static void *msft_skb_pull(struct hci_dev *hdev, struct sk_buff *skb,
>  	return data;
>  }
>  
> +static int msft_add_address_filter_sync(struct hci_dev *hdev, void *data)
> +{
> +	struct sk_buff *skb = data;
> +	struct msft_monitor_addr_filter_data *address_filter = NULL;
> +	struct sk_buff *nskb;
> +	struct msft_rp_le_monitor_advertisement *rp;
> +	bool remove = false;
> +	struct msft_data *msft = hdev->msft_data;
> +	int err;
> +
> +	if (!msft) {
> +		bt_dev_err(hdev, "MSFT: msft data is freed");
> +		err = -EINVAL;
> +		goto error;
> +	}
> +
> +	mutex_lock(&msft->filter_lock);
> +
> +	address_filter = msft_find_address_data(hdev,
> +						addr_filter_cb(skb)->addr_type,
> +						&addr_filter_cb(skb)->bdaddr,
> +						addr_filter_cb(skb)->pattern_handle);

nit: mutex_unlock() could go here, to avoid duplicating it below.

> +	if (!address_filter) {
> +		bt_dev_warn(hdev, "MSFT: No address (%pMR) filter to enable",
> +			    &addr_filter_cb(skb)->bdaddr);
> +		mutex_unlock(&msft->filter_lock);
> +		err = -ENODEV;
> +		goto error;
> +	}
> +
> +	mutex_unlock(&msft->filter_lock);

...

> +/* This function requires the caller holds msft->filter_lock */
> +static struct msft_monitor_addr_filter_data *msft_add_address_filter
> +		(struct hci_dev *hdev, u8 addr_type, bdaddr_t *bdaddr,
> +		 struct msft_monitor_advertisement_handle_data *handle_data)
> +{
> +	struct sk_buff *skb;
> +	struct msft_cp_le_monitor_advertisement *cp;
> +	struct msft_monitor_addr_filter_data *address_filter = NULL;
> +	size_t size;
> +	struct msft_data *msft = hdev->msft_data;
> +	int err;
> +
> +	size = sizeof(*cp) + sizeof(addr_type) + sizeof(*bdaddr);
> +	skb = alloc_skb(size, GFP_KERNEL);
> +	if (!skb) {
> +		bt_dev_err(hdev, "MSFT: alloc skb err in device evt");
> +		return NULL;
> +	}
> +
> +	cp = skb_put(skb, sizeof(*cp));
> +	cp->sub_opcode	    = MSFT_OP_LE_MONITOR_ADVERTISEMENT;
> +	cp->rssi_high	    = handle_data->rssi_high;
> +	cp->rssi_low	    = handle_data->rssi_low;
> +	cp->rssi_low_interval    = handle_data->rssi_low_interval;
> +	cp->rssi_sampling_period = handle_data->rssi_sampling_period;
> +	cp->cond_type	    = MSFT_MONITOR_ADVERTISEMENT_TYPE_ADDR;
> +	skb_put_u8(skb, addr_type);
> +	skb_put_data(skb, bdaddr, sizeof(*bdaddr));
> +
> +	address_filter = kzalloc(sizeof(*address_filter), GFP_KERNEL);
> +	if (!address_filter) {
> +		kfree_skb(skb);
> +		return NULL;
> +	}
> +
> +	address_filter->active		     = false;
> +	address_filter->msft_handle	     = 0xff;
> +	address_filter->pattern_handle	     = handle_data->msft_handle;
> +	address_filter->mgmt_handle	     = handle_data->mgmt_handle;
> +	address_filter->rssi_high	     = cp->rssi_high;
> +	address_filter->rssi_low	     = cp->rssi_low;
> +	address_filter->rssi_low_interval    = cp->rssi_low_interval;
> +	address_filter->rssi_sampling_period = cp->rssi_sampling_period;
> +	address_filter->addr_type	     = addr_type;
> +	bacpy(&address_filter->bdaddr, bdaddr);
> +	list_add_tail(&address_filter->list, &msft->address_filters);
> +
> +	addr_filter_cb(skb)->pattern_handle = address_filter->pattern_handle;
> +	addr_filter_cb(skb)->addr_type = addr_type;
> +	bacpy(&addr_filter_cb(skb)->bdaddr, bdaddr);
> +
> +	err = hci_cmd_sync_queue(hdev, msft_add_address_filter_sync, skb, NULL);
> +	if (err < 0) {
> +		bt_dev_err(hdev, "MSFT: Add address %pMR filter err", bdaddr);
> +		list_del(&address_filter->list);
> +		kfree(address_filter);
> +		kfree_skb(skb);
> +		return NULL;
> +	}
> +
> +	bt_dev_info(hdev, "MSFT: Add device %pMR address filter",
> +		    &address_filter->bdaddr);
> +
> +	return address_filter;

I think it would be more idiomatic to handle duplicated cleanup on error
using a label, something like this:

err_skb:
	kfree(skb);
	return NULL;

> +}

...