Return-Path: <cyrus@holtmann.org>
From: Tedd Ho-Jeong An <tedd.an@intel.com>
To: "Fry, Don" <don.fry@intel.com>
Cc: linux-bluetooth <linux-bluetooth@vger.kernel.org>, marcel <marcel@holtmann.org>, "Hedberg, Johan" <johan.hedberg@intel.com>
Subject: Re: [RFC] Bluetooth: Add support for Intel Bluetooth device [8087:07dc]
Date: Wed, 10 Apr 2013 09:23:59 -0700
Message-ID: <3789870.U0zBdCRI2Y@tedd-ubuntu>
In-Reply-To: <CD8AD196.1D35%don.fry@intel.com>
References: <CD8AD196.1D35%don.fry@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
List-ID: <linux-bluetooth.vger.kernel.org>

Thanks for comments, Don

I saw Marcel's patch this morning and I will incorporate his patch with your comments and send new one.

Tedd

On Wednesday, April 10, 2013 08:40:13 AM Fry, Don wrote:
> Just a couple comments inline.  I know outlook munges whitespaces, so I
> don't comment on those.
> Don
> 
> On 4/9/13 7:44 PM, "An, Tedd" <tedd.an@intel.com> wrote:
> 
> >From: Tedd Ho-Jeong An <tedd.an@intel.com>
> >
> >This patch adds support for Intel Bluetooth device by adding
> >btusb_setup_intel() routine that updates the device with ROM patch
> >during HCI_SETUP.
> 
> <DF> snip
> 
> >
> >+	/* Get bseq file name */
> >+	snprintf(pfile, 32, "intel/%02x%02x%02x%02x%02x%02x%02x%02x%02x.bseq",
> >+				skb->data[1], skb->data[2], skb->data[3],
> >+				skb->data[4], skb->data[5], skb->data[6],
> >+				skb->data[7], skb->data[8], skb->data[9]);
> 
> <DF> Is the length of the data always 9 bytes?  If less data is returned
> garbage will be used.
> 
> >+	kfree_skb(skb);
> >+	BT_DBG("%s patch file: %s", hdev->name, pfile);
> >+
> >
> <DF> snip
> >
> >+		/* Send command based on the evt */
> >+		if (evt->evt == HCI_EV_CMD_COMPLETE) {
> >+			/* Command Complete Event */
> >+			skb = __hci_cmd_sync(hdev, cmd->opcode, cmd->plen,
> 
> <DF> use le16_to_cpu(cmd->opcode) for big endian systems
> 
> >+					(void *)param,
> >+					HCI_INIT_TIMEOUT);
> >+			if (IS_ERR(skb)) {
> >+				BT_ERR("__hci_cmd_sync(patch): %ld",
> >+						PTR_ERR(skb));
> >+				m_off_code = m_off_1;
> >+				goto exit_mfg;
> >+			}
> >+
> >+			/* Check the event status */
> >+			if (skb->data[0]) {
> >+				BT_ERR("%s patch failed(%02x)", hdev->name,
> >+						skb->data[0]);
> >+				m_off_code = m_off_1;
> >+				kfree_skb(skb);
> >+				goto exit_mfg;
> >+			}
> >+		} else {
> >+			/* Non Command Complete Event */
> >+			skb = __hci_cmd_sync_ev(hdev, cmd->opcode, cmd->plen,
> 
> <DF> same as above le16_to_cpu()
> 
> >+					(void *)param, evt->evt,
> >+					HCI_INIT_TIMEOUT);
> >+			if (IS_ERR(skb)) {
> >+				BT_ERR("__hci_cmd_sync_ev(patch): %ld",
> >+						PTR_ERR(skb));
> >+				m_off_code = m_off_1;
> >+				goto exit_mfg;
> >+			}
> >+
> 
> <DF> the length (skb->len) is not checked before this memcmp
> 
> >+			/* Checking the returned event */
> >+			if (memcmp(skb->data, evt_param, evt->plen)) {
> >+				BT_ERR("%s patch event doesn't match!!",
> >+						hdev->name);
> >+				m_off_code = m_off_1;
> >+				kfree_skb(skb);
> >+				goto exit_mfg;
> >+			}
> >+		}
> 
> <DF> The two cases above can be combined and treated identically.
> Just call __hci_cmd_sync_ev with evt->evt and then both can verify the
> returned
> data is what was expected.
> 
> >+		BT_DBG("%s patch cmd succeeded %d of %d",
> >+				hdev->name, patch_curr - fw->data, fw->size);
> >+		kfree_skb(skb);
> >
> <DF> snip
> >
> >+
> > static int btusb_setup(struct hci_dev *hdev)
> > {
> > 	struct btusb_data *data = hci_get_drvdata(hdev);
> > 
> > 	BT_DBG("%s", hdev->name);
> > 
> >+	if (data->driver_info & BTUSB_INTEL) {
> >+		return btusb_setup_intel(hdev);
> 
> <DF> with Marcel's latest patch, setting hdev->setup in the probe routine
> removes this test
> 
> >+	}
> >+
> > 	if (data->driver_info & BTUSB_BCM92035) {
> > 		struct sk_buff *skb;
> > 		__u8 val = 0x00;
>