2020-06-09 14:05:11

by Alain Michaud

[permalink] [raw]
Subject: [PATCH v1 1/3] mgmt: read/set system parameter definitions

This patch submits the corresponding kernel definitions to mgmt.h.
This is submitted before the implementation to avoid any conflicts in
values allocations.

Reviewed-by: Abhishek Pandit-Subedi <[email protected]>
Reviewed-by: Yu Liu <[email protected]>

Signed-off-by: Alain Michaud <[email protected]>
---

include/net/bluetooth/mgmt.h | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)

diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 16e0d87bd8fa..1081e371f03d 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -702,6 +702,24 @@ struct mgmt_rp_set_exp_feature {
__le32 flags;
} __packed;

+#define MGMT_OP_READ_DEFAULT_SYSTEM_PARAMETERS 0x004b
+
+struct mgmt_system_parameter_tlv {
+ __u16 type;
+ __u8 length;
+ __u8 value[];
+} __packed;
+
+struct mgmt_rp_read_default_system_parameters {
+ __u8 parameters[0]; /* mgmt_system_parameter_tlv */
+} __packed;
+
+#define MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS 0x004c
+
+struct mgmt_cp_set_default_system_parameters {
+ __u8 parameters[0]; /* mgmt_system_parameter_tlv */
+} __packed;
+
#define MGMT_EV_CMD_COMPLETE 0x0001
struct mgmt_ev_cmd_complete {
__le16 opcode;
--
2.27.0.278.ge193c7cf3a9-goog


2020-06-10 16:43:36

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH v1 1/3] mgmt: read/set system parameter definitions

Hi Alain,

> This patch submits the corresponding kernel definitions to mgmt.h.
> This is submitted before the implementation to avoid any conflicts in
> values allocations.
>
> Reviewed-by: Abhishek Pandit-Subedi <[email protected]>
> Reviewed-by: Yu Liu <[email protected]>
>
> Signed-off-by: Alain Michaud <[email protected]>
> ---
>
> include/net/bluetooth/mgmt.h | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index 16e0d87bd8fa..1081e371f03d 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -702,6 +702,24 @@ struct mgmt_rp_set_exp_feature {
> __le32 flags;
> } __packed;
>
> +#define MGMT_OP_READ_DEFAULT_SYSTEM_PARAMETERS 0x004b
> +

I would go for MGMT_OP_READ_DEF_SYSTEM_CONFIG or MGMT_OP_READ_DEFAULT_SYSTEM_CONFIG to match the name in the mgmt-api.txt more closely.

> +struct mgmt_system_parameter_tlv {
> + __u16 type;
> + __u8 length;
> + __u8 value[];
> +} __packed;
> +

Can we just introduce a generic mgmt_tlv {} struct. I think we could use it more broadly. However I wonder if we need it actually since have the EIR parsing support. Maybe just extend that one.

> +struct mgmt_rp_read_default_system_parameters {
> + __u8 parameters[0]; /* mgmt_system_parameter_tlv */
> +} __packed;
> +
> +#define MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS 0x004c

Similar to the comment above.

