From: Marcel Holtmann <marcel@holtmann.org>
To: Alain Michaud <alainm@chromium.org>
Cc: linux-bluetooth@vger.kernel.org
Subject: Re: [PATCH] bluetooth: Refactoring mgmt cmd op_code structure
Date: Fri, 17 Jan 2020 18:11:36 +0100 [thread overview]
Message-ID: <4AC68A7A-5F6F-48D1-8A5D-76718FAA200D@holtmann.org> (raw)
In-Reply-To: <20200117034318.51409-1-alainm@chromium.org>
Hi Alain,
> This change refactors the way op-codes are managed in the kernel to
> facilitate easier cherry-picking of features on downlevel kernels
> without incuring significant merge conflicts and op_code collisions.
>
> Signed-off-by: Alain Michaud <alainm@chromium.org>
> ---
>
> include/net/bluetooth/hci_core.h | 1 +
> net/bluetooth/hci_sock.c | 14 +-
> net/bluetooth/mgmt.c | 470 +++++++++++++++++++------------
> 3 files changed, 297 insertions(+), 188 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 89ecf0a80aa1..0cc2f7c11c3a 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1494,6 +1494,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event);
> #define HCI_MGMT_UNCONFIGURED BIT(3)
>
> struct hci_mgmt_handler {
> + unsigned short op_code;
> int (*func) (struct sock *sk, struct hci_dev *hdev, void *data,
> u16 data_len);
> size_t data_len;
> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
> index 3ae508674ef7..4e121607d644 100644
> --- a/net/bluetooth/hci_sock.c
> +++ b/net/bluetooth/hci_sock.c
> @@ -1490,9 +1490,9 @@ static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk,
> void *buf;
> u8 *cp;
> struct mgmt_hdr *hdr;
> - u16 opcode, index, len;
> + u16 opcode, index, len, i;
> struct hci_dev *hdev = NULL;
> - const struct hci_mgmt_handler *handler;
> + const struct hci_mgmt_handler *handler = NULL;
> bool var_len, no_hdev;
> int err;
>
> @@ -1533,16 +1533,18 @@ static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk,
> }
> }
>
> - if (opcode >= chan->handler_count ||
> - chan->handlers[opcode].func == NULL) {
> + for (i = 0; i < chan->handler_count; ++i) {
> + if (opcode == chan->handlers[i].op_code)
> + handler = &chan->handlers[i];
> + }
> +
> + if (!handler || !handler->func) {
> BT_DBG("Unknown op %u", opcode);
> err = mgmt_cmd_status(sk, index, opcode,
> MGMT_STATUS_UNKNOWN_COMMAND);
> goto done;
> }
>
> - handler = &chan->handlers[opcode];
> -
> if (!hci_sock_test_flag(sk, HCI_SOCK_TRUSTED) &&
> !(handler->flags & HCI_MGMT_UNTRUSTED)) {
> err = mgmt_cmd_status(sk, index, opcode,
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 0dc610faab70..7baf9a2809a8 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -40,73 +40,271 @@
> #define MGMT_VERSION 1
> #define MGMT_REVISION 15
>
> -static const u16 mgmt_commands[] = {
> - MGMT_OP_READ_INDEX_LIST,
> - MGMT_OP_READ_INFO,
> - MGMT_OP_SET_POWERED,
> - MGMT_OP_SET_DISCOVERABLE,
> - MGMT_OP_SET_CONNECTABLE,
> - MGMT_OP_SET_FAST_CONNECTABLE,
> - MGMT_OP_SET_BONDABLE,
> - MGMT_OP_SET_LINK_SECURITY,
> - MGMT_OP_SET_SSP,
> - MGMT_OP_SET_HS,
> - MGMT_OP_SET_LE,
> - MGMT_OP_SET_DEV_CLASS,
> - MGMT_OP_SET_LOCAL_NAME,
> - MGMT_OP_ADD_UUID,
> - MGMT_OP_REMOVE_UUID,
> - MGMT_OP_LOAD_LINK_KEYS,
> - MGMT_OP_LOAD_LONG_TERM_KEYS,
> - MGMT_OP_DISCONNECT,
> - MGMT_OP_GET_CONNECTIONS,
> - MGMT_OP_PIN_CODE_REPLY,
> - MGMT_OP_PIN_CODE_NEG_REPLY,
> - MGMT_OP_SET_IO_CAPABILITY,
> - MGMT_OP_PAIR_DEVICE,
> - MGMT_OP_CANCEL_PAIR_DEVICE,
> - MGMT_OP_UNPAIR_DEVICE,
> - MGMT_OP_USER_CONFIRM_REPLY,
> - MGMT_OP_USER_CONFIRM_NEG_REPLY,
> - MGMT_OP_USER_PASSKEY_REPLY,
> - MGMT_OP_USER_PASSKEY_NEG_REPLY,
> - MGMT_OP_READ_LOCAL_OOB_DATA,
> - MGMT_OP_ADD_REMOTE_OOB_DATA,
> - MGMT_OP_REMOVE_REMOTE_OOB_DATA,
> - MGMT_OP_START_DISCOVERY,
> - MGMT_OP_STOP_DISCOVERY,
> - MGMT_OP_CONFIRM_NAME,
> - MGMT_OP_BLOCK_DEVICE,
> - MGMT_OP_UNBLOCK_DEVICE,
> - MGMT_OP_SET_DEVICE_ID,
> - MGMT_OP_SET_ADVERTISING,
> - MGMT_OP_SET_BREDR,
> - MGMT_OP_SET_STATIC_ADDRESS,
> - MGMT_OP_SET_SCAN_PARAMS,
> - MGMT_OP_SET_SECURE_CONN,
> - MGMT_OP_SET_DEBUG_KEYS,
> - MGMT_OP_SET_PRIVACY,
> - MGMT_OP_LOAD_IRKS,
> - MGMT_OP_GET_CONN_INFO,
> - MGMT_OP_GET_CLOCK_INFO,
> - MGMT_OP_ADD_DEVICE,
> - MGMT_OP_REMOVE_DEVICE,
> - MGMT_OP_LOAD_CONN_PARAM,
> - MGMT_OP_READ_UNCONF_INDEX_LIST,
> - MGMT_OP_READ_CONFIG_INFO,
> - MGMT_OP_SET_EXTERNAL_CONFIG,
> - MGMT_OP_SET_PUBLIC_ADDRESS,
> - MGMT_OP_START_SERVICE_DISCOVERY,
> - MGMT_OP_READ_LOCAL_OOB_EXT_DATA,
> - MGMT_OP_READ_EXT_INDEX_LIST,
> - MGMT_OP_READ_ADV_FEATURES,
> - MGMT_OP_ADD_ADVERTISING,
> - MGMT_OP_REMOVE_ADVERTISING,
> - MGMT_OP_GET_ADV_SIZE_INFO,
> - MGMT_OP_START_LIMITED_DISCOVERY,
> - MGMT_OP_READ_EXT_INFO,
> - MGMT_OP_SET_APPEARANCE,
> - MGMT_OP_SET_BLOCKED_KEYS,
> +static int read_version(struct sock *sk, struct hci_dev *hdev, void *data,
> + u16 data_len);
> +static int read_commands(struct sock *sk, struct hci_dev *hdev, void *data,
> + u16 data_len);
<snip>
I do not see this as a good thing. This massive list of forward declarations is not acceptable. That makes the code unmaintainable for upstream.
Regards
Marcel
next prev parent reply other threads:[~2020-01-17 17:11 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-17 3:43 [PATCH] bluetooth: Refactoring mgmt cmd op_code structure Alain Michaud
2020-01-17 17:11 ` Marcel Holtmann [this message]
2020-01-17 17:58 ` Alain Michaud
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4AC68A7A-5F6F-48D1-8A5D-76718FAA200D@holtmann.org \
--to=marcel@holtmann.org \
--cc=alainm@chromium.org \
--cc=linux-bluetooth@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).