Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-4352-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: "K, Kiran" <kiran.k@intel.com>
To: Bjorn Helgaas <helgaas@kernel.org>
CC: "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
	"Srivatsa, Ravishankar" <ravishankar.srivatsa@intel.com>, "Tumkur Narayan,
 Chethan" <chethan.tumkur.narayan@intel.com>, "Devegowda, Chandrashekar"
	<chandrashekar.devegowda@intel.com>
Subject: RE: [PATCH v1 3/3] Bluetooth: btintel_pcie: Add *setup* function to
 download firmware
Thread-Topic: [PATCH v1 3/3] Bluetooth: btintel_pcie: Add *setup* function to
 download firmware
Thread-Index: AQHagP/adJNL3NvsIE6GVsdD7VhRo7FNYx6AgD6oD/A=
Date: Tue, 7 May 2024 13:59:04 +0000
Message-ID: <PH0PR11MB7585E68BE8E075D7349057E6F5E42@PH0PR11MB7585.namprd11.prod.outlook.com>
References: <20240328111904.992068-3-kiran.k@intel.com>
 <20240328170831.GA1570559@bhelgaas>
In-Reply-To: <20240328170831.GA1570559@bhelgaas>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?+/Y0PlJEkUIGO/OvkWcVHeebuaGO5lnDzhlHExqBaw6mzMZEqYS8/u3EAq8a?=
 =?us-ascii?Q?6pT3H8wsgFy2ZV56fNAoVDHiShzKNvi0kWGs/dwgiC8+0WRb8IgtXaPvdEmC?=
 =?us-ascii?Q?xJFkJleGaVVQwxEHm7Rc+IhqeAOOKslDLlpZgasVyppIQix9lkAw5XMGbScE?=
 =?us-ascii?Q?3hwmz0c2XTl2f8Dn5cOUBgkOxucn4DwXag9YCteLH/NrW4PLPjqXudJU2KAf?=
 =?us-ascii?Q?Iq7lE185u0oyWU5uQMO//6UAbdr2fZoZEwOmXTarGcKPx14Rm/cYICcRWIbm?=
 =?us-ascii?Q?/O6hLzsZOnCsNbiKbCS6iya4tRrq15zYD0YEJqbmcQxETWiX7wDOiSnJ5q37?=
 =?us-ascii?Q?RAfQte8URO1/OEuo6SVPVO5u3nNywZuBJM0OeDHKLnh88HKt+Ksuos5l1efl?=
 =?us-ascii?Q?WbKzpdmrh0Iszz8z6sCbhWqW59sb9ti7nvJl5DPCyphOkudsaoWQ8vRTu+hP?=
 =?us-ascii?Q?la9+89XwAj7gjryfl5KM2tY403NBTPf/jxym1qCW8f0i3g1rtQE4sXJuq1wC?=
 =?us-ascii?Q?A2rfCmjh0IJVMOcCc2waTX8/ZbBbVRhqpiYk3I9gAzOg6CiCDt2Q6713ZcKL?=
 =?us-ascii?Q?Yvn1pqqDwB8nHtIgAor3W6/Hq3cWscyrH9wZmQAzpxnmwDDCvEX1gZ9LNwFY?=
 =?us-ascii?Q?p9+zpRJFDIGe9REdXsCRL7ywB8GiqEidgv7C+jFJW+uPnnQwQBHmlMDPRH4q?=
 =?us-ascii?Q?DufJUUiNQBD+iRb3B8pT4Ka8D4z0KTRPUQ1pYxtdYnDvBqPePB2TdawnE3jW?=
 =?us-ascii?Q?LTcF191WSkkzRhkm68Q0MgxESjgGkkTkSm6bbjgABI2Mk1EYHnLuKvguS1x8?=
 =?us-ascii?Q?o22CQ8e/4U/M0Jd0iYZAzwaLPe3hH4hmW7+dVMjEvEhoXSIiVBqk0e6bU3nb?=
 =?us-ascii?Q?nJIBCKFWqrzd266HkbsSwd5j16DKyUwgo9EbjDx+TeyXnKOOppREgOfsIp3G?=
 =?us-ascii?Q?i/ZBdP4QUsbEIWw5tjPL2+Hl+Q1jJzo+uLfn5T5obylTJFjT2beEjJkXTH8i?=
 =?us-ascii?Q?Xh3cJDeSI3h85HyLLk+yL8kvij6th6vwe3pMOd+aLFLgYuDxBWu9ijxhwGo6?=
 =?us-ascii?Q?AEoR4/gMFV/W+zJDVJoINJLSz/vK0QKor7Swv8hJf8uLROYqrQ3hLIyG/NvS?=
 =?us-ascii?Q?8C77oERJFYAe5vj8v7lzcAaCEc9LwLqJUh1Zp/xDReNJXez31P7J/Z6l+45W?=
 =?us-ascii?Q?WLyn7jX8iaCu3zRtA+5XOnWKYYVbWLW6k4NFwJ4DfpjqEznynZ7T0202f/5K?=
 =?us-ascii?Q?kchDBWEPiNiCG8BVyYPcHxaQayJ8Sp2pdi7q7w8qpvbPCtAKCEPX4feFoDmO?=
 =?us-ascii?Q?93sfsN+a82+0a5Cqbc5jdvrhGRDLYIKZyzMjBso8GyTMppyq1yJX02RNs3iR?=
 =?us-ascii?Q?fap3SGACoBzf5LtocKqyqm0pTo5ebrE/NJWNL6aWBgl3CltrYE5uSSyv9vw+?=
 =?us-ascii?Q?F7FSwQ+E/eG/6YEHuVJ6cM0u8qnHxjvqK3j15x92tn9EySpsahoeGCM7obaR?=
 =?us-ascii?Q?HAWYqFPHRx9NBqB/vnavO/a9Ch0rYN4cLHJ9npIu9iAJZjhgRGDdYx/4lZM1?=
 =?us-ascii?Q?xm9RXJduoIcHm2jafYg=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-bluetooth@vger.kernel.org
