From: "K, Kiran" <kiran.k@intel.com>
To: Tedd Ho-Jeong An <hj.tedd.an@gmail.com>,
"linux-bluetooth@vger.kernel.org"
<linux-bluetooth@vger.kernel.org>
Cc: "An, Tedd" <tedd.an@intel.com>
Subject: RE: [PATCH v6 02/12] Bluetooth: btintel: Add combined setup and shutdown functions
Date: Wed, 4 Aug 2021 15:22:14 +0000 [thread overview]
Message-ID: <DM8PR11MB55734A91C7215B1523409D80F5F19@DM8PR11MB5573.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20210804044032.59729-3-hj.tedd.an@gmail.com>
Hi Tedd,
> -----Original Message-----
> From: Tedd Ho-Jeong An <hj.tedd.an@gmail.com>
> Sent: Wednesday, August 4, 2021 10:10 AM
> To: linux-bluetooth@vger.kernel.org
> Cc: An, Tedd <tedd.an@intel.com>
> Subject: [PATCH v6 02/12] Bluetooth: btintel: Add combined setup and
> shutdown functions
>
> From: Tedd Ho-Jeong An <tedd.an@intel.com>
>
> There are multiple setup and shutdown functions for Intel device and the
> setup function to be used is depends on the USB PID/VID, which makes
> difficult to maintain the code and increases the code size.
>
> This patch adds combined setup and shutdown functions to provide a single
> entry point for all Intel devices and choose the setup functions based on the
> information read with HCI_Intel_Read_Version command.
>
> Starting from TyP device, the command and response parameters for
> HCI_Intel_Read_Version command are changed even though OCF remains
> same. However, the legacy devices still can handle the command without
> error even if it has a extra parameter, so to simplify the flow, the new
> command format is used to read the version information for both legacy and
> new (tlv based) format.
>
> Also, it also adds a routine to setup the hdev callbacks in btintel.
>
> Signed-off-by: Tedd Ho-Jeong An <tedd.an@intel.com>
> ---
> drivers/bluetooth/btintel.c | 230 ++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/btintel.h | 6 +
> 2 files changed, 236 insertions(+)
>
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c index
> e44b6993cf91..3d98fc2a64b9 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -236,6 +236,8 @@ int btintel_version_info(struct hci_dev *hdev, struct
> intel_version *ver)
> * compatibility options when newer hardware variants come along.
> */
> switch (ver->hw_variant) {
> + case 0x07: /* WP - Legacy ROM */
> + case 0x08: /* StP - Legacy ROM */
> case 0x0b: /* SfP */
> case 0x0c: /* WsP */
> case 0x11: /* JfP */
> @@ -250,9 +252,15 @@ int btintel_version_info(struct hci_dev *hdev, struct
> intel_version *ver)
> }
>
> switch (ver->fw_variant) {
> + case 0x01:
> + variant = "Legacy ROM 2.5";
> + break;
> case 0x06:
> variant = "Bootloader";
> break;
> + case 0x22:
> + variant = "Legacy ROM 2.x";
> + break;
> case 0x23:
> variant = "Firmware";
> break;
> @@ -483,6 +491,107 @@ int btintel_version_info_tlv(struct hci_dev *hdev,
> struct intel_version_tlv *ver }
> EXPORT_SYMBOL_GPL(btintel_version_info_tlv);
>
> +static int btintel_parse_version_tlv(struct hci_dev *hdev,
> + struct intel_version_tlv *version,
> + struct sk_buff *skb)
> +{
> + /* Consume Command Complete Status field */
> + skb_pull(skb, 1);
> +
> + /* 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;
> +
> + /* Make sure skb has a minimum length of the header */
> + if (skb->len < 2)
> + return -EINVAL;
Can we use sizeof(*tlv) instead of 2 ?
> +
> + tlv = (struct intel_tlv *)skb->data;
> +
> + /* Make sure skb has a enough data */
> + if (skb->len < tlv->len + sizeof(*tlv))
> + return -EINVAL;
> +
> + switch (tlv->type) {
> + case INTEL_TLV_CNVI_TOP:
> + version->cnvi_top = get_unaligned_le32(tlv->val);
> + break;
> + 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:
> + /* If image type is Operational firmware (0x03), then
> + * running FW Calendar Week and Year information
> can
> + * be extracted from Timestamp information
> + */
> + version->min_fw_build_cw = tlv->val[0];
> + version->min_fw_build_yy = tlv->val[1];
> + 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:
> + /* If image type is Operational firmware (0x03), then
> + * running FW build number can be extracted from
> the
> + * Build information
> + */
> + version->min_fw_build_nn = tlv->val[0];
> + 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);
I think we need to restrict copying to only sizeof(version->otp_bd_addr) bytes here.
> + break;
> + default:
> + /* Ignore rest of information */
> + break;
> + }
> + /* consume the current tlv and move to next*/
> + skb_pull(skb, tlv->len + sizeof(*tlv));
> + }
> +
> + return 0;
> +}
> +
> int btintel_read_version_tlv(struct hci_dev *hdev, struct intel_version_tlv
> *version) {
> struct sk_buff *skb;
> @@ -1272,6 +1381,127 @@ int btintel_set_debug_features(struct hci_dev
> *hdev, } EXPORT_SYMBOL_GPL(btintel_set_debug_features);
>
> +static int btintel_setup_combined(struct hci_dev *hdev) {
> + const u8 param[1] = { 0xFF };
> + struct intel_version ver;
> + struct intel_version_tlv ver_tlv;
> + struct sk_buff *skb;
> + int err;
> +
> + BT_DBG("%s", hdev->name);
> +
> + /* Starting from TyP device, the command parameter and response
> are
> + * changed even though the OCF for HCI_Intel_Read_Version
> command
> + * remains same. The legacy devices can handle even if the
> + * command has a parameter and returns a correct version
> information.
> + * So, it uses new format to support both legacy and new format.
> + */
> + skb = __hci_cmd_sync(hdev, 0xfc05, 1, param, HCI_CMD_TIMEOUT);
> + if (IS_ERR(skb)) {
> + bt_dev_err(hdev, "Reading Intel version command failed
> (%ld)",
> + PTR_ERR(skb));
> + return PTR_ERR(skb);
> + }
> +
> + /* Check the status */
> + if (skb->data[0]) {
Need to check for skb->len before accessing data.
> + bt_dev_err(hdev, "Intel Read Version command failed
> (%02x)",
> + skb->data[0]);
> + kfree_skb(skb);
> + return -EIO;
> + }
> +
> + /* For Legacy device, check the HW platform value and size */
> + if (skb->len == sizeof(ver) && skb->data[1] == 0x37) {
> + bt_dev_dbg(hdev, "Read the legacy Intel version
> information");
> +
> + memcpy(&ver, skb->data, sizeof(ver));
> +
> + /* Display version information */
> + btintel_version_info(hdev, &ver);
> +
> + /* Check for supported iBT hardware variants of this
> firmware
> + * loading method.
> + *
> + * This check has been put in place to ensure correct forward
> + * compatibility options when newer hardware variants come
> + * along.
> + */
> + switch (ver.hw_variant) {
> + case 0x07: /* WP */
> + case 0x08: /* StP */
> + /* Legacy ROM product */
> + /* TODO: call setup routine for legacy rom product */
> + break;
> + case 0x0b: /* SfP */
> + case 0x0c: /* WsP */
> + case 0x11: /* JfP */
> + case 0x12: /* ThP */
> + case 0x13: /* HrP */
> + case 0x14: /* CcP */
> + /* TODO: call setup routine for bootloader product
> */
> + break;
> + default:
> + bt_dev_err(hdev, "Unsupported Intel hw variant
> (%u)",
> + ver.hw_variant);
> + return -EINVAL;
Possibility of memory leak here
> + }
> +
> + return err;
Ditto.. plus possibility of returning uninitialized err.
> + }
> +
> + /* For TLV type device, parse the tlv data */
> + err = btintel_parse_version_tlv(hdev, &ver_tlv, skb);
> + if (err) {
> + bt_dev_err(hdev, "Failed to parse TLV version information");
> + return err;
> + }
> +
> + if (INTEL_HW_PLATFORM(ver_tlv.cnvi_bt) != 0x37) {
> + bt_dev_err(hdev, "Unsupported Intel hardware platform
> (0x%2x)",
> + INTEL_HW_PLATFORM(ver_tlv.cnvi_bt));
> + return -EINVAL;
> + }
> +
> + /* Display version information of TLV type */
> + btintel_version_info_tlv(hdev, &ver_tlv);
> +
> + /* TODO: Need to filter the device for new generation */
> + /* TODO: call setup routine for tlv based bootloader product */
> +
> + return err;
> +}
> +
> +static int btintel_shutdown_combined(struct hci_dev *hdev) {
> + struct sk_buff *skb;
> +
> + /* Send HCI Reset to the controller to stop any BT activity which
> + * were triggered. This will help to save power and maintain the
> + * sync b/w Host and controller
> + */
> + skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL,
> HCI_INIT_TIMEOUT);
> + if (IS_ERR(skb)) {
> + bt_dev_err(hdev, "HCI reset during shutdown failed");
> + return PTR_ERR(skb);
> + }
> + kfree_skb(skb);
> +
> + return 0;
> +}
> +
> +int btintel_configure_setup(struct hci_dev *hdev) {
> + /* TODO: Setup hdev callback here */
> + hdev->manufacturer = 2;
> + hdev->setup = btintel_setup_combined;
> + hdev->shutdown = btintel_shutdown_combined;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(btintel_configure_setup);
> +
> MODULE_AUTHOR("Marcel Holtmann <marcel@holtmann.org>");
> MODULE_DESCRIPTION("Bluetooth support for Intel devices ver " VERSION);
> MODULE_VERSION(VERSION); diff --git a/drivers/bluetooth/btintel.h
> b/drivers/bluetooth/btintel.h index d184064a5e7c..dda890d94a07 100644
> --- a/drivers/bluetooth/btintel.h
> +++ b/drivers/bluetooth/btintel.h
> @@ -175,6 +175,7 @@ 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_configure_setup(struct hci_dev *hdev);
> #else
>
> static inline int btintel_check_bdaddr(struct hci_dev *hdev) @@ -307,4
> +308,9 @@ static inline int btintel_set_debug_features(struct hci_dev *hdev,
> return -EOPNOTSUPP;
> }
>
> +static inline int btintel_configure_setup(struct hci_dev *hdev) {
> + return -ENODEV;
> +}
> +
> #endif
> --
> 2.25.1
Thanks,
Kiran
next prev parent reply other threads:[~2021-08-04 15:22 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-04 4:40 [PATCH v6 00/12] Bluetooth: btintel: Refactoring setup routines Tedd Ho-Jeong An
2021-08-04 4:40 ` [PATCH v6 01/12] Bluetooth: Add support hdev to allocate private data Tedd Ho-Jeong An
2021-08-04 5:15 ` Bluetooth: btintel: Refactoring setup routines bluez.test.bot
2021-08-04 4:40 ` [PATCH v6 02/12] Bluetooth: btintel: Add combined setup and shutdown functions Tedd Ho-Jeong An
2021-08-04 15:22 ` K, Kiran [this message]
2021-08-04 4:40 ` [PATCH v6 03/12] Bluetooth: btintel: Refactoring setup routine for legacy ROM sku Tedd Ho-Jeong An
2021-08-04 4:40 ` [PATCH v6 04/12] Bluetooth: btintel: Add btintel data struct Tedd Ho-Jeong An
2021-08-04 4:40 ` [PATCH v6 05/12] Bluetooth: btintel: Fix the first HCI command not work with ROM device Tedd Ho-Jeong An
2021-08-04 4:40 ` [PATCH v6 06/12] Bluetooth: btintel: Fix the LED is not turning off immediately Tedd Ho-Jeong An
2021-08-04 4:40 ` [PATCH v6 07/12] Bluetooth: btintel: Add combined set_diag functions Tedd Ho-Jeong An
2021-08-04 4:40 ` [PATCH v6 08/12] Bluetooth: btintel: Refactoring setup routine for bootloader devices Tedd Ho-Jeong An
2021-08-04 4:40 ` [PATCH v6 09/12] Bluetooth: btintel: Move hci quirks to setup routine Tedd Ho-Jeong An
2021-08-04 4:40 ` [PATCH v6 10/12] Bluetooth: btintel: Clean the exported function to static Tedd Ho-Jeong An
2021-08-04 4:40 ` [PATCH v6 11/12] Bluetooth: btintel: Fix the legacy bootloader returns tlv based version Tedd Ho-Jeong An
2021-08-04 4:40 ` [PATCH v6 12/12] Bluetooth: btintel: Combine setting up MSFT extension Tedd Ho-Jeong An
2021-08-04 15:00 ` [PATCH v6 00/12] Bluetooth: btintel: Refactoring setup routines Marcel Holtmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DM8PR11MB55734A91C7215B1523409D80F5F19@DM8PR11MB5573.namprd11.prod.outlook.com \
--to=kiran.k@intel.com \
--cc=hj.tedd.an@gmail.com \
--cc=linux-bluetooth@vger.kernel.org \
--cc=tedd.an@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.