* [PATCH 1/3] Bluetooth: btrtl: Let rtl_load_file() always return error code @ 2019-01-24 15:23 Kai-Heng Feng 2019-01-24 15:23 ` [PATCH 2/3] Bluetooth: btrtl: Let btrtl_initialize() " Kai-Heng Feng 2019-01-24 15:23 ` [PATCH 3/3] Bluetooth: btrtl: Skip initialization if firmware is already loaded Kai-Heng Feng 0 siblings, 2 replies; 5+ messages in thread From: Kai-Heng Feng @ 2019-01-24 15:23 UTC (permalink / raw) To: marcel, johan.hedberg; +Cc: drake, linux-bluetooth, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/3] Bluetooth: btrtl: Let btrtl_initialize() always return error code 2019-01-24 15:23 [PATCH 1/3] Bluetooth: btrtl: Let rtl_load_file() always return error code Kai-Heng Feng @ 2019-01-24 15:23 ` Kai-Heng Feng 2019-01-24 15:23 ` [PATCH 3/3] Bluetooth: btrtl: Skip initialization if firmware is already loaded Kai-Heng Feng 1 sibling, 0 replies; 5+ messages in thread From: Kai-Heng Feng @ 2019-01-24 15:23 UTC (permalink / raw) To: marcel, johan.hedberg; +Cc: drake, linux-bluetooth, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/3] Bluetooth: btrtl: Skip initialization if firmware is already loaded 2019-01-24 15:23 [PATCH 1/3] Bluetooth: btrtl: Let rtl_load_file() always return error code Kai-Heng Feng 2019-01-24 15:23 ` [PATCH 2/3] Bluetooth: btrtl: Let btrtl_initialize() " Kai-Heng Feng @ 2019-01-24 15:23 ` Kai-Heng Feng 2019-01-25 0:55 ` Daniel Drake 1 sibling, 1 reply; 5+ messages in thread From: Kai-Heng Feng @ 2019-01-24 15:23 UTC (permalink / raw) To: marcel, johan.hedberg; +Cc: drake, linux-bluetooth, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] Bluetooth: btrtl: Skip initialization if firmware is already loaded 2019-01-24 15:23 ` [PATCH 3/3] Bluetooth: btrtl: Skip initialization if firmware is already loaded Kai-Heng Feng @ 2019-01-25 0:55 ` Daniel Drake 2019-01-25 17:46 ` Kai-Heng Feng 0 siblings, 1 reply; 5+ messages in thread From: Daniel Drake @ 2019-01-25 0:55 UTC (permalink / raw) To: Kai-Heng Feng Cc: Marcel Holtmann, Johan Hedberg, Linux Bluetooth mailing list, Linux Kernel, Martin Blumenstingl 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] Bluetooth: btrtl: Skip initialization if firmware is already loaded 2019-01-25 0:55 ` Daniel Drake @ 2019-01-25 17:46 ` Kai-Heng Feng 0 siblings, 0 replies; 5+ messages in thread From: Kai-Heng Feng @ 2019-01-25 17:46 UTC (permalink / raw) To: Daniel Drake Cc: Marcel Holtmann, Johan Hedberg, Linux Bluetooth mailing list, Linux Kernel, Martin Blumenstingl > 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 >> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-01-25 17:46 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-24 15:23 [PATCH 1/3] Bluetooth: btrtl: Let rtl_load_file() always return error code Kai-Heng Feng 2019-01-24 15:23 ` [PATCH 2/3] Bluetooth: btrtl: Let btrtl_initialize() " Kai-Heng Feng 2019-01-24 15:23 ` [PATCH 3/3] Bluetooth: btrtl: Skip initialization if firmware is already loaded Kai-Heng Feng 2019-01-25 0:55 ` Daniel Drake 2019-01-25 17:46 ` Kai-Heng Feng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).