> +
> +struct mgmt_cp_set_default_system_parameters {
> + __u8 parameters[0]; /* mgmt_system_parameter_tlv */
> +} __packed;
> +
> #define MGMT_EV_CMD_COMPLETE 0x0001
> struct mgmt_ev_cmd_complete {
> __le16 opcode;

If you have a chance, please also add MGMT_OP_{READ,SET}_DEF_RUNTIME_CONFIG as well. If not, then I am going to send out a patch for that by myself.

Regards

Marcel

2020-06-10 17:17:42

by Alain Michaud

[permalink] [raw]
Subject: Re: [PATCH v1 1/3] mgmt: read/set system parameter definitions

Hi Marcel,

Since this has already been committed in user space, could we agree to
keep it as is? The alternative is that we'd need to re-patch all the
userspace implementation through a seperate patch. Up to you.

I won't have time to implement the runtime config ones in the next few
weeks, feel free to post it separately, or I can get to it in July.

Thanks,
Alain

On Wed, Jun 10, 2020 at 10:16 AM Marcel Holtmann <[email protected]> wrote:
>
> Hi Alain,
>
> > This patch submits the corresponding kernel definitions to mgmt.h.
> > This is submitted before the implementation to avoid any conflicts in
> > values allocations.
> >
> > Reviewed-by: Abhishek Pandit-Subedi <[email protected]>
> > Reviewed-by: Yu Liu <[email protected]>
> >
> > Signed-off-by: Alain Michaud <[email protected]>
> > ---
> >
> > include/net/bluetooth/mgmt.h | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> > index 16e0d87bd8fa..1081e371f03d 100644
> > --- a/include/net/bluetooth/mgmt.h
> > +++ b/include/net/bluetooth/mgmt.h
> > @@ -702,6 +702,24 @@ struct mgmt_rp_set_exp_feature {
> > __le32 flags;
> > } __packed;
> >
> > +#define MGMT_OP_READ_DEFAULT_SYSTEM_PARAMETERS 0x004b
> > +
>
> I would go for MGMT_OP_READ_DEF_SYSTEM_CONFIG or MGMT_OP_READ_DEFAULT_SYSTEM_CONFIG to match the name in the mgmt-api.txt more closely.
>
> > +struct mgmt_system_parameter_tlv {
> > + __u16 type;
> > + __u8 length;
> > + __u8 value[];
> > +} __packed;
> > +
>
> Can we just introduce a generic mgmt_tlv {} struct. I think we could use it more broadly. However I wonder if we need it actually since have the EIR parsing support. Maybe just extend that one.
>
> > +struct mgmt_rp_read_default_system_parameters {
> > + __u8 parameters[0]; /* mgmt_system_parameter_tlv */
> > +} __packed;
> > +
> > +#define MGMT_OP_SET_DEFAULT_SYSTEM_PARAMETERS 0x004c
>
> Similar to the comment above.
>
> > +
> > +struct mgmt_cp_set_default_system_parameters {
> > + __u8 parameters[0]; /* mgmt_system_parameter_tlv */
> > +} __packed;
> > +
> > #define MGMT_EV_CMD_COMPLETE 0x0001
> > struct mgmt_ev_cmd_complete {
> > __le16 opcode;
>
> If you have a chance, please also add MGMT_OP_{READ,SET}_DEF_RUNTIME_CONFIG as well. If not, then I am going to send out a patch for that by myself.
>
> Regards
>
> Marcel
>

2020-06-10 17:19:27

by Alain Michaud

[permalink] [raw]
Subject: Re: [PATCH v1 1/3] mgmt: read/set system parameter definitions

Hi Marcel,

I'll be happy to review :)

Thanks,
Alain

On Wed, Jun 10, 2020 at 12:02 PM Marcel Holtmann <[email protected]> wrote:
>
> Hi Alain,
>
> > Since this has already been committed in user space, could we agree to
> > keep it as is? The alternative is that we'd need to re-patch all the
> > userspace implementation through a seperate patch. Up to you.
>
> we can fix this up easily. And I can do that if needed.
>
> > I won't have time to implement the runtime config ones in the next few
> > weeks, feel free to post it separately, or I can get to it in July.
>
> That is fine, then I will add it for you. It would be just great if you can review it though.
>
> Regards
>
> Marcel
>

2020-06-10 17:20:01

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH v1 1/3] mgmt: read/set system parameter definitions

Hi Alain,

> Since this has already been committed in user space, could we agree to
> keep it as is? The alternative is that we'd need to re-patch all the
> userspace implementation through a seperate patch. Up to you.

we can fix this up easily. And I can do that if needed.

> I won't have time to implement the runtime config ones in the next few
> weeks, feel free to post it separately, or I can get to it in July.

That is fine, then I will add it for you. It would be just great if you can review it though.

Regards

Marcel