Hi Luiz, Marcel,
Thanks for the comments. I will make a new patch and suggested and send it for review.
Regards,
Kiran
> -----Original Message-----
> From: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> Sent: Thursday, June 10, 2021 4:02 AM
> To: Marcel Holtmann <marcel@xxxxxxxxxxxx>
> Cc: K, Kiran <kiran.k@xxxxxxxxx>; Bluez mailing list <linux-
> bluetooth@xxxxxxxxxxxxxxx>; An, Tedd <tedd.an@xxxxxxxxx>; Von Dentz,
> Luiz <luiz.von.dentz@xxxxxxxxx>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@xxxxxxxxx>; Srivatsa, Ravishankar
> <ravishankar.srivatsa@xxxxxxxxx>
> Subject: Re: [PATCH v1] Bluetooth: btintel: Support Digital(N) + RF(N-1)
> combination
>
> Hi Marcel, Kiran,
>
> On Wed, Jun 9, 2021 at 12:15 PM Marcel Holtmann <marcel@xxxxxxxxxxxx>
> 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@xxxxxxxxx>
> > > Reviewed-by: Chethan T N <chethan.tumkur.narayan@xxxxxxxxx>
> > > Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@xxxxxxxxx>
> > > ---
> > > 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
