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;
IronPort-SDR: Qo5eSyrZyQ8dT3x41+TNDcJYeE6aTFFWT3zsnTDcbmn+kIQv0hzSRCpR+KvnkW4g4jOrnBSuOA
 jAKw1Tw0cK8w==
IronPort-SDR: pEZPBZ9BaE1CtKxSURVq5+m9WZgS3Xds9FMogfJWVTVV3djtprm/8N52ltZCr91O0ub55e5jKd
 X+JWnFPni3pQ==
From:   "Narasimman, Sathish" <sathish.narasimman@intel.com>
To:     "'linux-bluetooth@vger.kernel.org'" <linux-bluetooth@vger.kernel.org>
CC:     "Tumkur Narayan, Chethan" <chethan.tumkur.narayan@intel.com>,
        "Srivatsa, Ravishankar" <ravishankar.srivatsa@intel.com>,
        "K, Kiran" <kiran.k@intel.com>,
        "Narasimman, Sathish" <sathish.narasimman@intel.com>
Subject: RE: [PATCH v3] Bluetooth: btintel parse TLV structure
Thread-Topic: [PATCH v3] Bluetooth: btintel parse TLV structure
Thread-Index: AQHWtB1rM6CQhzntHkOiqKGk6aUoPKnLvmXAgAlx94CABm8EsA==
Date:   Fri, 27 Nov 2020 06:09:46 +0000
Message-ID: <SN6PR11MB2703D767EBADAA02A3BB7BBE94F80@SN6PR11MB2703.namprd11.prod.outlook.com>
References: <20201106092119.20486-1-sathish.narasimman@intel.com>
 <SN6PR11MB27034462DA5BA784215D760794E20@SN6PR11MB2703.namprd11.prod.outlook.com>
 <SN6PR11MB27032D732BABC78757CAEC3A94FC0@SN6PR11MB2703.namprd11.prod.outlook.com>
In-Reply-To: <SN6PR11MB27032D732BABC78757CAEC3A94FC0@SN6PR11MB2703.namprd11.prod.outlook.com>
Accept-Language: en-IN, en-US
Content-Language: en-US
dlp-version: 11.5.1.3
dlp-reaction: no-action
dlp-product: dlpe-windows
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: dead733f-c3bb-46b0-ad52-08d8929b0a8e
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Nov 2020 06:09:46.6584
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: rh4a/hSlGlqHBASUORgFIfPPuBuCbC0db+qbwwsfqemOYcKYGbZcQLQZydGg7atnEVRewA2/n24jlVF49bjcxbqK9CGtJqKYTjSAEWjVhAc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR11MB2976
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-bluetooth.vger.kernel.org>
X-Mailing-List: linux-bluetooth@vger.kernel.org

Hi

