On Thu, Mar 16, 2023 at 05:07:29PM +0800, hildawu@xxxxxxxxxxx wrote:
> From: Hilda Wu <hildawu@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx>
> Signed-off-by: Hilda Wu <hildawu@xxxxxxxxxxx>
> ---
> 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@xxxxxxxxx/
Link: https://lore.kernel.org/netdev/202303170056.UsZ6RDV4-lkp@xxxxxxxxx/
>
> 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;
> +}
...
