diff mbox series

[v3,4/7] Bluetooth: Add handler of MGMT_OP_REMOVE_ADV_MONITOR

Message ID 20200611231459.v3.4.Ib4effd5813fb2f8585e2c7394735050c16a765eb@changeid (mailing list archive)
State Superseded
Delegated to: Marcel Holtmann
Headers show
Series [v3,1/7] Bluetooth: Add definitions for advertisement monitor features | expand

Commit Message

Miao-chen Chou June 12, 2020, 6:15 a.m. UTC 
This adds the request handler of MGMT_OP_REMOVE_ADV_MONITOR command.
Note that the controller-based monitoring is not yet in place. This
removes the internal monitor(s) without sending HCI traffic, so the
request returns immediately.

The following test was performed.
- Issue btmgmt advmon-remove with valid and invalid handles.

Signed-off-by: Miao-chen Chou <mcchou@chromium.org>
---

Changes in v3:
- Update the opcode in the mgmt table.
- Convert the endianness of the returned handle.

Changes in v2: None

 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/hci_core.c         | 31 +++++++++++++++++++++++++++++++
 net/bluetooth/mgmt.c             | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+)

Comments

kernel test robot June 12, 2020, 10:33 a.m. UTC | #1 
Hi Miao-chen,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bluetooth-next/master]
[also build test WARNING on net-next/master sparc-next/master net/master next-20200611]
[cannot apply to bluetooth/master v5.7]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Miao-chen-Chou/Bluetooth-Add-definitions-for-advertisement-monitor-features/20200612-141848
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: m68k-randconfig-s031-20200612 (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-250-g42323db3-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 ARCH=m68k CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

   net/bluetooth/mgmt.c:3599:29: sparse: sparse: restricted __le16 degrades to integer
>> net/bluetooth/mgmt.c:4009:46: sparse: sparse: incorrect type in argument 2 (different base types) @@     expected unsigned short [usertype] handle @@     got restricted __le16 [usertype] monitor_handle @@
>> net/bluetooth/mgmt.c:4009:46: sparse:     expected unsigned short [usertype] handle
>> net/bluetooth/mgmt.c:4009:46: sparse:     got restricted __le16 [usertype] monitor_handle
>> net/bluetooth/mgmt.c:4018:29: sparse: sparse: cast from restricted __le16
>> net/bluetooth/mgmt.c:4018:29: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected unsigned short [usertype] val @@     got restricted __le16 [usertype] monitor_handle @@
>> net/bluetooth/mgmt.c:4018:29: sparse:     expected unsigned short [usertype] val
   net/bluetooth/mgmt.c:4018:29: sparse:     got restricted __le16 [usertype] monitor_handle
>> net/bluetooth/mgmt.c:4018:29: sparse: sparse: cast from restricted __le16
>> net/bluetooth/mgmt.c:4018:29: sparse: sparse: cast from restricted __le16

vim +4009 net/bluetooth/mgmt.c

  3997	
  3998	static int remove_adv_monitor(struct sock *sk, struct hci_dev *hdev,
  3999				      void *data, u16 len)
  4000	{
  4001		struct mgmt_cp_remove_adv_monitor *cp = data;
  4002		struct mgmt_rp_remove_adv_monitor rp;
  4003		int err;
  4004	
  4005		BT_DBG("request for %s", hdev->name);
  4006	
  4007		hci_dev_lock(hdev);
  4008	
> 4009		err = hci_remove_adv_monitor(hdev, cp->monitor_handle);
  4010		if (err == -ENOENT) {
  4011			err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_REMOVE_ADV_MONITOR,
  4012					      MGMT_STATUS_INVALID_INDEX);
  4013			goto unlock;
  4014		}
  4015	
  4016		hci_dev_unlock(hdev);
  4017	
> 4018		rp.monitor_handle = cpu_to_le16(cp->monitor_handle);
  4019	
  4020		return mgmt_cmd_complete(sk, hdev->id, MGMT_OP_REMOVE_ADV_MONITOR,
  4021					 MGMT_STATUS_SUCCESS, &rp, sizeof(rp));
  4022	
  4023	unlock:
  4024		hci_dev_unlock(hdev);
  4025		return err;
  4026	}
  4027	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Jakub Kicinski June 12, 2020, 5:49 p.m. UTC | #2 
On Thu, 11 Jun 2020 23:15:26 -0700 Miao-chen Chou wrote:
> This adds the request handler of MGMT_OP_REMOVE_ADV_MONITOR command.
> Note that the controller-based monitoring is not yet in place. This
> removes the internal monitor(s) without sending HCI traffic, so the
> request returns immediately.
> 
> The following test was performed.
> - Issue btmgmt advmon-remove with valid and invalid handles.
> 
> Signed-off-by: Miao-chen Chou <mcchou@chromium.org>

Still doesn't build cleanly with W=1 C=1

net/bluetooth/mgmt.c:4009:46: warning: incorrect type in argument 2 (different base types)
net/bluetooth/mgmt.c:4009:46:    expected unsigned short [usertype] handle
net/bluetooth/mgmt.c:4009:46:    got restricted __le16 [usertype] monitor_handle
net/bluetooth/mgmt.c:4018:29: warning: cast from restricted __le16
Miao-chen Chou June 12, 2020, 11:51 p.m. UTC | #3 
Hi Jakub,