> -----Original Message-----
> From: Narasimman, Sathish <sathish.narasimman@intel.com>
> Sent: Monday, November 23, 2020 9:25 AM
> To: 'linux-bluetooth@vger.kernel.org' <linux-bluetooth@vger.kernel.org>
> Cc: Tumkur Narayan, Chethan <chethan.tumkur.narayan@intel.com>;
> Srivatsa, Ravishankar <ravishankar.srivatsa@intel.com>; K, Kiran
> <kiran.k@intel.com>; Narasimman, Sathish
> <sathish.narasimman@intel.com>
> Subject: RE: [PATCH v3] Bluetooth: btintel parse TLV structure
>=20
> Hi
>=20
> > -----Original Message-----
> > From: Narasimman, Sathish
> > Sent: Tuesday, November 17, 2020 9:11 AM
> > To: linux-bluetooth@vger.kernel.org
> > Cc: Tumkur Narayan, Chethan <chethan.tumkur.narayan@intel.com>;
> > Srivatsa, Ravishankar <ravishankar.srivatsa@intel.com>; K, Kiran
> > <kiran.k@intel.com>
> > Subject: RE: [PATCH v3] Bluetooth: btintel parse TLV structure
> >
> > Hi
> >
> > > -----Original Message-----
> > > From: Narasimman, Sathish <sathish.narasimman@intel.com>
> > > Sent: Friday, November 6, 2020 2:51 PM
> > > To: linux-bluetooth@vger.kernel.org
> > > Cc: Tumkur Narayan, Chethan <chethan.tumkur.narayan@intel.com>;
> > > Srivatsa, Ravishankar <ravishankar.srivatsa@intel.com>; Narasimman,
> > > Sathish <sathish.narasimman@intel.com>
> > > Subject: [PATCH v3] Bluetooth: btintel parse TLV structure
> > >
> > > Latest intel firmware supports TLV structure in operational mode for
> > > intel read version. so made changes accordingly to support both
> > > bootloader and operational mode . These changes are only to specific
> > > intel bluetooth controller for example ThP, CcP.
> > >
> > > Signed-off-by: Sathish Narasimman <sathish.narasimman@intel.com>
> > > ---
> > >  drivers/bluetooth/btintel.c | 105
> > > +++++++++++++++++++++++++++---------
> > >  drivers/bluetooth/btintel.h |  16 ++++++
> > >  drivers/bluetooth/btusb.c   |  41 ++++++++++----
> > >  3 files changed, 129 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/drivers/bluetooth/btintel.c
> > > b/drivers/bluetooth/btintel.c index
> > > 88ce5f0ffc4b..67267afc83e1 100644
> > > --- a/drivers/bluetooth/btintel.c
> > > +++ b/drivers/bluetooth/btintel.c
> > > @@ -401,31 +401,9 @@ void btintel_version_info_tlv(struct hci_dev
> > > *hdev, struct intel_version_tlv *ve  }
> > > EXPORT_SYMBOL_GPL(btintel_version_info_tlv);
> > >
> > > -int btintel_read_version_tlv(struct hci_dev *hdev, struct
> > > intel_version_tlv
> > > *version)
> > > +static void btintel_parse_tlv(struct sk_buff *skb,
> > > +			      struct intel_version_tlv *version)
> > >  {
> > > -	struct sk_buff *skb;
> > > -	const u8 param[1] =3D { 0xFF };
> > > -
> > > -	if (!version)
> > > -		return -EINVAL;
> > > -
> > > -	skb =3D __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;
> > > -	}
> > > -
> > > -	/* 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 @@ -
> > > 496,6 +474,85 @@ int btintel_read_version_tlv(struct hci_dev *hdev,
> > > struct intel_version_tlv *ver
> > >  		/* consume the current tlv and move to next*/
> > >  		skb_pull(skb, tlv->len + sizeof(*tlv));
> > >  	}
> > > +}
> > > +
> > > +int btintel_read_version_new(struct hci_dev *hdev, struct
> > > +btintel_version *ver) {
> > > +	struct sk_buff *skb;
> > > +	struct intel_version *version =3D &ver->ver;
> > > +	const u8 param[1] =3D { 0xFF };
> > > +
> > > +	skb =3D __hci_cmd_sync(hdev, 0xfc05, 1, param, HCI_CMD_TIMEOUT);
> > > +	if (IS_ERR(skb)) {
> > > +		bt_dev_err(hdev, "Reading Intel version info 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;
> > > +	}
> > > +
> > > +	/* The new Intel read version is backward compatible for Thp and
> > > CcP
> > > +	 * type cards. when the controller is in bootloader mode the
> > > controller
> > > +	 * response remains same as old intel_read version. For ThP/CcP
> > > cards
> > > +	 * TLV structure supports only during the Operation Mode. The best
> > > way
> > > +	 * to differentiate the read_version response is to check the lengt=
h
> > > +	 * parameter and the first byte of the payload, which is a fixed va=
lue.
> > > +	 * After the status parameter if the payload starts with 0x37(This =
is
> > > +	 * a fixed value) and length of the payload is 10 then it is identi=
fied
> > > +	 * as legacy struct intel_version. In the latest firmware the suppo=
rt
> > > +	 * of TLV structure is added during Operational Firmware.
> > > +	 */
> > > +	if (skb->len =3D=3D sizeof(*version) && skb->data[1] =3D=3D 0x37) {
> > > +		memcpy(version, skb->data, sizeof(*version));
> > > +		ver->tlv_format =3D false;
> > > +		goto finish;
> > > +	}
> > > +
> > > +	/* Consume Command Complete Status field */
> > > +	skb_pull(skb, 1);
> > > +
> > > +	ver->tlv_format =3D true;
> > > +
> > > +	bt_dev_info(hdev, "Parsing TLV Supported intel read version");
> > > +	btintel_parse_tlv(skb, &ver->ver_tlv);
> > > +
> > > +finish:
> > > +	kfree_skb(skb);
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(btintel_read_version_new);
> > > +
> > > +int btintel_read_version_tlv(struct hci_dev *hdev, struct
> > > +intel_version_tlv *version) {
> > > +	struct sk_buff *skb;
> > > +	const u8 param[1] =3D { 0xFF };
> > > +
> > > +	if (!version)
> > > +		return -EINVAL;
> > > +
> > > +	skb =3D __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;
> > > +	}
> > > +
> > > +	/* Consume Command Complete Status field */
> > > +	skb_pull(skb, 1);
> > > +
> > > +	btintel_parse_tlv(skb, version);
> > >
> > >  	kfree_skb(skb);
> > >  	return 0;
> > > diff --git a/drivers/bluetooth/btintel.h
> > > b/drivers/bluetooth/btintel.h index 09346ae308eb..952da44b79de
> > > 100644
> > > --- a/drivers/bluetooth/btintel.h
> > > +++ b/drivers/bluetooth/btintel.h
> > > @@ -132,6 +132,15 @@ struct intel_debug_features {
> > >  	__u8    page1[16];
> > >  } __packed;
> > >
> > > +struct btintel_version {
> > > +	bool tlv_format;
> > > +	union {
> > > +		struct intel_version ver; /*Legacy Intel read version*/
> > > +		struct intel_version_tlv ver_tlv;
> > > +	};
> > > +} __packed;
> > > +
> > > +#define INTEL_HW_VARIANT(cnvx_bt)	((u8)(((cnvx_bt) &
> > 0x003f0000) >> 16))
> > >  #if IS_ENABLED(CONFIG_BT_INTEL)
> > >
> > >  int btintel_check_bdaddr(struct hci_dev *hdev); @@ -151,6 +160,7 @@
> > > int btintel_set_event_mask(struct hci_dev *hdev, bool debug);  int
> > > btintel_set_event_mask_mfg(struct hci_dev *hdev, bool debug);  int
> > > btintel_read_version(struct hci_dev *hdev, struct intel_version
> > > *ver); int btintel_read_version_tlv(struct hci_dev *hdev, struct
> > > intel_version_tlv *ver);
> > > +int btintel_read_version_new(struct hci_dev *hdev, struct
> > > +btintel_version *ver);
> > >
> > >  struct regmap *btintel_regmap_init(struct hci_dev *hdev, u16
> > opcode_read,
> > >  				   u16 opcode_write);
> > > @@ -248,6 +258,12 @@ static inline int
> > > btintel_read_version_tlv(struct hci_dev *hdev,
> > >  	return -EOPNOTSUPP;
> > >  }
> > >
> > > +static inline int btintel_read_version_new(struct hci_dev *hdev,
> > > +					   struct btintel_version *ver) {
> > > +	return -EOPNOTSUPP;
> > > +}
> > > +
> > >  static inline struct regmap *btintel_regmap_init(struct hci_dev *hde=
v,
> > >  						 u16 opcode_read,
> > >  						 u16 opcode_write)
> > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > index 1005b6e8ff74..c63bc8a0c84f 100644
> > > --- a/drivers/bluetooth/btusb.c
> > > +++ b/drivers/bluetooth/btusb.c
> > > @@ -2554,7 +2554,8 @@ static int
> > > btusb_intel_download_firmware(struct
> > > hci_dev *hdev,  static int btusb_setup_intel_new(struct hci_dev *hdev=
)  {
> > >  	struct btusb_data *data =3D hci_get_drvdata(hdev);
> > > -	struct intel_version ver;
> > > +	struct btintel_version bt_ver;
> > > +	u8 hw_variant;
> > >  	struct intel_boot_params params;
> > >  	u32 boot_param;
> > >  	char ddcname[64];
> > > @@ -2577,19 +2578,33 @@ static int btusb_setup_intel_new(struct
> > > hci_dev
> > > *hdev)
> > >  	 * is in bootloader mode or if it already has operational firmware
> > >  	 * loaded.
> > >  	 */
> > > -	err =3D btintel_read_version(hdev, &ver);
> > > +	err =3D btintel_read_version_new(hdev, &bt_ver);
> > >  	if (err) {
> > >  		bt_dev_err(hdev, "Intel Read version failed (%d)", err);
> > >  		btintel_reset_to_bootloader(hdev);
> > >  		return err;
> > >  	}
> > >
> > > -	err =3D btusb_intel_download_firmware(hdev, &ver, &params,
> > > &boot_param);
> > > +	/* If TLV format is supported then it is in Operational Firmware. T=
LV
> > > +	 * structure is supported only in operational mode in latest
> > > Firmware.
> > > +	 */
> > > +	if (bt_ver.tlv_format && bt_ver.ver_tlv.img_type =3D=3D 0x03) {
> > > +		btintel_version_info_tlv(hdev, &bt_ver.ver_tlv);
> > > +		clear_bit(BTUSB_BOOTLOADER, &data->flags);
> > > +		goto finish;
> > > +	}
> > > +
> > > +	err =3D btusb_intel_download_firmware(hdev, &bt_ver.ver,  &params,
> > > +					    &boot_param);
> > >  	if (err)
> > >  		return err;
> > >
> > > -	/* controller is already having an operational firmware */
> > > -	if (ver.fw_variant =3D=3D 0x23)
> > > +	/* In case if old firmware is used, it should be backward compatibl=
e
> > > +	 * to check for operational firmware. only on latest firmware the
> > > +	 * support for TLV structure is added. so check for tlv is not
> > > +	 * required here.
> > > +	 */
> > > +	if (bt_ver.ver.fw_variant =3D=3D 0x23)
> > >  		goto finish;
> > >
> > >  	rettime =3D ktime_get();
> > > @@ -2641,7 +2656,7 @@ static int btusb_setup_intel_new(struct
> > > hci_dev
> > > *hdev)
> > >
> > >  	clear_bit(BTUSB_BOOTLOADER, &data->flags);
> > >
> > > -	err =3D btusb_setup_intel_new_get_fw_name(&ver, &params,
> > > ddcname,
> > > +	err =3D btusb_setup_intel_new_get_fw_name(&bt_ver.ver, &params,
> > > ddcname,
> > >  						sizeof(ddcname), "ddc");
> > >
> > >  	if (!err) {
> > > @@ -2665,17 +2680,25 @@ static int btusb_setup_intel_new(struct
> > > hci_dev
> > > *hdev)
> > >  	btintel_set_debug_features(hdev, &features);
> > >
> > >  	/* Read the Intel version information after loading the FW  */
> > > -	err =3D btintel_read_version(hdev, &ver);
> > > +	err =3D btintel_read_version_new(hdev, &bt_ver);
> > >  	if (err)
> > >  		return err;
> > >
> > > -	btintel_version_info(hdev, &ver);
> > > +	if (bt_ver.tlv_format)
> > > +		btintel_version_info_tlv(hdev, &bt_ver.ver_tlv);
> > > +	else
> > > +		btintel_version_info(hdev, &bt_ver.ver);
> > >
> > >  finish:
> > >  	/* All Intel controllers that support the Microsoft vendor
> > >  	 * extension are using 0xFC1E for VsMsftOpCode.
> > >  	 */
> > > -	switch (ver.hw_variant) {
> > > +	if (!bt_ver.tlv_format)
> > > +		hw_variant =3D bt_ver.ver.hw_variant;
> > > +	else
> > > +		hw_variant =3D INTEL_HW_VARIANT(bt_ver.ver_tlv.cnvi_bt);
> > > +
> > > +	switch (hw_variant) {
> > >  	case 0x12:	/* ThP */
> > >  		hci_set_msft_opcode(hdev, 0xFC1E);
> > >  		break;
> > > --
> > > 2.17.1
> >
> > Gentle reminder
> >
> > Regards
> > Sathish N
>=20
> Gentle reminder
>=20
> Regards
> Sathish N

Please help to review the patches

Regards
Sathish N