Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200609140351.153833-1-alainm@chromium.org> <20200609140351.153833-2-alainm@chromium.org>
 <F728221B-1F3D-421C-9164-BF6D3C9F6A41@holtmann.org>
In-Reply-To: <F728221B-1F3D-421C-9164-BF6D3C9F6A41@holtmann.org>
From:   Alain Michaud <alainmichaud@google.com>
Date:   Wed, 10 Jun 2020 11:54:29 -0400
Message-ID: <CALWDO_W4vnn-X-YYJ1AjvYMgoK+aDqY3gxKZOWqMw5x0C0y1Cg@mail.gmail.com>
Subject: Re: [PATCH v1 1/3] mgmt: read/set system parameter definitions
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     Alain Michaud <alainm@chromium.org>,
        BlueZ <linux-bluetooth@vger.kernel.org>,
        Abhishek Pandit-Subedi <abhishekpandit@chromium.org>,
        Yu Liu <yudiliu@google.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

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 <marcel@holtmann.org> 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 <abhishekpandit@chromium.org>
> > Reviewed-by: Yu Liu <yudiliu@google.com>
> >
> > Signed-off-by: Alain Michaud <alainm@chromium.org>
> > ---
> >
> > 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
>