On Fri, 2012-12-21 at 02:52 +0000, Larry Finger wrote:
[...]
> --- /dev/null
> +++ b/drivers/bluetooth/rtk_btusb.c
[...]
> +#include <linux/version.h>
Not needed.
> +#include <linux/pm_runtime.h>
Move this up to the first group of #includes.
> +#define HDEV_BUS (hdev->bus)
This is just obfuscation.
> +#define USB_RPM 1
> +
> +#define GET_DRV_DATA(x) hci_get_drvdata(x)
> +
> +
> +#define BTUSB_RPM 0 /*1 SS enable; 0 SS disable */
Run-time power management should be enabled if it works and not included
yet if it doesn't. This shouldn't be a compile-time option in a
production driver.
> +#define LOAD_CONFIG 0
Seems to be a rather pointless development option, but if it still has
some value then it deserves a comment.
> +#define URB_CANCELING_DELAY_MS 10 /* Added by Realtek */
/* BWH 2012-12-25: Doesn't Realtek have version control to record this? */
[...]
> +/*******************************
> +** Reasil patch code
> +********************************/
Another weird little bit of history which no-one cares about.
> +#include <linux/firmware.h>
> +#include <linux/suspend.h>
> +#include <net/bluetooth/hci.h>
Belong at the top of the file.
[...]
> +static int rtkbt_pm_notify(struct notifier_block *notifier, ulong pm_event,
> + void *unused)
> +{
> + struct dev_data *dev_entry;
> + struct patch_info *patch_entry;
> + struct usb_device *udev;
> +
> + dev_entry = container_of(notifier, struct dev_data, pm_notifier);
> + patch_entry = dev_entry->patch_entry;
> + udev = dev_entry->udev;
> + RTKBT_DBG("rtkbt_pm_notify pm_event =%ld", pm_event);
> + switch (pm_event) {
> + case PM_SUSPEND_PREPARE:
> + case PM_HIBERNATION_PREPARE:
> + patch_entry->fw_len = load_firmware(dev_entry,
> + &patch_entry->fw_cache);
But this is done once for each device, not for each device type. So you
potentially load the firmware multiple times here and leak all but one.
Anyway I'm not sure this caching is needed any more due to the firmware
management improvements in 3.7.
[...]
> +static int load_firmware(struct dev_data *dev_entry, uint8_t **buff)
> +{
> +#if LOAD_CONFIG
> + const struct firmware *fw;
> +#endif
> + struct usb_device *udev;
> + struct patch_info *patch_entry;
> + char *fw_name;
> + int fw_len = 0, ret_val;
> +
> + udev = dev_entry->udev;
> + init_completion(&dev_entry->firmware_loading_complete);
> + patch_entry = dev_entry->patch_entry;
> + fw_name = patch_entry->patch_name;
> + RTKBT_DBG("Reading firmware file %s", fw_name);
> + ret_val = request_firmware_nowait(THIS_MODULE, 1, fw_name, &udev->dev,
> + GFP_KERNEL, dev_entry, bt_fw_cb);
> + if (ret_val < 0)
> + goto fw_fail;
> +
> + wait_for_completion(&dev_entry->firmware_loading_complete);
What was the point of using request_firmware_nowait() then?
> + if (!dev_entry->fw)
> + goto fw_fail;
> + *buff = kzalloc(dev_entry->fw->size, GFP_KERNEL);
> + if (NULL == *buff)
> + goto alloc_fail;
> + memcpy(*buff, dev_entry->fw->data, dev_entry->fw->size);
> + fw_len = dev_entry->fw->size;
> +
> +#if LOAD_CONFIG
> + release_firmware(dev_entry->fw);
> + fw_name = patch_entry->config_name;
> + ret_val = request_firmware(&fw, fw_name, &udev->dev);
> + if (ret_val < 0) {
> + fw_len = 0;
> + kfree(*buff);
> + *buff = NULL;
> + goto fw_fail;
> + }
> +
> + *buff = krealloc(*buff, fw_len + fw->size, GFP_KERNEL);
> + if (NULL == *buff) {
> + fw_len = 0;
> + release_firmware(fw);
> + goto fw_fail;
> + }
> + memcpy(*buff + fw_len, fw->data, fw->size);
> + fw_len += fw->size;
> +#endif
It's easy to concatenate files in userland; why do it in the driver?
> +alloc_fail:
> + release_firmware(dev_entry->fw);
> +fw_fail:
> + return fw_len;
> +}
[...]
--
Ben Hutchings
Every program is either trivial or else contains at least one bug
Attachment:
signature.asc
Description: This is a digitally signed message part
