[PATCH 1/3] Bluetooth: btrtl: Let rtl_load_file() always return error code

[PATCH 1/3] Bluetooth: btrtl: Let rtl_load_file() always return error code

[Kai-Heng Feng]

The return value of rtl_load_file() can be either firmware length or
error code.

For consistency, always return error code and pass firmware length via
another pointer.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/bluetooth/btrtl.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index 41405de27d66..a134a79bf0ef 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -413,7 +413,8 @@ static int rtl_download_firmware(struct hci_dev *hdev,
 	return ret;
 }
 
-static int rtl_load_file(struct hci_dev *hdev, const char *name, u8 **buff)
+static int rtl_load_file(struct hci_dev *hdev, const char *name,
+			 u8 **buff, int *fw_size)
 {
 	const struct firmware *fw;
 	int ret;
@@ -422,8 +423,8 @@ static int rtl_load_file(struct hci_dev *hdev, const char *name, u8 **buff)
 	ret = request_firmware(&fw, name, &hdev->dev);
 	if (ret < 0)
 		return ret;
-	ret = fw->size;
-	*buff = kmemdup(fw->data, ret, GFP_KERNEL);
+	*fw_size = fw->size;
+	*buff = kmemdup(fw->data, fw->size, GFP_KERNEL);
 	if (!*buff)
 		ret = -ENOMEM;
 
@@ -564,12 +565,11 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev,
 			goto err_free;
 	}
 
-	btrtl_dev->fw_len = rtl_load_file(hdev, btrtl_dev->ic_info->fw_name,
-					  &btrtl_dev->fw_data);
-	if (btrtl_dev->fw_len < 0) {
+	ret = rtl_load_file(hdev, btrtl_dev->ic_info->fw_name,
+			    &btrtl_dev->fw_data, &btrtl_dev->fw_len);
+	if (ret || !btrtl_dev->fw_len) {
 		rtl_dev_err(hdev, "firmware file %s not found\n",
 			    btrtl_dev->ic_info->fw_name);
-		ret = btrtl_dev->fw_len;
 		goto err_free;
 	}
 
@@ -581,13 +581,12 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev,
 			snprintf(cfg_name, sizeof(cfg_name), "%s.bin",
 				 btrtl_dev->ic_info->cfg_name);
 		}
-		btrtl_dev->cfg_len = rtl_load_file(hdev, cfg_name,
-						   &btrtl_dev->cfg_data);
+		ret = rtl_load_file(hdev, cfg_name, &btrtl_dev->cfg_data,
+				    &btrtl_dev->cfg_len);
 		if (btrtl_dev->ic_info->config_needed &&
-		    btrtl_dev->cfg_len <= 0) {
+		    (ret || !btrtl_dev->cfg_len)) {
 			rtl_dev_err(hdev, "mandatory config file %s not found\n",
 				    btrtl_dev->ic_info->cfg_name);
-			ret = btrtl_dev->cfg_len;
 			goto err_free;
 		}
 	}
-- 
2.17.1

[PATCH 2/3] Bluetooth: btrtl: Let btrtl_initialize() always return error code

[Kai-Heng Feng]

btrtl_initialize() may return an address or error code.

To make it more consistent, refactor btrtl_initialize() to always return
error code.

This requires "struct btrtl_device_info" to be allocated on stack. It's
a small structure, and simiar patterns can be found in other bluetooth
vendor helpers.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/bluetooth/btrtl.c  | 43 +++++++++++++-------------------------
 drivers/bluetooth/btrtl.h  |  5 +++--
 drivers/bluetooth/hci_h5.c | 16 ++++++--------
 3 files changed, 23 insertions(+), 41 deletions(-)

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index a134a79bf0ef..c36f500d8313 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -516,10 +516,10 @@ void btrtl_free(struct btrtl_device_info *btrtl_dev)
 }
 EXPORT_SYMBOL_GPL(btrtl_free);
 
-struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev,
-					   const char *postfix)
+int btrtl_initialize(struct hci_dev *hdev,
+		     struct btrtl_device_info *btrtl_dev,
+		     const char *postfix)
 {
-	struct btrtl_device_info *btrtl_dev;
 	struct sk_buff *skb;
 	struct hci_rp_read_local_version *resp;
 	char cfg_name[40];
@@ -527,16 +527,9 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev,
 	u8 hci_ver;
 	int ret;
 
-	btrtl_dev = kzalloc(sizeof(*btrtl_dev), GFP_KERNEL);
-	if (!btrtl_dev) {
-		ret = -ENOMEM;
-		goto err_alloc;
-	}
-
 	skb = btrtl_read_local_version(hdev);
 	if (IS_ERR(skb)) {
-		ret = PTR_ERR(skb);
-		goto err_free;
+		return PTR_ERR(skb);
 	}
 
 	resp = (struct hci_rp_read_local_version *)skb->data;
@@ -555,14 +548,13 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev,
 	if (!btrtl_dev->ic_info) {
 		rtl_dev_err(hdev, "rtl: unknown IC info, lmp subver %04x, hci rev %04x, hci ver %04x",
 			    lmp_subver, hci_rev, hci_ver);
-		ret = -EINVAL;
-		goto err_free;
+		return -EINVAL;
 	}
 
 	if (btrtl_dev->ic_info->has_rom_version) {
 		ret = rtl_read_rom_version(hdev, &btrtl_dev->rom_version);
 		if (ret)
-			goto err_free;
+			return ret;
 	}
 
 	ret = rtl_load_file(hdev, btrtl_dev->ic_info->fw_name,
@@ -570,7 +562,7 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev,
 	if (ret || !btrtl_dev->fw_len) {
 		rtl_dev_err(hdev, "firmware file %s not found\n",
 			    btrtl_dev->ic_info->fw_name);
-		goto err_free;
+		return -ENOENT;
 	}
 
 	if (btrtl_dev->ic_info->cfg_name) {
@@ -587,16 +579,11 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev,
 		    (ret || !btrtl_dev->cfg_len)) {
 			rtl_dev_err(hdev, "mandatory config file %s not found\n",
 				    btrtl_dev->ic_info->cfg_name);
-			goto err_free;
+			return -ENOENT;
 		}
 	}
 
-	return btrtl_dev;
-
-err_free:
-	btrtl_free(btrtl_dev);
-err_alloc:
-	return ERR_PTR(ret);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(btrtl_initialize);
 
@@ -627,16 +614,14 @@ EXPORT_SYMBOL_GPL(btrtl_download_firmware);
 
 int btrtl_setup_realtek(struct hci_dev *hdev)
 {
-	struct btrtl_device_info *btrtl_dev;
+	struct btrtl_device_info btrtl_dev;
 	int ret;
 
-	btrtl_dev = btrtl_initialize(hdev, NULL);
-	if (IS_ERR(btrtl_dev))
-		return PTR_ERR(btrtl_dev);
-
-	ret = btrtl_download_firmware(hdev, btrtl_dev);
+	ret = btrtl_initialize(hdev, &btrtl_dev, NULL);
+	if (ret)
+		return ret;
 
-	btrtl_free(btrtl_dev);
+	ret = btrtl_download_firmware(hdev, &btrtl_dev);
 
 	return ret;
 }
diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h
index f5e36f3993a8..df15480b7077 100644
--- a/drivers/bluetooth/btrtl.h
+++ b/drivers/bluetooth/btrtl.h
@@ -59,8 +59,9 @@ struct rtl_vendor_config {
 
 #if IS_ENABLED(CONFIG_BT_RTL)
 
-struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev,
-					   const char *postfix);
+int btrtl_initialize(struct hci_dev *hdev,
+		     struct btrtl_device_info *btrtl_dev,
+		     const char *postfix);
 void btrtl_free(struct btrtl_device_info *btrtl_dev);
 int btrtl_download_firmware(struct hci_dev *hdev,
 			    struct btrtl_device_info *btrtl_dev);
diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index 069d1c8fde73..d1fb0a4d82ae 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -868,7 +868,7 @@ static int __maybe_unused h5_serdev_resume(struct device *dev)
 #ifdef CONFIG_BT_HCIUART_RTL
 static int h5_btrtl_setup(struct h5 *h5)
 {
-	struct btrtl_device_info *btrtl_dev;
+	struct btrtl_device_info btrtl_dev;
 	struct sk_buff *skb;
 	__le32 baudrate_data;
 	u32 device_baudrate;
@@ -876,23 +876,22 @@ static int h5_btrtl_setup(struct h5 *h5)
 	bool flow_control;
 	int err;
 
-	btrtl_dev = btrtl_initialize(h5->hu->hdev, h5->id);
-	if (IS_ERR(btrtl_dev))
-		return PTR_ERR(btrtl_dev);
+	err = btrtl_initialize(h5->hu->hdev, &btrtl_dev, h5->id);
+	if (err)
+		return err;
 
 	err = btrtl_get_uart_settings(h5->hu->hdev, btrtl_dev,
 				      &controller_baudrate, &device_baudrate,
 				      &flow_control);
 	if (err)
-		goto out_free;
+		return err;
 
 	baudrate_data = cpu_to_le32(device_baudrate);
 	skb = __hci_cmd_sync(h5->hu->hdev, 0xfc17, sizeof(baudrate_data),
 			     &baudrate_data, HCI_INIT_TIMEOUT);
 	if (IS_ERR(skb)) {
 		rtl_dev_err(h5->hu->hdev, "set baud rate command failed\n");
-		err = PTR_ERR(skb);
-		goto out_free;
+		return PTR_ERR(skb);
 	} else {
 		kfree_skb(skb);
 	}
@@ -906,9 +905,6 @@ static int h5_btrtl_setup(struct h5 *h5)
 	/* Give the device some time before the hci-core sends it a reset */
 	usleep_range(10000, 20000);
 
-out_free:
-	btrtl_free(btrtl_dev);
-
 	return err;
 }
 
-- 
2.17.1

[PATCH 3/3] Bluetooth: btrtl: Skip initialization if firmware is already loaded

[Kai-Heng Feng]

Realtek bluetooth may not work after reboot:
[   12.446130] Bluetooth: hci0: RTL: rtl: unknown IC info, lmp subver a99e, hci rev 826c, hci ver 0008

The power is not cut during system reboot, so the firmware is kept in
Bluetooth controller.

Realtek bluetooth doesn't have the ability to check firmware loading
status. but the version queried by HCI_OP_READ_LOCAL_VERSION will be
different if firmware is already loaded. Realtek's own fork, rtk_btusb
also use this method to detect the loading status.

So let's assume the firmware is already loaded when we can't find
matching IC info.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201921
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/bluetooth/btrtl.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index c36f500d8313..e2f89d57dd14 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -546,9 +546,10 @@ int btrtl_initialize(struct hci_dev *hdev,
 					    hdev->bus);
 
 	if (!btrtl_dev->ic_info) {
-		rtl_dev_err(hdev, "rtl: unknown IC info, lmp subver %04x, hci rev %04x, hci ver %04x",
-			    lmp_subver, hci_rev, hci_ver);
-		return -EINVAL;
+		rtl_dev_info(hdev, "rtl: unknown IC info, lmp subver %04x, hci rev %04x, hci ver %04x",
+			     lmp_subver, hci_rev, hci_ver);
+		rtl_dev_info(hdev, "rtl: firmware may be already loaded, or it's an unsupported IC.");
+		return 0;
 	}
 
 	if (btrtl_dev->ic_info->has_rom_version) {
@@ -621,7 +622,8 @@ int btrtl_setup_realtek(struct hci_dev *hdev)
 	if (ret)
 		return ret;
 
-	ret = btrtl_download_firmware(hdev, &btrtl_dev);
+	if (btrtl_dev.ic_info)
+		ret = btrtl_download_firmware(hdev, &btrtl_dev);
 
 	return ret;
 }
-- 
2.17.1

Re: [PATCH 3/3] Bluetooth: btrtl: Skip initialization if firmware is already loaded

[Daniel Drake]

On Thu, Jan 24, 2019 at 11:23 PM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
> Realtek bluetooth may not work after reboot:
> [   12.446130] Bluetooth: hci0: RTL: rtl: unknown IC info, lmp subver a99e, hci rev 826c, hci ver 0008
>
> The power is not cut during system reboot, so the firmware is kept in
> Bluetooth controller.
>
> Realtek bluetooth doesn't have the ability to check firmware loading
> status. but the version queried by HCI_OP_READ_LOCAL_VERSION will be
> different if firmware is already loaded. Realtek's own fork, rtk_btusb
> also use this method to detect the loading status.
>
> So let's assume the firmware is already loaded when we can't find
> matching IC info.

This logic was already present in the driver - but it looks like this
regressed at this point:

commit 26503ad25de8c7c93a2037f919c2e49a62cf65f1
Author: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date:   Thu Aug 2 16:57:13 2018 +0200

    Bluetooth: btrtl: split the device initialization into smaller parts

After your patch it is effectively there in two places now, since it
is also in btrtl_download_firmware() (although not really effective
after the above commit). I wonder if that can be cleaned up to avoid
duplication.

Regarding the other patches that move away from the style of returning
either a useful value or an error, is this purely a stylistic thing or
is it needed for your 3rd patch? I don't have strong feelings either
way but I have the impression that the currently implemented approach
is a common style within kernel code and I don't see benefit in
splitting off a separate out parameter.

Daniel


> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201921
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/bluetooth/btrtl.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
> index c36f500d8313..e2f89d57dd14 100644
> --- a/drivers/bluetooth/btrtl.c
> +++ b/drivers/bluetooth/btrtl.c
> @@ -546,9 +546,10 @@ int btrtl_initialize(struct hci_dev *hdev,
>                                             hdev->bus);
>
>         if (!btrtl_dev->ic_info) {
> -               rtl_dev_err(hdev, "rtl: unknown IC info, lmp subver %04x, hci rev %04x, hci ver %04x",
> -                           lmp_subver, hci_rev, hci_ver);
> -               return -EINVAL;
> +               rtl_dev_info(hdev, "rtl: unknown IC info, lmp subver %04x, hci rev %04x, hci ver %04x",
> +                            lmp_subver, hci_rev, hci_ver);
> +               rtl_dev_info(hdev, "rtl: firmware may be already loaded, or it's an unsupported IC.");
> +               return 0;
>         }
>
>         if (btrtl_dev->ic_info->has_rom_version) {
> @@ -621,7 +622,8 @@ int btrtl_setup_realtek(struct hci_dev *hdev)
>         if (ret)
>                 return ret;
>
> -       ret = btrtl_download_firmware(hdev, &btrtl_dev);
> +       if (btrtl_dev.ic_info)
> +               ret = btrtl_download_firmware(hdev, &btrtl_dev);
>
>         return ret;
>  }
> --
> 2.17.1
>

Re: [PATCH 3/3] Bluetooth: btrtl: Skip initialization if firmware is already loaded

[Kai-Heng Feng]

> On Jan 25, 2019, at 08:55, Daniel Drake <drake@endlessm.com> wrote:
> 
> On Thu, Jan 24, 2019 at 11:23 PM Kai-Heng Feng
> <kai.heng.feng@canonical.com> wrote:
>> Realtek bluetooth may not work after reboot:
>> [   12.446130] Bluetooth: hci0: RTL: rtl: unknown IC info, lmp subver a99e, hci rev 826c, hci ver 0008
>> 
>> The power is not cut during system reboot, so the firmware is kept in
>> Bluetooth controller.
>> 
>> Realtek bluetooth doesn't have the ability to check firmware loading
>> status. but the version queried by HCI_OP_READ_LOCAL_VERSION will be
>> different if firmware is already loaded. Realtek's own fork, rtk_btusb
>> also use this method to detect the loading status.
>> 
>> So let's assume the firmware is already loaded when we can't find
>> matching IC info.
> 
> This logic was already present in the driver - but it looks like this
> regressed at this point:
> 
> commit 26503ad25de8c7c93a2037f919c2e49a62cf65f1
> Author: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Date:   Thu Aug 2 16:57:13 2018 +0200
> 
>    Bluetooth: btrtl: split the device initialization into smaller parts

Thanks, didn’t find out it’s a regression.

> 
> After your patch it is effectively there in two places now, since it
> is also in btrtl_download_firmware() (although not really effective
> after the above commit). I wonder if that can be cleaned up to avoid
> duplication.

Put the additional check to btrtl_download_firmware() should be sufficient.

> 
> Regarding the other patches that move away from the style of returning
> either a useful value or an error, is this purely a stylistic thing or
> is it needed for your 3rd patch? I don't have strong feelings either
> way but I have the impression that the currently implemented approach
> is a common style within kernel code and I don't see benefit in
> splitting off a separate out parameter.

Ok. I’ll send a v2 without the refactoring part.

Kai-Heng

> 
> Daniel
> 
> 
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201921
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> ---
>> drivers/bluetooth/btrtl.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
>> index c36f500d8313..e2f89d57dd14 100644
>> --- a/drivers/bluetooth/btrtl.c
>> +++ b/drivers/bluetooth/btrtl.c
>> @@ -546,9 +546,10 @@ int btrtl_initialize(struct hci_dev *hdev,
>>                                            hdev->bus);
>> 
>>        if (!btrtl_dev->ic_info) {
>> -               rtl_dev_err(hdev, "rtl: unknown IC info, lmp subver %04x, hci rev %04x, hci ver %04x",
>> -                           lmp_subver, hci_rev, hci_ver);
>> -               return -EINVAL;
>> +               rtl_dev_info(hdev, "rtl: unknown IC info, lmp subver %04x, hci rev %04x, hci ver %04x",
>> +                            lmp_subver, hci_rev, hci_ver);
>> +               rtl_dev_info(hdev, "rtl: firmware may be already loaded, or it's an unsupported IC.");
>> +               return 0;
>>        }
>> 
>>        if (btrtl_dev->ic_info->has_rom_version) {
>> @@ -621,7 +622,8 @@ int btrtl_setup_realtek(struct hci_dev *hdev)
>>        if (ret)
>>                return ret;
>> 
>> -       ret = btrtl_download_firmware(hdev, &btrtl_dev);
>> +       if (btrtl_dev.ic_info)
>> +               ret = btrtl_download_firmware(hdev, &btrtl_dev);
>> 
>>        return ret;
>> }
>> --
>> 2.17.1
>>