RE: [PATCH v2 1/2] Bluetooth: btusb: support link statistics telemetry events

[Tumkur Narayan, Chethan]

Hi Luiz,

Thanks for your feedback and sorry for the delay in response.

> Hi Joseph,
> 
> On Tue, Apr 13, 2021 at 12:45 AM Joseph Hwang <josephsih@xxxxxxxxxxxx>
> wrote:
> >
> > From: Chethan T N <chethan.tumkur.narayan@xxxxxxxxx>
> >
> > This patch supports the link statistics telemetry events for Intel
> > controllers
> >
> > To avoid the overhead, this debug feature is disabled by default.
> >
> > Reviewed-by: Miao-chen Chou <mcchou@xxxxxxxxxxxx>
> > Signed-off-by: Chethan T N <chethan.tumkur.narayan@xxxxxxxxx>
> > Signed-off-by: Kiran K <kiran.k@xxxxxxxxx>
> > Signed-off-by: Joseph Hwang <josephsih@xxxxxxxxxxxx>
> > ---
> >
> > Changes in v2:
> > - take care of intel_newgen as well as intel_new
> > - fix the long-line issue
> >
> >  drivers/bluetooth/btintel.c | 20 +++++++++++++++++++-
> >  drivers/bluetooth/btusb.c   | 18 ------------------
> >  2 files changed, 19 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> > index e44b6993cf91..de1dbdc01e5a 100644
> > --- a/drivers/bluetooth/btintel.c
> > +++ b/drivers/bluetooth/btintel.c
> > @@ -1248,8 +1248,10 @@
> EXPORT_SYMBOL_GPL(btintel_read_debug_features);
> >  int btintel_set_debug_features(struct hci_dev *hdev,
> >                                const struct intel_debug_features
> > *features)  {
> > -       u8 mask[11] = { 0x0a, 0x92, 0x02, 0x07, 0x00, 0x00, 0x00, 0x00,
> > +       u8 mask[11] = { 0x0a, 0x92, 0x02, 0x7f, 0x00, 0x00, 0x00,
> > + 0x00,
> >                         0x00, 0x00, 0x00 };
> > +       u8 period[5] = { 0x04, 0x91, 0x02, 0x01, 0x00 };
> > +       u8 trace_enable = 0x02;
> >         struct sk_buff *skb;
> 
> Instead of using a byte array and custo opcode I would recommend we start
> adding proper defines for Intel vendor opcodes with structs for them, similar
> opcodes and structs are also very useful for adding support for btmon, in fact I
> would start with btmon first and only when we have proper decoding support
> start changing the kernel parts so you can actually add btmon output to the
> patch description here.

As of now I don't see any of the VS commands have been defined as per your suggestion. However upcoming patches of btmon tool shall have the decoding of the commands and events for debug purpose as per your recommendation. 
And completely abide to the GPL licensing terms and open for anyone to modify these patches later. I shall rebased these patches; re-send them again.
> 
> >         if (!features)
> > @@ -1266,8 +1268,24 @@ int btintel_set_debug_features(struct hci_dev
> *hdev,
> >                            PTR_ERR(skb));
> >                 return PTR_ERR(skb);
> >         }
> > +       kfree_skb(skb);
> > +
> > +       skb = __hci_cmd_sync(hdev, 0xfc8b, 5, period, HCI_INIT_TIMEOUT);
> > +       if (IS_ERR(skb)) {
> > +               bt_dev_err(hdev, "Setting periodicity for link statistics traces failed
> (%ld)",
> > +                          PTR_ERR(skb));
> > +               return PTR_ERR(skb);
> > +       }
> > +       kfree_skb(skb);
> >
> > +       skb = __hci_cmd_sync(hdev, 0xfca1, 1, &trace_enable,
> HCI_INIT_TIMEOUT);
> > +       if (IS_ERR(skb)) {
> > +               bt_dev_err(hdev, "Enable tracing of link statistics events failed (%ld)",
> > +                          PTR_ERR(skb));
> > +               return PTR_ERR(skb);
> > +       }
> >         kfree_skb(skb);
> > +
> >         return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(btintel_set_debug_features);
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 192cb8c191bc..f29946f15f59 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -2811,7 +2811,6 @@ static int btusb_setup_intel_new(struct hci_dev
> *hdev)
> >         u32 boot_param;
> >         char ddcname[64];
> >         int err;
> > -       struct intel_debug_features features;
> >
> >         BT_DBG("%s", hdev->name);
> >
> > @@ -2865,14 +2864,6 @@ static int btusb_setup_intel_new(struct hci_dev
> *hdev)
> >                 btintel_load_ddc_config(hdev, ddcname);
> >         }
> >
> > -       /* Read the Intel supported features and if new exception formats
> > -        * supported, need to load the additional DDC config to enable.
> > -        */
> > -       btintel_read_debug_features(hdev, &features);
> > -
> > -       /* Set DDC mask for available debug features */
> > -       btintel_set_debug_features(hdev, &features);
> > -
> >         /* Read the Intel version information after loading the FW  */
> >         err = btintel_read_version(hdev, &ver);
> >         if (err)
> > @@ -2911,7 +2902,6 @@ static int btusb_setup_intel_newgen(struct hci_dev
> *hdev)
> >         u32 boot_param;
> >         char ddcname[64];
> >         int err;
> > -       struct intel_debug_features features;
> >         struct intel_version_tlv version;
> >
> >         bt_dev_dbg(hdev, "");
> > @@ -2961,14 +2951,6 @@ static int btusb_setup_intel_newgen(struct hci_dev
> *hdev)
> >          */
> >         btintel_load_ddc_config(hdev, ddcname);
> >
> > -       /* Read the Intel supported features and if new exception formats
> > -        * supported, need to load the additional DDC config to enable.
> > -        */
> > -       btintel_read_debug_features(hdev, &features);
> > -
> > -       /* Set DDC mask for available debug features */
> > -       btintel_set_debug_features(hdev, &features);
> > -
> >         /* Read the Intel version information after loading the FW  */
> >         err = btintel_read_version_tlv(hdev, &version);
> >         if (err)
> > --
> > 2.31.1.295.g9ea45b61b8-goog
> >
> 
> 
> --
> Luiz Augusto von Dentz