Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220526112456.2488536-1-josephsih@chromium.org>
 <20220526112456.2488536-3-josephsih@chromium.org> <CABBYNZ+jxkXvsmKxka0McLDgZOXp=aJ9Q3pT0P-2_0k79+2Q5g@mail.gmail.com>
 <CAHFy41_D_JRxe34hUheg0pGVRk9ns0GWUvZPkCQe1G7iZHAitQ@mail.gmail.com>
In-Reply-To: <CAHFy41_D_JRxe34hUheg0pGVRk9ns0GWUvZPkCQe1G7iZHAitQ@mail.gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 30 May 2022 14:02:56 -0700
Message-ID: <CABBYNZLh_J2_y_RcAaMshgiQVYKWGmxyGSZ23p1gs9+D5sUp=A@mail.gmail.com>
Subject: Re: [BlueZ PATCH v6 3/8] adapter: remove quality report from
 experimental features
To:     Joseph Hwang <josephsih@google.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Marcel Holtmann <marcel@holtmann.org>,
        =?UTF-8?Q?Pali_Roh=C3=A1r?= <pali@kernel.org>,
        ChromeOS Bluetooth Upstreaming 
        <chromeos-bluetooth-upstreaming@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Joseph,

On Thu, May 26, 2022 at 8:13 PM Joseph Hwang <josephsih@google.com> wrote:
>
> Thanks Luiz for the comment! Does it mean that bluez should 1) try to
> use MGMT_OP_SET_QUALITY_REPORT; and if it failed, use the experimental
> feature EXP_FEAT_BQR instead?

Yep, in fact you should probably keep the existing code as it since it
only set BQR if the kernel supports it anyway, that said how you are
planning to support MGMT_OP_SET_QUALITY_REPORT since that is not
enabled via UUID even if we can detect its support by the kernel Ive
assumed it wouldn't be enabled all the time or that safe to be always
enabled?

> Regards,
> Joseph
>
>
> On Fri, May 27, 2022 at 4:31 AM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Joseph,
> >
> > On Thu, May 26, 2022 at 4:25 AM Joseph Hwang <josephsih@chromium.org> wrote:
> > >
> > > The quality report feature is now enabled through
> > > MGMT_OP_SET_QUALITY_REPORT instead of through the experimental
> > > features.
> > >
> > > ---
> > >
> > > Changes in v6:
> > > - Fixed a patch conflict.
> > >
> > > Changes in v5:
> > > - Move is_quality_report_supported() implementation to next patch.
> > >   The function does not belong to this patch.
> > >
> > > Changes in v4:
> > > - Move forward this patch in the patch series so that this
> > >   command patch is prior to the quality report event patches.
> > >
> > > Changes in v3:
> > > - This is a new patch that enables the quality report feature via
> > >   MGMT_OP_SET_QUALITY_REPORT.
> > >
> > >  src/adapter.c | 14 --------------
> > >  src/adapter.h |  1 -
> > >  2 files changed, 15 deletions(-)
> > >
> > > diff --git a/src/adapter.c b/src/adapter.c
> > > index f7faaa263..2ceea6e1c 100644
> > > --- a/src/adapter.c
> > > +++ b/src/adapter.c
> > > @@ -120,13 +120,6 @@ static const struct mgmt_exp_uuid le_simult_central_peripheral_uuid = {
> > >         .str = "671b10b5-42c0-4696-9227-eb28d1b049d6"
> > >  };
> > >
> > > -/* 330859bc-7506-492d-9370-9a6f0614037f */
> > > -static const struct mgmt_exp_uuid quality_report_uuid = {
> > > -       .val = { 0x7f, 0x03, 0x14, 0x06, 0x6f, 0x9a, 0x70, 0x93,
> > > -               0x2d, 0x49, 0x06, 0x75, 0xbc, 0x59, 0x08, 0x33 },
> > > -       .str = "330859bc-7506-492d-9370-9a6f0614037f"
> > > -};
> > > -
> > >  /* 15c0a148-c273-11ea-b3de-0242ac130004 */
> > >  static const struct mgmt_exp_uuid rpa_resolution_uuid = {
> > >         .val = { 0x04, 0x00, 0x13, 0xac, 0x42, 0x02, 0xde, 0xb3,
> > > @@ -9621,12 +9614,6 @@ static void le_simult_central_peripheral_func(struct btd_adapter *adapter,
> > >                                 (void *)le_simult_central_peripheral_uuid.val);
> > >  }
> > >
> > > -static void quality_report_func(struct btd_adapter *adapter, uint8_t action)
> > > -{
> > > -       if (action)
> > > -               queue_push_tail(adapter->exps, (void *)quality_report_uuid.val);
> > > -}
> > > -
> > >  static void set_rpa_resolution_complete(uint8_t status, uint16_t len,
> > >                                         const void *param, void *user_data)
> > >  {
> > > @@ -9703,7 +9690,6 @@ static const struct exp_feat {
> > >         EXP_FEAT(EXP_FEAT_DEBUG, &debug_uuid, exp_debug_func),
> > >         EXP_FEAT(EXP_FEAT_LE_SIMULT_ROLES, &le_simult_central_peripheral_uuid,
> > >                  le_simult_central_peripheral_func),
> > > -       EXP_FEAT(EXP_FEAT_BQR, &quality_report_uuid, quality_report_func),
> > >         EXP_FEAT(EXP_FEAT_RPA_RESOLUTION, &rpa_resolution_uuid,
> > >                 rpa_resolution_func),
> > >         EXP_FEAT(EXP_FEAT_CODEC_OFFLOAD, &codec_offload_uuid,
> > > diff --git a/src/adapter.h b/src/adapter.h
> > > index 688ed51c6..3d53a962d 100644
> > > --- a/src/adapter.h
> > > +++ b/src/adapter.h
> > > @@ -257,7 +257,6 @@ bool btd_le_connect_before_pairing(void);
> > >  enum experimental_features {
> > >         EXP_FEAT_DEBUG                  = 1 << 0,
> > >         EXP_FEAT_LE_SIMULT_ROLES        = 1 << 1,
> > > -       EXP_FEAT_BQR                    = 1 << 2,
> > >         EXP_FEAT_RPA_RESOLUTION         = 1 << 3,
> > >         EXP_FEAT_CODEC_OFFLOAD          = 1 << 4,
> > >  };
> >
> > We can't remove existing experimental features since there could be
> > kernels which don't support MGMT_OP_SET_QUALITY_REPORT, so this will
> > need to stay for as long as we support such kernels versions.
> >
> > > --
> > > 2.36.1.124.g0e6072fb45-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
>
>
> --
>
> Joseph Shyh-In Hwang
> Email: josephsih@google.com


-- 
Luiz Augusto von Dentz