List-Id: <linux-bluetooth.vger.kernel.org>
List-Subscribe: <mailto:linux-bluetooth+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-bluetooth+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB7585.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 19ccdc30-5d89-4c63-47b5-08dc6e9ddb66
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 May 2024 13:59:04.3516
 (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: v0HdfO33TuoAft6S80mc2ouEX3+Y37AvFSBStUfw9rR5I5PWEJTBWOYmaOuFoTEKD5bdX1bV33B/YrFRSGzZrQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB7421
X-OriginatorOrg: intel.com

Hi Bjorn,

Thanks for your comments.

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Thursday, March 28, 2024 10:39 PM
> To: K, Kiran <kiran.k@intel.com>
> Cc: linux-bluetooth@vger.kernel.org; Srivatsa, Ravishankar
> <ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@intel.com>; Devegowda, Chandrashekar
> <chandrashekar.devegowda@intel.com>
> Subject: Re: [PATCH v1 3/3] Bluetooth: btintel_pcie: Add *setup* function=
 to
> download firmware
>=20
> On Thu, Mar 28, 2024 at 04:49:04PM +0530, Kiran K wrote:
> > Add to support to download firmware.
>=20
> s/Add to/Add/
>=20
Ack.

> > +static void btintel_pcie_prepare_tx(struct txq *txq, u16 tfd_index,
> > +				    struct sk_buff *skb)
> > +{
> > +	struct data_buf *buf;
> > +	struct tfd *tfd;
> > +
> > +	tfd =3D &txq->tfds[tfd_index];
> > +	memset(tfd, 0, sizeof(*tfd));
> > +
> > +	/* Get the buffer of the tfd index for DMA */
>=20
> s/tfd/TFD/ for consistency.
Ack.

>=20
> > +static int btintel_pcie_hci_send_frame(struct hci_dev *hdev,
> > +				       struct sk_buff *skb)
> > +{
> > +	struct btintel_pcie_data *data =3D hci_get_drvdata(hdev);
> > +	int ret;
> > +	u32 type;
> > +
> > +	/* Due to the fw limitation, the type header of the packet should be
> > +	 * 4 bytes unlikely 1 byte for UART. In UART, the firmware can reads
> > +	 * the first byte to get the packet type and redirect the rest of dat=
a
> > +	 * packet to the right handler. But for PCIe, THF(Transfer Flow Handl=
er)
> > +	 * fetches the 4 bytes of data from DMA memory and by the time it
> reads
> > +	 * the first 4 bytes, it already consumes some part of packet. Thus
> > +	 * the packet type indicator for iBT PCIe is 4 bytes.
> > +	 * Luckily, when HCI core creates the skb, it allocated 8 bytes of
> > +	 * head room for profile and driver use, and before sending the data
> > +	 * to the device, append the iBT PCIe packet type in the front.
>=20
> s/unlikely/unlike/
> s/can reads/can read/
> s/it already consumes/it has already consumed/
>=20
> Add blank line between paragraphs.
Ack.

>=20
> > +	 */
> > +	switch (hci_skb_pkt_type(skb)) {
> > +	case HCI_COMMAND_PKT:
> > +		type =3D BTINTEL_PCIE_HCI_CMD_PKT;
> > +		if (btintel_test_flag(hdev, INTEL_BOOTLOADER)) {
> > +			struct hci_command_hdr *cmd =3D (void *)skb->data;
> > +			__u16 opcode =3D le16_to_cpu(cmd->opcode);
> > +
> > +			/* When the 0xfc01 command is issued to boot into
> > +			 * the operational firmware, it will actually not
> > +			 * send a command complete event. To keep the flow
> > +			 * control working inject that event here.
> > +			 */
> > +			if (opcode =3D=3D 0xfc01)
> > +				btintel_pcie_inject_cmd_complete(hdev,
> opcode);
> > +		}
> > +		hdev->stat.cmd_tx++;
> > +		break;
> > +	case HCI_ACLDATA_PKT:
> > +		type =3D BTINTEL_PCIE_HCI_ACL_PKT;
> > +		hdev->stat.acl_tx++;
> > +		break;
> > +	case HCI_SCODATA_PKT:
> > +		type =3D BTINTEL_PCIE_HCI_SCO_PKT;
> > +		hdev->stat.sco_tx++;
> > +		break;
> > +	default:
> > +		bt_dev_err(hdev, "Unknown HCI packet type");
> > +		ret =3D -EILSEQ;
> > +		goto exit_error;
> > +	}
> > +	memcpy(skb_push(skb, BTINTEL_PCIE_HCI_TYPE_LEN), &type,
> > +	       BTINTEL_PCIE_HCI_TYPE_LEN);
> > +
> > +	ret =3D btintel_pcie_send_sync(data, skb);
> > +	if (ret) {
> > +		hdev->stat.err_tx++;
> > +		bt_dev_err(hdev, "Failed to send frame (%d)", ret);
> > +		goto exit_error;
> > +	} else {
> > +		hdev->stat.byte_tx +=3D skb->len;
> > +		kfree_skb(skb);
> > +	}
> > +
> > +exit_error:
> > +
> > +	return ret;
>=20
> There's no cleanup here, so it would be simpler to omit "ret"
> completely and return directly above instead of using the goto.
>=20
Ack.

> > +}

Thanks,
Kiran