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: <20200504142625.213143-1-alainm@chromium.org> <7610937E-424D-4968-B8D9-378E44773088@holtmann.org>
In-Reply-To: <7610937E-424D-4968-B8D9-378E44773088@holtmann.org>
From:   Alain Michaud <alainmichaud@google.com>
Date:   Wed, 6 May 2020 10:06:24 -0400
Message-ID: <CALWDO_UPC5UnbEigTBz0Go-mtyXqCm8GNTyew+M4Eu-P2s-Dyw@mail.gmail.com>
Subject: Re: [BlueZ PATCH v1] doc:adding definitions for load default params
 mgmt op
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     Alain Michaud <alainm@chromium.org>,
        BlueZ <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Marcel,

On Wed, May 6, 2020 at 7:22 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Alain,
>
> > This change adds the definition for the load default parameter command.
> > In particular, this command is used to load default parameters for
> > various operations in the kernel. This mechanism is also expandable to
> > future knobs that may be necessary.
> >
> > This will allow bluetoothd to load parameters from a conf file that may
> > be customized for the specific requirements of each platforms.
> >
> > ---
> >
> > doc/mgmt-api.txt | 59 ++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 59 insertions(+)
> >
> > diff --git a/doc/mgmt-api.txt b/doc/mgmt-api.txt
> > index 39f23c456..71d528706 100644
> > --- a/doc/mgmt-api.txt
> > +++ b/doc/mgmt-api.txt
> > @@ -3137,6 +3137,65 @@ Read Security Information Command
> >       Possible errors:        Invalid Parameters
> >                               Invalid Index
> >
> > +Load Default Parameter Command
> > +=============================
>
> I think that Load Controller Default Parameters is a better name.
I wanted to avoid using "controller" in the name as some of these are
not really "controller" parameters but instead just general
parameters.

>
> > +
> > +     Command Code:           0x0049
> > +     Controller Index:       <controller id>
> > +     Command Parameters:     Parameter_Count (2 Octets)
> > +                             Parameter1 {
> > +                                     Parameter_Type (2 Octet)
> > +                                     Value_Length (1 Octet)
> > +                                     Value (0-255 Octets)
> > +                             }
> > +                             Parameter2 { }
> > +                             ...
> > +     Return Parameters:
> > +
> > +     This command is used to feed the kernel a list of default parameters.
> > +
> > +     Currently defined Parameter_Type values are:
> > +
> > +                     0x0000  BR/EDR Page Scan Type
> > +                     0x0001  BR/EDR Page Scan Interval
> > +                     0x0002  BR/EDR Page Scan Window
> > +                     0x0003  BR/EDR Inquiry Scan Type
> > +                     0x0004  BR/EDR Inquiry Scan Interval
> > +                     0x0005  BR/EDR Inquiry Scan Window
> > +                     0x0006  BR/EDR Link Supervision Timeout
> > +                     0x0007  BR/EDR Page Timeout
> > +                     0x0008  BR/EDR Min Sniff Interval
> > +                     0x0009  BR/EDR Max Sniff Interval
> > +                     0x0080  LE Advertisement Min Interval
> > +                     0x0081  LE Advertisement Max Interval
> > +                     0x0082  LE Multi Advertisement Rotation Interval
> > +                     0x0083  LE Scanning Interval for auto connect
> > +                     0x0084  LE Scanning Window for auto connect
> > +                     0x0085  LE Scanning Interval for HID only
> > +                     0x0086  LE Scanning Window for HID only
> > +                     0x0087  LE Scanning Interval for wake scenarios
> > +                     0x0088  LE Scanning Window for wake scenarios
> > +                     0x0089  LE Scanning Interval for discovery
> > +                     0x008a  LE Scanning Window for discovery
> > +                     0x008b  LE Scanning Interval for adv monitoring
> > +                     0x008c  LE Scanning Window for adv monitoring
> > +                     0x008d  LE Scanning Interval for connect
> > +                     0x008e  LE Scanning Window for connect
> > +                     0x008f  LE Min Connection Interval
> > +                     0x0090  LE Max Connection Interval
> > +                     0x0091  LE Connection Connection Latency
> > +                     0x0092  LE Connection Supervision Timeout
>
> I would just enumerate these and not try to split between 0x000 and 0x008 for BR/EDR and LE. Also I would just start with a few values that we currently want to change.
The goal was to try to keep BR and LE parameters together, but if you
prefer, I don't mind collapsing them.  Note that we intend to use all
of these, so I'd prefer not break this into a smaller subset.

>
> > +
> > +     This command can be used any time, but will not take effect until the
> > +     next activity requiring the parameters.  In the case the parameters are
> > +     used during initialization of the adapter, it is expected that the
> > +     parameters be set before the adapter is powered.
>
> Hmmm. This one, I might want to limit this controllers powered down. Otherwise you get into the conflict case that we would expect parameters taking affect and I think that is just too complicated.
Things like connection intervals and scan defaults actually apply the
next time the activity is started.  We need to be able to change these
without completely powering down the controller and tearing down all
connections etc.  Things that are obviously only used during
initialization, then I would expect they would require a power cycles.

>
> > +
> > +     This command generates a Command Complete event on success or
> > +     a Command Status event on failure.
> > +
> > +     Possible errors:        Invalid Parameters
> > +                             Invalid Index
> >
>
> Regards
>
> Marcel
>