Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-2923-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 28 Mar 2024 12:08:31 -0500
From: Bjorn Helgaas <helgaas@kernel.org>
To: Kiran K <kiran.k@intel.com>
Cc: linux-bluetooth@vger.kernel.org, ravishankar.srivatsa@intel.com,
	chethan.tumkur.narayan@intel.com,
	Chandrashekar <chandrashekar.devegowda@intel.com>
Subject: Re: [PATCH v1 3/3] Bluetooth: btintel_pcie: Add *setup* function to
 download firmware
Message-ID: <20240328170831.GA1570559@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240328111904.992068-3-kiran.k@intel.com>

On Thu, Mar 28, 2024 at 04:49:04PM +0530, Kiran K wrote:
> Add to support to download firmware.

s/Add to/Add/

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

s/tfd/TFD/ for consistency.

> +static int btintel_pcie_hci_send_frame(struct hci_dev *hdev,
> +				       struct sk_buff *skb)
> +{
> +	struct btintel_pcie_data *data = 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 data
> +	 * packet to the right handler. But for PCIe, THF(Transfer Flow Handler)
> +	 * 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.

s/unlikely/unlike/
s/can reads/can read/
s/it already consumes/it has already consumed/

Add blank line between paragraphs.

> +	 */
> +	switch (hci_skb_pkt_type(skb)) {
> +	case HCI_COMMAND_PKT:
> +		type = BTINTEL_PCIE_HCI_CMD_PKT;
> +		if (btintel_test_flag(hdev, INTEL_BOOTLOADER)) {
> +			struct hci_command_hdr *cmd = (void *)skb->data;
> +			__u16 opcode = 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 == 0xfc01)
> +				btintel_pcie_inject_cmd_complete(hdev, opcode);
> +		}
> +		hdev->stat.cmd_tx++;
> +		break;
> +	case HCI_ACLDATA_PKT:
> +		type = BTINTEL_PCIE_HCI_ACL_PKT;
> +		hdev->stat.acl_tx++;
> +		break;
> +	case HCI_SCODATA_PKT:
> +		type = BTINTEL_PCIE_HCI_SCO_PKT;
> +		hdev->stat.sco_tx++;
> +		break;
> +	default:
> +		bt_dev_err(hdev, "Unknown HCI packet type");
> +		ret = -EILSEQ;
> +		goto exit_error;
> +	}
> +	memcpy(skb_push(skb, BTINTEL_PCIE_HCI_TYPE_LEN), &type,
> +	       BTINTEL_PCIE_HCI_TYPE_LEN);
> +
> +	ret = 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 += skb->len;
> +		kfree_skb(skb);
> +	}
> +
> +exit_error:
> +
> +	return ret;

There's no cleanup here, so it would be simpler to omit "ret"
completely and return directly above instead of using the goto.

> +}