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: <20210609114029.1656-1-kiran.k@intel.com> <E6C00EEB-8D02-4EF1-87FD-75E58023BA67@holtmann.org>
In-Reply-To: <E6C00EEB-8D02-4EF1-87FD-75E58023BA67@holtmann.org>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Wed, 9 Jun 2021 15:32:10 -0700
Message-ID: <CABBYNZKL73_Z7x2-by4cFbx4rHiyedsYQcfrO5a52BCy3ATt2g@mail.gmail.com>
Subject: Re: [PATCH v1] Bluetooth: btintel: Support Digital(N) + RF(N-1) combination
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     Kiran K <kiran.k@intel.com>,
        Bluez mailing list <linux-bluetooth@vger.kernel.org>,
        Tedd Ho-Jeong An <tedd.an@intel.com>,
        Luiz Augusto von Dentz <luiz.von.dentz@intel.com>,
        "Tumkur Narayan, Chethan" <chethan.tumkur.narayan@intel.com>,
        "Srivatsa, Ravishankar" <ravishankar.srivatsa@intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Marcel, Kiran,

On Wed, Jun 9, 2021 at 12:15 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Kiran,
>
> > New generation Intel controllers(N) need to support RF from (N-1)
> > generation. Since PID comes from OTP present in RF module,
> > *setup* function gets mapped to BTUSB_INTEL_NEW instead of
> > BTUSB_INTEL_NEWGEN. This patch checks generation of CNVi in
> > *setup* of BTUSB_INTEL_NEW and maps callbacks to BTUSB_INTEL_NEWGEN
> > if new generation controller is found and attempts *setup* of
> > BTUSB_INTEL_NEWGEN.
> >
> > Signed-off-by: Kiran K <kiran.k@intel.com>
> > Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> > Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> > ---
> > drivers/bluetooth/btintel.c | 119 ++++++++++++++++++++++++++++++++++++
> > drivers/bluetooth/btintel.h |  10 +++
> > drivers/bluetooth/btusb.c   |  45 +++++++++++++-
> > net/bluetooth/hci_core.c    |   5 +-
> > 4 files changed, 177 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> > index e44b6993cf91..1d9ecc481f14 100644
> > --- a/drivers/bluetooth/btintel.c
> > +++ b/drivers/bluetooth/btintel.c
> > @@ -483,6 +483,85 @@ int btintel_version_info_tlv(struct hci_dev *hdev, struct intel_version_tlv *ver
> > }
> > EXPORT_SYMBOL_GPL(btintel_version_info_tlv);
> >
> > +void btintel_parse_version_tlv(struct hci_dev *hdev, struct sk_buff *skb,
> > +                            struct intel_version_tlv *version)
> > +{
> > +     /* Consume Command Complete Status field */
> > +     skb_pull(skb, sizeof(__u8));
> > +
> > +     /* Event parameters contatin multiple TLVs. Read each of them
> > +      * and only keep the required data. Also, it use existing legacy
> > +      * version field like hw_platform, hw_variant, and fw_variant
> > +      * to keep the existing setup flow
> > +      */
> > +     while (skb->len) {
> > +             struct intel_tlv *tlv;
> > +
> > +             tlv = (struct intel_tlv *)skb->data;
> > +             switch (tlv->type) {
> > +             case INTEL_TLV_CNVI_TOP:
> > +                     version->cnvi_top = get_unaligned_le32(tlv->val);
> > +                     break;
>
> I think we already had this issue that you need to check that enough data is actually in the SKB.
>
> > +             case INTEL_TLV_CNVR_TOP:
> > +                     version->cnvr_top = get_unaligned_le32(tlv->val);
> > +                     break;
> > +             case INTEL_TLV_CNVI_BT:
> > +                     version->cnvi_bt = get_unaligned_le32(tlv->val);
> > +                     break;
> > +             case INTEL_TLV_CNVR_BT:
> > +                     version->cnvr_bt = get_unaligned_le32(tlv->val);
> > +                     break;
> > +             case INTEL_TLV_DEV_REV_ID:
> > +                     version->dev_rev_id = get_unaligned_le16(tlv->val);
> > +                     break;
> > +             case INTEL_TLV_IMAGE_TYPE:
> > +                     version->img_type = tlv->val[0];
> > +                     break;
> > +             case INTEL_TLV_TIME_STAMP:
> > +                     version->timestamp = get_unaligned_le16(tlv->val);
> > +                     break;
> > +             case INTEL_TLV_BUILD_TYPE:
> > +                     version->build_type = tlv->val[0];
> > +                     break;
> > +             case INTEL_TLV_BUILD_NUM:
> > +                     version->build_num = get_unaligned_le32(tlv->val);
> > +                     break;
> > +             case INTEL_TLV_SECURE_BOOT:
> > +                     version->secure_boot = tlv->val[0];
> > +                     break;
> > +             case INTEL_TLV_OTP_LOCK:
> > +                     version->otp_lock = tlv->val[0];
> > +                     break;
> > +             case INTEL_TLV_API_LOCK:
> > +                     version->api_lock = tlv->val[0];
> > +                     break;
> > +             case INTEL_TLV_DEBUG_LOCK:
> > +                     version->debug_lock = tlv->val[0];
> > +                     break;
> > +             case INTEL_TLV_MIN_FW:
> > +                     version->min_fw_build_nn = tlv->val[0];
> > +                     version->min_fw_build_cw = tlv->val[1];
> > +                     version->min_fw_build_yy = tlv->val[2];
> > +                     break;
> > +             case INTEL_TLV_LIMITED_CCE:
> > +                     version->limited_cce = tlv->val[0];
> > +                     break;
> > +             case INTEL_TLV_SBE_TYPE:
> > +                     version->sbe_type = tlv->val[0];
> > +                     break;
> > +             case INTEL_TLV_OTP_BDADDR:
> > +                     memcpy(&version->otp_bd_addr, tlv->val, tlv->len);
> > +                     break;
> > +             default:
> > +                     /* Ignore rest of information */
> > +                     break;
> > +             }
> > +             /* consume the current tlv and move to next*/
> > +             skb_pull(skb, tlv->len + sizeof(*tlv));
> > +     }
> > +}
> > +EXPORT_SYMBOL_GPL(btintel_parse_version_tlv);
> > +
> > int btintel_read_version_tlv(struct hci_dev *hdev, struct intel_version_tlv *version)
> > {
> >       struct sk_buff *skb;
> > @@ -595,6 +674,46 @@ int btintel_read_version_tlv(struct hci_dev *hdev, struct intel_version_tlv *ver
> > }
> > EXPORT_SYMBOL_GPL(btintel_read_version_tlv);
> >
> > +int btintel_generic_read_version(struct hci_dev *hdev,
> > +                              struct intel_version_tlv *ver_tlv,
> > +                              struct intel_version *ver, bool *is_tlv)
> > +{
> > +     struct sk_buff *skb;
> > +     const u8 param[1] = { 0xFF };
> > +
> > +     skb = __hci_cmd_sync(hdev, 0xfc05, 1, param, HCI_CMD_TIMEOUT);
> > +     if (IS_ERR(skb)) {
> > +             bt_dev_err(hdev, "Reading Intel version information failed (%ld)",
> > +                        PTR_ERR(skb));
> > +             return PTR_ERR(skb);
> > +     }
> > +
> > +     if (skb->data[0]) {
> > +             bt_dev_err(hdev, "Intel Read Version command failed (%02x)",
> > +                        skb->data[0]);
> > +             kfree_skb(skb);
> > +             return -EIO;
> > +     }
> > +
> > +     if (skb->len < sizeof(struct intel_version))
> > +             return -EILSEQ;
> > +
> > +     if (skb->len == sizeof(struct intel_version) &&
> > +         skb->data[1] == 0x37)
> > +             *is_tlv = false;
> > +     else
> > +             *is_tlv = true;
> > +
> > +     if (*is_tlv)
> > +             btintel_parse_version_tlv(hdev, skb, ver_tlv);
> > +     else
> > +             memcpy(ver, skb->data, sizeof(*ver));
> > +
> > +     kfree_skb(skb);
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(btintel_generic_read_version);
> > +
>
> I have the feeling that we are falling back to a patch that I already rejected.
>
> > /* ------- REGMAP IBT SUPPORT ------- */
> >
> > #define IBT_REG_MODE_8BIT  0x00
> > diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
> > index d184064a5e7c..366cb746f9c4 100644
> > --- a/drivers/bluetooth/btintel.h
> > +++ b/drivers/bluetooth/btintel.h
> > @@ -175,6 +175,10 @@ int btintel_read_debug_features(struct hci_dev *hdev,
> >                               struct intel_debug_features *features);
> > int btintel_set_debug_features(struct hci_dev *hdev,
> >                              const struct intel_debug_features *features);
> > +int btintel_generic_read_version(struct hci_dev *hdev,
> > +                              struct intel_version_tlv *ver_tlv,
> > +                              struct intel_version *ver,
> > +                              bool *is_tlv);
> > #else
> >
> > static inline int btintel_check_bdaddr(struct hci_dev *hdev)
> > @@ -307,4 +311,10 @@ static inline int btintel_set_debug_features(struct hci_dev *hdev,
> >       return -EOPNOTSUPP;
> > }
> >
> > +static int btintel_generic_read_version(struct hci_dev *hdev,
> > +                                     struct intel_version_tlv *ver_tlv,
> > +                                     struct intel_version *ver, bool *is_tlv)
> > +{
> > +     return -EOPNOTSUPP;
> > +}
> > #endif
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index a9855a2dd561..15d91aae52cc 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -583,6 +583,9 @@ struct btusb_data {
> >       unsigned cmd_timeout_cnt;
> > };
> >
> > +static int btusb_setup_intel_newgen(struct hci_dev *hdev);
> > +static int btusb_shutdown_intel_new(struct hci_dev *hdev);
> > +
> > static void btusb_intel_cmd_timeout(struct hci_dev *hdev)
> > {
> >       struct btusb_data *data = hci_get_drvdata(hdev);
> > @@ -2842,6 +2845,18 @@ static int btusb_intel_boot(struct hci_dev *hdev, u32 boot_addr)
> >       return err;
> > }
> >
> > +static bool btintel_is_newgen_controller(struct hci_dev *hdev, u32 cnvi)
> > +{
> > +     switch (cnvi & 0xFFF) {
> > +     case 0x400: /* Slr */
> > +     case 0x401: /* Slr-F */
> > +     case 0x410: /* TyP */
> > +     case 0x810: /* Mgr */
> > +             return true;
> > +     }
> > +     return false;
> > +}
> > +
> > static int btusb_setup_intel_new(struct hci_dev *hdev)
> > {
> >       struct btusb_data *data = hci_get_drvdata(hdev);
> > @@ -2851,6 +2866,8 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
> >       char ddcname[64];
> >       int err;
> >       struct intel_debug_features features;
> > +     struct intel_version_tlv ver_tlv;
> > +     bool is_tlv;
> >
> >       BT_DBG("%s", hdev->name);
> >
> > @@ -2864,12 +2881,38 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
> >        * is in bootloader mode or if it already has operational firmware
> >        * loaded.
> >        */
> > -     err = btintel_read_version(hdev, &ver);
> > +     err = btintel_generic_read_version(hdev, &ver_tlv, &ver, &is_tlv);
> >       if (err) {
> >               bt_dev_err(hdev, "Intel Read version failed (%d)", err);
> >               btintel_reset_to_bootloader(hdev);
> >               return err;
> >       }
> > +     if (is_tlv) {
> > +             /* We got TLV data. Check for new generation CNVi. If present,
> > +              * then map the callbacks to BTUSB_INTEL_NEWGEN and attempt
> > +              * setup function again
> > +              */
> > +             if (btintel_is_newgen_controller(hdev, ver_tlv.cnvi_top)) {
> > +                     hdev->send = btusb_send_frame_intel;
> > +                     hdev->setup = btusb_setup_intel_newgen;
>
> So this is a clear no, your are not changing hdev->setup within hdev->setup.
>
> > +                     hdev->shutdown = btusb_shutdown_intel_new;
> > +                     hdev->hw_error = btintel_hw_error;
> > +                     hdev->set_diag = btintel_set_diag;
> > +                     hdev->set_bdaddr = btintel_set_bdaddr;
> > +                     hdev->cmd_timeout = btusb_intel_cmd_timeout;
> > +                     return -EAGAIN;
> > +             }
> > +
> > +             /* we need to read legacy version here to minimize the changes
> > +              * and keep the esixiting flow
> > +              */
> > +             err = btintel_read_version(hdev, &ver);
> > +             if (err) {
> > +                     bt_dev_err(hdev, "Intel Read version failed (%d)", err);
> > +                     btintel_reset_to_bootloader(hdev);
> > +                     return err;
> > +             }
> > +     }
> >
> >       err = btintel_version_info(hdev, &ver);
> >       if (err)
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index 1eb7ffd0dd29..8e407bad0e31 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -1496,8 +1496,11 @@ static int hci_dev_do_open(struct hci_dev *hdev)
> >
> >               hci_sock_dev_event(hdev, HCI_DEV_SETUP);
> >
> > -             if (hdev->setup)
> > +             if (hdev->setup) {
> >                       ret = hdev->setup(hdev);
> > +                     if (ret && ret == -EAGAIN)
> > +                             ret = hdev->setup(hdev);
> > +             }
>
> NO. Please stop hacking here. I think you need to take a whiteboard and draw how our controllers are initialized.

+1

It is already strange that we have mixed generation, besides we never
really did a good job tracking the generation properly, but with this
it seems we are attempting to mix generations so we no longer can
detect them based on USB PID/VID but need to dig into the version
information at runtime, so imo we should either detected based on USB
PID/VID or if that is not possible then switch it to be runtime only
and not try to do both as it should be clear by now that will just
make the code really hard to follow.

-- 
Luiz Augusto von Dentz