Hi Andrei,
> Define Continuation flag which the only flag used from Flags field
> in L2CAP Configuration Request and Response.
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@xxxxxxxxx>
> ---
> include/net/bluetooth/l2cap.h | 4 ++++
> net/bluetooth/l2cap_core.c | 12 ++++++------
> 2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 452fcc4..8fdfaca 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -271,6 +271,10 @@ struct l2cap_conf_rsp {
> #define L2CAP_CONF_PENDING 0x0004
> #define L2CAP_CONF_EFS_REJECT 0x0005
>
> +/* conf req/rsp continuation flag */
> +#define L2CAP_CONF_FLAG_CONT_CLEARED 0
> +#define L2CAP_CONF_FLAG_CONT_SET 1
> +
this is misleading. Even while the only defined flag is currently the
continuation flag, it is clearly not limited to it.
I am fine with introducing L2CAP_CONF_FLAG_CONTINUATION or
L2CAP_CONF_FLAG_CONT, but we need to keep the no flags set part to 0 or
something else.
> struct l2cap_conf_opt {
> __u8 type;
> __u8 len;
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index d9f215f..65e56d5 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -2544,7 +2544,7 @@ done:
> }
>
> req->dcid = cpu_to_le16(chan->dcid);
> - req->flags = cpu_to_le16(0);
> + req->flags = cpu_to_le16(L2CAP_CONF_FLAG_CONT_CLEARED);
And as a side note, this should be __constant_cpu_to_le16(0).
> return ptr - data;
> }
> @@ -2764,7 +2764,7 @@ done:
> }
> rsp->scid = cpu_to_le16(chan->dcid);
> rsp->result = cpu_to_le16(result);
> - rsp->flags = cpu_to_le16(0x0000);
> + rsp->flags = cpu_to_le16(L2CAP_CONF_FLAG_CONT_CLEARED);
Same here, __constant_cpu_to_le16(0) is the right call.
> return ptr - data;
> }
> @@ -2863,7 +2863,7 @@ static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len, voi
> }
>
> req->dcid = cpu_to_le16(chan->dcid);
> - req->flags = cpu_to_le16(0x0000);
> + req->flags = cpu_to_le16(L2CAP_CONF_FLAG_CONT_CLEARED);
And here as well.
No idea on how we kept missing to change this all the time.
>
> return ptr - data;
> }
> @@ -3218,11 +3218,11 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> memcpy(chan->conf_req + chan->conf_len, req->data, len);
> chan->conf_len += len;
>
> - if (flags & 0x0001) {
> + if (flags & L2CAP_CONF_FLAG_CONT_SET) {
> /* Incomplete config. Send empty response. */
> l2cap_send_cmd(conn, cmd->ident, L2CAP_CONF_RSP,
> l2cap_build_conf_rsp(chan, rsp,
> - L2CAP_CONF_SUCCESS, 0x0001), rsp);
> + L2CAP_CONF_SUCCESS, flags), rsp);
Here you are actually changing behavior. If flags ever has more than one
option, you are returning the other flags as well.
We might better restructure the code a little bit to make it more
readable.
> goto unlock;
> }
>
> @@ -3369,7 +3369,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> goto done;
> }
>
> - if (flags & 0x01)
> + if (flags & L2CAP_CONF_FLAG_CONT_SET)
> goto done;
>
> set_bit(CONF_INPUT_DONE, &chan->conf_state);
Regards
Marcel
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
