Return-Path: From: "An, Tedd" To: Marcel Holtmann CC: "linux-bluetooth@vger.kernel.org" , Johan Hedberg Subject: Re: [RFC v2 1/4] Bluetooth: Refactor Intel_read_version and opens firmware patch file Date: Tue, 9 Dec 2014 23:16:05 +0000 Message-ID: References: <20141208152439.5acd87da@tedd-test> In-Reply-To: Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 List-ID: Hi Marcel, On 12/8/14, 9:34 PM, "Marcel Holtmann" wrote: >Hi Tedd, > >> This patch refactors the routines that read the device version and opens >> firmware patch file. >>=20 >> Signed-off-by: Tedd Ho-Jeong An >> --- >> drivers/bluetooth/btusb.c | 191 >>+++++++++++++++++++++++----------------------- >> 1 file changed, 96 insertions(+), 95 deletions(-) >>=20 >> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c >> index 31dd24a..9ab396b 100644 >> --- a/drivers/bluetooth/btusb.c >> +++ b/drivers/bluetooth/btusb.c >> @@ -1276,6 +1276,51 @@ static int btusb_setup_csr(struct hci_dev *hdev) >> return ret; >> } >>=20 >> +#define BDADDR_INTEL (&(bdaddr_t) {{0x00, 0x8b, 0x9e, 0x19, 0x03, >>0x00}}) >> + >> +static int btusb_check_bdaddr_intel(struct hci_dev *hdev) >> +{ >> + struct sk_buff *skb; >> + struct hci_rp_read_bd_addr *rp; >> + >> + skb =3D __hci_cmd_sync(hdev, HCI_OP_READ_BD_ADDR, 0, NULL, >> + HCI_INIT_TIMEOUT); >> + if (IS_ERR(skb)) { >> + BT_ERR("%s reading Intel device address failed (%ld)", >> + hdev->name, PTR_ERR(skb)); >> + return PTR_ERR(skb); >> + } >> + >> + if (skb->len !=3D sizeof(*rp)) { >> + BT_ERR("%s Intel device address length mismatch", hdev->name); >> + kfree_skb(skb); >> + return -EIO; >> + } >> + >> + rp =3D (struct hci_rp_read_bd_addr *)skb->data; >> + if (rp->status) { >> + BT_ERR("%s Intel device address result failed (%02x)", >> + hdev->name, rp->status); >> + kfree_skb(skb); >> + return -bt_to_errno(rp->status); >> + } >> + >> + /* For some Intel based controllers, the default Bluetooth device >> + * address 00:03:19:9E:8B:00 can be found. These controllers are >> + * fully operational, but have the danger of duplicate addresses >> + * and that in turn can cause problems with Bluetooth operation. >> + */ >> + if (!bacmp(&rp->bdaddr, BDADDR_INTEL)) { >> + BT_ERR("%s found Intel default device address (%pMR)", >> + hdev->name, &rp->bdaddr); >> + set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks); >> + } >> + >> + kfree_skb(skb); >> + >> + return 0; >> +} >> + >> struct intel_version { >> u8 status; >> u8 hw_platform; >> @@ -1289,13 +1334,61 @@ struct intel_version { >> u8 fw_patch_num; >> } __packed; >>=20 >> -static const struct firmware *btusb_setup_intel_get_fw(struct hci_dev >>*hdev, >> - struct intel_version *ver) >> +static const struct firmware *btusb_setup_intel_get_fw(struct hci_dev >>*hdev) >> { >> const struct firmware *fw; >> + struct sk_buff *skb; >> + struct intel_version *ver; >> char fwname[64]; >> int ret; >>=20 >> + BT_DBG("%s", hdev->name); >> + >> + /* Read Intel specific controller version first to allow selection of >> + * which firmware file to load. >> + * >> + * The returned information are hardware variant and revision plus >> + * firmware variant, revision and build number. >> + */ >> + skb =3D __hci_cmd_sync(hdev, 0xfc05, 0, NULL, HCI_INIT_TIMEOUT); >> + if (IS_ERR(skb)) { >> + BT_ERR("%s reading Intel fw version command failed (%ld)", >> + hdev->name, PTR_ERR(skb)); >> + return NULL; >> + } >> + >> + if (skb->len !=3D sizeof(*ver)) { >> + BT_ERR("%s Intel version event length mismatch", hdev->name); >> + kfree_skb(skb); >> + return NULL; >> + } >> + >> + ver =3D (struct intel_version *)skb->data; >> + if (ver->status) { >> + BT_ERR("%s Intel fw version event failed (%02x)", hdev->name, >> + ver->status); >> + kfree_skb(skb); >> + return NULL; >> + } >> + >> + BT_INFO("%s: read Intel version: >>%02x%02x%02x%02x%02x%02x%02x%02x%02x", >> + hdev->name, ver->hw_platform, ver->hw_variant, >> + ver->hw_revision, ver->fw_variant, ver->fw_revision, >> + ver->fw_build_num, ver->fw_build_ww, ver->fw_build_yy, >> + ver->fw_patch_num); >> + >> + /* fw_patch_num indicates the version of patch the device currently >> + * have. If there is no patch data in the device, it is always 0x00. >> + * So, if it is other than 0x00, no need to patch the device again. >> + */ >> + if (ver->fw_patch_num) { >> + BT_INFO("%s: Intel device is already patched. patch num: %02x", >> + hdev->name, ver->fw_patch_num); >> + btusb_check_bdaddr_intel(hdev); >> + return NULL; >> + } >> + >> + /* Open the firwmare file if it exists */ >> snprintf(fwname, sizeof(fwname), >> "intel/ibt-hw-%x.%x.%x-fw-%x.%x.%x.%x.%x.bseq", >> ver->hw_platform, ver->hw_variant, ver->hw_revision, >> @@ -1445,58 +1538,12 @@ static int btusb_setup_intel_patching(struct >>hci_dev *hdev, >> return 0; >> } >>=20 >> -#define BDADDR_INTEL (&(bdaddr_t) {{0x00, 0x8b, 0x9e, 0x19, 0x03, >>0x00}}) >> - >> -static int btusb_check_bdaddr_intel(struct hci_dev *hdev) >> -{ >> - struct sk_buff *skb; >> - struct hci_rp_read_bd_addr *rp; >> - >> - skb =3D __hci_cmd_sync(hdev, HCI_OP_READ_BD_ADDR, 0, NULL, >> - HCI_INIT_TIMEOUT); >> - if (IS_ERR(skb)) { >> - BT_ERR("%s reading Intel device address failed (%ld)", >> - hdev->name, PTR_ERR(skb)); >> - return PTR_ERR(skb); >> - } >> - >> - if (skb->len !=3D sizeof(*rp)) { >> - BT_ERR("%s Intel device address length mismatch", hdev->name); >> - kfree_skb(skb); >> - return -EIO; >> - } >> - >> - rp =3D (struct hci_rp_read_bd_addr *)skb->data; >> - if (rp->status) { >> - BT_ERR("%s Intel device address result failed (%02x)", >> - hdev->name, rp->status); >> - kfree_skb(skb); >> - return -bt_to_errno(rp->status); >> - } >> - >> - /* For some Intel based controllers, the default Bluetooth device >> - * address 00:03:19:9E:8B:00 can be found. These controllers are >> - * fully operational, but have the danger of duplicate addresses >> - * and that in turn can cause problems with Bluetooth operation. >> - */ >> - if (!bacmp(&rp->bdaddr, BDADDR_INTEL)) { >> - BT_ERR("%s found Intel default device address (%pMR)", >> - hdev->name, &rp->bdaddr); >> - set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks); >> - } >> - >> - kfree_skb(skb); >> - >> - return 0; >> -} >> - >> static int btusb_setup_intel(struct hci_dev *hdev) >> { >> struct sk_buff *skb; >> const struct firmware *fw; >> const u8 *fw_ptr; >> int disable_patch; >> - struct intel_version *ver; >>=20 >> const u8 mfg_enable[] =3D { 0x01, 0x00 }; >> const u8 mfg_disable[] =3D { 0x00, 0x00 }; >> @@ -1521,60 +1568,14 @@ static int btusb_setup_intel(struct hci_dev >>*hdev) >> } >> kfree_skb(skb); >>=20 >> - /* Read Intel specific controller version first to allow selection of >> - * which firmware file to load. >> - * >> - * The returned information are hardware variant and revision plus >> - * firmware variant, revision and build number. >> - */ >> - skb =3D __hci_cmd_sync(hdev, 0xfc05, 0, NULL, HCI_INIT_TIMEOUT); >> - if (IS_ERR(skb)) { >> - BT_ERR("%s reading Intel fw version command failed (%ld)", >> - hdev->name, PTR_ERR(skb)); >> - return PTR_ERR(skb); >> - } >> - >> - if (skb->len !=3D sizeof(*ver)) { >> - BT_ERR("%s Intel version event length mismatch", hdev->name); >> - kfree_skb(skb); >> - return -EIO; >> - } >> - >> - ver =3D (struct intel_version *)skb->data; >> - if (ver->status) { >> - BT_ERR("%s Intel fw version event failed (%02x)", hdev->name, >> - ver->status); >> - kfree_skb(skb); >> - return -bt_to_errno(ver->status); >> - } >> - >> - BT_INFO("%s: read Intel version: >>%02x%02x%02x%02x%02x%02x%02x%02x%02x", >> - hdev->name, ver->hw_platform, ver->hw_variant, >> - ver->hw_revision, ver->fw_variant, ver->fw_revision, >> - ver->fw_build_num, ver->fw_build_ww, ver->fw_build_yy, >> - ver->fw_patch_num); >> - >> - /* fw_patch_num indicates the version of patch the device currently >> - * have. If there is no patch data in the device, it is always 0x00. >> - * So, if it is other than 0x00, no need to patch the deivce again. >> - */ >> - if (ver->fw_patch_num) { >> - BT_INFO("%s: Intel device is already patched. patch num: %02x", >> - hdev->name, ver->fw_patch_num); >> - kfree_skb(skb); >> - btusb_check_bdaddr_intel(hdev); >> - return 0; >> - } >> - >> /* Opens the firmware patch file based on the firmware version read >> * from the controller. If it fails to open the matching firmware >> * patch file, it tries to open the default firmware patch file. >> * If no patch file is found, allow the device to operate without >> * a patch. >> */ >> - fw =3D btusb_setup_intel_get_fw(hdev, ver); >> + fw =3D btusb_setup_intel_get_fw(hdev); >> if (!fw) { >> - kfree_skb(skb); >> btusb_check_bdaddr_intel(hdev); >> return 0; >> } > >moving the btusb_check_bdaddr_intel around is actually not needed. The >one call to it in the error path of btusb_setup_intel_get_fw right here >will do all the needed handling. No need to run it twice. Thanks for your comments. I updated the patch and will send out v3. > >Regards > >Marcel > Regards, Tedd