I uploaded v4 to address these. Thanks for the reminder.

Regards,
Miao

On Fri, Jun 12, 2020 at 10:49 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 11 Jun 2020 23:15:26 -0700 Miao-chen Chou wrote:
> > This adds the request handler of MGMT_OP_REMOVE_ADV_MONITOR command.
> > Note that the controller-based monitoring is not yet in place. This
> > removes the internal monitor(s) without sending HCI traffic, so the
> > request returns immediately.
> >
> > The following test was performed.
> > - Issue btmgmt advmon-remove with valid and invalid handles.
> >
> > Signed-off-by: Miao-chen Chou <mcchou@chromium.org>
>
> Still doesn't build cleanly with W=1 C=1
>
> net/bluetooth/mgmt.c:4009:46: warning: incorrect type in argument 2 (different base types)
> net/bluetooth/mgmt.c:4009:46:    expected unsigned short [usertype] handle
> net/bluetooth/mgmt.c:4009:46:    got restricted __le16 [usertype] monitor_handle
> net/bluetooth/mgmt.c:4018:29: warning: cast from restricted __le16
diff mbox series

Patch

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 862d94f711bc0..78ac7fd282d77 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1242,6 +1242,7 @@  void hci_adv_instances_set_rpa_expired(struct hci_dev *hdev, bool rpa_expired);
 void hci_adv_monitors_clear(struct hci_dev *hdev);
 void hci_free_adv_monitor(struct adv_monitor *monitor);
 int hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor);
+int hci_remove_adv_monitor(struct hci_dev *hdev, u16 handle);
 
 void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb);
 
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index fdbb58eb2fb22..d0f30e2e29471 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3041,6 +3041,37 @@  int hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor)
 	return 0;
 }
 
+static int free_adv_monitor(int id, void *ptr, void *data)
+{
+	struct hci_dev *hdev = data;
+	struct adv_monitor *monitor = ptr;
+
+	idr_remove(&hdev->adv_monitors_idr, monitor->handle);
+	hci_free_adv_monitor(monitor);
+
+	return 0;
+}
+
+/* This function requires the caller holds hdev->lock */
+int hci_remove_adv_monitor(struct hci_dev *hdev, u16 handle)
+{
+	struct adv_monitor *monitor;
+
+	if (handle) {
+		monitor = idr_find(&hdev->adv_monitors_idr, handle);
+		if (!monitor)
+			return -ENOENT;
+
+		idr_remove(&hdev->adv_monitors_idr, monitor->handle);
+		hci_free_adv_monitor(monitor);
+	} else {
+		/* Remove all monitors if handle is 0. */
+		idr_for_each(&hdev->adv_monitors_idr, &free_adv_monitor, hdev);
+	}
+
+	return 0;
+}
+
 struct bdaddr_list *hci_bdaddr_list_lookup(struct list_head *bdaddr_list,
 					 bdaddr_t *bdaddr, u8 type)
 {
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 8e0d4ccf81f15..5dc47bba98a90 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -114,6 +114,7 @@  static const u16 mgmt_commands[] = {
 	MGMT_OP_SET_EXP_FEATURE,
 	MGMT_OP_READ_ADV_MONITOR_FEATURES,
 	MGMT_OP_ADD_ADV_PATTERNS_MONITOR,
+	MGMT_OP_REMOVE_ADV_MONITOR,
 };
 
 static const u16 mgmt_events[] = {
@@ -3994,6 +3995,36 @@  static int add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
 	return err;
 }
 
+static int remove_adv_monitor(struct sock *sk, struct hci_dev *hdev,
+			      void *data, u16 len)
+{
+	struct mgmt_cp_remove_adv_monitor *cp = data;
+	struct mgmt_rp_remove_adv_monitor rp;
+	int err;
+
+	BT_DBG("request for %s", hdev->name);
+
+	hci_dev_lock(hdev);
+
+	err = hci_remove_adv_monitor(hdev, cp->monitor_handle);
+	if (err == -ENOENT) {
+		err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_REMOVE_ADV_MONITOR,
+				      MGMT_STATUS_INVALID_INDEX);
+		goto unlock;
+	}
+
+	hci_dev_unlock(hdev);
+
+	rp.monitor_handle = cpu_to_le16(cp->monitor_handle);
+
+	return mgmt_cmd_complete(sk, hdev->id, MGMT_OP_REMOVE_ADV_MONITOR,
+				 MGMT_STATUS_SUCCESS, &rp, sizeof(rp));
+
+unlock:
+	hci_dev_unlock(hdev);
+	return err;
+}
+
 static void read_local_oob_data_complete(struct hci_dev *hdev, u8 status,
 				         u16 opcode, struct sk_buff *skb)
 {
@@ -7451,6 +7482,7 @@  static const struct hci_mgmt_handler mgmt_handlers[] = {
 	{ read_adv_monitor_features, MGMT_READ_ADV_MONITOR_FEATURES_SIZE },
 	{ add_adv_patterns_monitor, MGMT_ADD_ADV_PATTERNS_MONITOR_SIZE,
 						HCI_MGMT_VAR_LEN },
+	{ remove_adv_monitor, MGMT_REMOVE_ADV_MONITOR_SIZE },
 };
 
 void mgmt_index_added(struct hci_dev *hdev)