Return-path: Received: from mx1.redhat.com ([209.132.183.28]:46544 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753075Ab2DPSOu (ORCPT ); Mon, 16 Apr 2012 14:14:50 -0400 Message-ID: <1334597520.10194.37.camel@dcbw.foobar.com> (sfid-20120416_201454_247277_1C81CBB5) Subject: Re: [RFC 1/4] libertas: Firmware loading simplifications From: Dan Williams To: Daniel Drake Cc: linux-wireless@vger.kernel.org, libertas-dev@lists.infradead.org Date: Mon, 16 Apr 2012 12:32:00 -0500 In-Reply-To: <20120403160104.D765B9D401E@zog.reactivated.net> References: <20120403160104.D765B9D401E@zog.reactivated.net> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Sender: linux-wireless-owner@vger.kernel.org List-ID: On Tue, 2012-04-03 at 17:01 +0100, Daniel Drake wrote: > Remove the ability to pass module parameters with firmware filenames > for USB and SDIO interfaces. > > Remove the ability to pass custom "user" filenames to lbs_get_firmware(). > > Remove the ability to reprogram internal device memory with a different > firmware from the USB driver (we don't know of any users), and simplify I think the reprogram functionality was intended for the school server use-case. I was advocating for a standalone firmware flashing tool for this anyway (and had written a Python-based one back in 2007) but if OLPC doesn't support the school server stuff anymore, we can safely remove the flashing capability. Dan > the OLPC firmware loading quirk to simply placing the OLPC firmware > at the top of the list (we don't know of any users other than OLPC). > > Move lbs_get_firmware() into its own file. > > These simplifications should have no real-life effect but make the > upcoming transition to asynchronous firmware loading considerably less > painful. > > Signed-off-by: Daniel Drake > --- > drivers/net/wireless/libertas/Makefile | 1 + > drivers/net/wireless/libertas/decl.h | 3 +- > drivers/net/wireless/libertas/firmware.c | 83 +++++++++++++++ > drivers/net/wireless/libertas/if_cs.c | 4 +- > drivers/net/wireless/libertas/if_sdio.c | 25 +---- > drivers/net/wireless/libertas/if_spi.c | 5 +- > drivers/net/wireless/libertas/if_usb.c | 169 ++---------------------------- > drivers/net/wireless/libertas/main.c | 105 ------------------ > 8 files changed, 98 insertions(+), 297 deletions(-) > create mode 100644 drivers/net/wireless/libertas/firmware.c > > Just posting these patches for review for now. Dan, could you please > look at these patches quickly before I send to John? > > Only if_sdio is converted to async fw loading for now, USB will follow > next week and I'll post some untested patches for the other interfaces > too. > > diff --git a/drivers/net/wireless/libertas/Makefile b/drivers/net/wireless/libertas/Makefile > index f7d01bf..eac72f7 100644 > --- a/drivers/net/wireless/libertas/Makefile > +++ b/drivers/net/wireless/libertas/Makefile > @@ -6,6 +6,7 @@ libertas-y += ethtool.o > libertas-y += main.o > libertas-y += rx.o > libertas-y += tx.o > +libertas-y += firmware.o > libertas-$(CONFIG_LIBERTAS_MESH) += mesh.o > > usb8xxx-objs += if_usb.o > diff --git a/drivers/net/wireless/libertas/decl.h b/drivers/net/wireless/libertas/decl.h > index bc951ab..2fb2e31 100644 > --- a/drivers/net/wireless/libertas/decl.h > +++ b/drivers/net/wireless/libertas/decl.h > @@ -66,8 +66,7 @@ int lbs_exit_auto_deep_sleep(struct lbs_private *priv); > u32 lbs_fw_index_to_data_rate(u8 index); > u8 lbs_data_rate_to_fw_index(u32 rate); > > -int lbs_get_firmware(struct device *dev, const char *user_helper, > - const char *user_mainfw, u32 card_model, > +int lbs_get_firmware(struct device *dev, u32 card_model, > const struct lbs_fw_table *fw_table, > const struct firmware **helper, > const struct firmware **mainfw); > diff --git a/drivers/net/wireless/libertas/firmware.c b/drivers/net/wireless/libertas/firmware.c > new file mode 100644 > index 0000000..7ec0063 > --- /dev/null > +++ b/drivers/net/wireless/libertas/firmware.c > @@ -0,0 +1,83 @@ > +/* > + * Firmware loading and handling functions. > + */ > + > +#include > +#include > + > +#include "decl.h" > + > +/** > + * lbs_get_firmware - Retrieves two-stage firmware > + * > + * @dev: A pointer to &device structure > + * @card_model: Bus-specific card model ID used to filter firmware table > + * elements > + * @fw_table: Table of firmware file names and device model numbers > + * terminated by an entry with a NULL helper name > + * @helper: On success, the helper firmware; caller must free > + * @mainfw: On success, the main firmware; caller must free > + * > + * returns: 0 on success, non-zero on failure > + */ > +int lbs_get_firmware(struct device *dev, u32 card_model, > + const struct lbs_fw_table *fw_table, > + const struct firmware **helper, > + const struct firmware **mainfw) > +{ > + const struct lbs_fw_table *iter; > + int ret; > + > + BUG_ON(helper == NULL); > + BUG_ON(mainfw == NULL); > + > + /* Search for firmware to use from the table. */ > + iter = fw_table; > + while (iter && iter->helper) { > + if (iter->model != card_model) > + goto next; > + > + if (*helper == NULL) { > + ret = request_firmware(helper, iter->helper, dev); > + if (ret) > + goto next; > + > + /* If the device has one-stage firmware (ie cf8305) and > + * we've got it then we don't need to bother with the > + * main firmware. > + */ > + if (iter->fwname == NULL) > + return 0; > + } > + > + if (*mainfw == NULL) { > + ret = request_firmware(mainfw, iter->fwname, dev); > + if (ret) { > + /* Clear the helper to ensure we don't have > + * mismatched firmware pairs. > + */ > + release_firmware(*helper); > + *helper = NULL; > + } > + } > + > + if (*helper && *mainfw) > + return 0; > + > + next: > + iter++; > + } > + > + /* Failed */ > + if (*helper) { > + release_firmware(*helper); > + *helper = NULL; > + } > + if (*mainfw) { > + release_firmware(*mainfw); > + *mainfw = NULL; > + } > + > + return -ENOENT; > +} > +EXPORT_SYMBOL_GPL(lbs_get_firmware); > diff --git a/drivers/net/wireless/libertas/if_cs.c b/drivers/net/wireless/libertas/if_cs.c > index 234ee88..bb3deb3 100644 > --- a/drivers/net/wireless/libertas/if_cs.c > +++ b/drivers/net/wireless/libertas/if_cs.c > @@ -890,8 +890,8 @@ static int if_cs_probe(struct pcmcia_device *p_dev) > goto out2; > } > > - ret = lbs_get_firmware(&p_dev->dev, NULL, NULL, card->model, > - &fw_table[0], &helper, &mainfw); > + ret = lbs_get_firmware(&p_dev->dev, card->model, &fw_table[0], > + &helper, &mainfw); > if (ret) { > pr_err("failed to find firmware (%d)\n", ret); > goto out2; > diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c > index 9804ebc..26c7235 100644 > --- a/drivers/net/wireless/libertas/if_sdio.c > +++ b/drivers/net/wireless/libertas/if_sdio.c > @@ -65,12 +65,6 @@ static void if_sdio_interrupt(struct sdio_func *func); > */ > static u8 user_rmmod; > > -static char *lbs_helper_name = NULL; > -module_param_named(helper_name, lbs_helper_name, charp, 0644); > - > -static char *lbs_fw_name = NULL; > -module_param_named(fw_name, lbs_fw_name, charp, 0644); > - > static const struct sdio_device_id if_sdio_ids[] = { > { SDIO_DEVICE(SDIO_VENDOR_ID_MARVELL, > SDIO_DEVICE_ID_MARVELL_LIBERTAS) }, > @@ -124,11 +118,6 @@ struct if_sdio_card { > unsigned long ioport; > unsigned int scratch_reg; > > - const char *helper; > - const char *firmware; > - bool helper_allocated; > - bool firmware_allocated; > - > u8 buffer[65536] __attribute__((aligned(4))); > > spinlock_t lock; > @@ -725,8 +714,8 @@ static int if_sdio_prog_firmware(struct if_sdio_card *card) > goto success; > } > > - ret = lbs_get_firmware(&card->func->dev, lbs_helper_name, lbs_fw_name, > - card->model, &fw_table[0], &helper, &mainfw); > + ret = lbs_get_firmware(&card->func->dev, card->model, &fw_table[0], > + &helper, &mainfw); > if (ret) { > pr_err("failed to find firmware (%d)\n", ret); > goto out; > @@ -1244,10 +1233,6 @@ free: > kfree(packet); > } > > - if (card->helper_allocated) > - kfree(card->helper); > - if (card->firmware_allocated) > - kfree(card->firmware); > kfree(card); > > goto out; > @@ -1295,12 +1280,6 @@ static void if_sdio_remove(struct sdio_func *func) > kfree(packet); > } > > - if (card->helper_allocated) > - kfree(card->helper); > - if (card->firmware_allocated) > - kfree(card->firmware); > - kfree(card); > - > lbs_deb_leave(LBS_DEB_SDIO); > } > > diff --git a/drivers/net/wireless/libertas/if_spi.c b/drivers/net/wireless/libertas/if_spi.c > index 50b1ee7..3a05b58 100644 > --- a/drivers/net/wireless/libertas/if_spi.c > +++ b/drivers/net/wireless/libertas/if_spi.c > @@ -1064,9 +1064,8 @@ static int if_spi_init_card(struct if_spi_card *card) > goto out; > } > > - err = lbs_get_firmware(&card->spi->dev, NULL, NULL, > - card->card_id, &fw_table[0], &helper, > - &mainfw); > + err = lbs_get_firmware(&card->spi->dev, card->card_id, > + &fw_table[0], &helper, &mainfw); > if (err) { > netdev_err(priv->dev, "failed to find firmware (%d)\n", > err); > diff --git a/drivers/net/wireless/libertas/if_usb.c b/drivers/net/wireless/libertas/if_usb.c > index 74da5f1..3dde3e0 100644 > --- a/drivers/net/wireless/libertas/if_usb.c > +++ b/drivers/net/wireless/libertas/if_usb.c > @@ -29,9 +29,6 @@ > > #define MESSAGE_HEADER_LEN 4 > > -static char *lbs_fw_name = NULL; > -module_param_named(fw_name, lbs_fw_name, charp, 0644); > - > MODULE_FIRMWARE("libertas/usb8388_v9.bin"); > MODULE_FIRMWARE("libertas/usb8388_v5.bin"); > MODULE_FIRMWARE("libertas/usb8388.bin"); > @@ -55,10 +52,7 @@ MODULE_DEVICE_TABLE(usb, if_usb_table); > > static void if_usb_receive(struct urb *urb); > static void if_usb_receive_fwload(struct urb *urb); > -static int __if_usb_prog_firmware(struct if_usb_card *cardp, > - const char *fwname, int cmd); > -static int if_usb_prog_firmware(struct if_usb_card *cardp, > - const char *fwname, int cmd); > +static int if_usb_prog_firmware(struct if_usb_card *cardp); > static int if_usb_host_to_card(struct lbs_private *priv, uint8_t type, > uint8_t *payload, uint16_t nb); > static int usb_tx_block(struct if_usb_card *cardp, uint8_t *payload, > @@ -67,69 +61,6 @@ static void if_usb_free(struct if_usb_card *cardp); > static int if_usb_submit_rx_urb(struct if_usb_card *cardp); > static int if_usb_reset_device(struct if_usb_card *cardp); > > -/* sysfs hooks */ > - > -/* > - * Set function to write firmware to device's persistent memory > - */ > -static ssize_t if_usb_firmware_set(struct device *dev, > - struct device_attribute *attr, const char *buf, size_t count) > -{ > - struct lbs_private *priv = to_net_dev(dev)->ml_priv; > - struct if_usb_card *cardp = priv->card; > - int ret; > - > - BUG_ON(buf == NULL); > - > - ret = if_usb_prog_firmware(cardp, buf, BOOT_CMD_UPDATE_FW); > - if (ret == 0) > - return count; > - > - return ret; > -} > - > -/* > - * lbs_flash_fw attribute to be exported per ethX interface through sysfs > - * (/sys/class/net/ethX/lbs_flash_fw). Use this like so to write firmware to > - * the device's persistent memory: > - * echo usb8388-5.126.0.p5.bin > /sys/class/net/ethX/lbs_flash_fw > - */ > -static DEVICE_ATTR(lbs_flash_fw, 0200, NULL, if_usb_firmware_set); > - > -/** > - * if_usb_boot2_set - write firmware to device's persistent memory > - * > - * @dev: target device > - * @attr: device attributes > - * @buf: firmware buffer to write > - * @count: number of bytes to write > - * > - * returns: number of bytes written or negative error code > - */ > -static ssize_t if_usb_boot2_set(struct device *dev, > - struct device_attribute *attr, const char *buf, size_t count) > -{ > - struct lbs_private *priv = to_net_dev(dev)->ml_priv; > - struct if_usb_card *cardp = priv->card; > - int ret; > - > - BUG_ON(buf == NULL); > - > - ret = if_usb_prog_firmware(cardp, buf, BOOT_CMD_UPDATE_BOOT2); > - if (ret == 0) > - return count; > - > - return ret; > -} > - > -/* > - * lbs_flash_boot2 attribute to be exported per ethX interface through sysfs > - * (/sys/class/net/ethX/lbs_flash_boot2). Use this like so to write firmware > - * to the device's persistent memory: > - * echo usb8388-5.126.0.p5.bin > /sys/class/net/ethX/lbs_flash_boot2 > - */ > -static DEVICE_ATTR(lbs_flash_boot2, 0200, NULL, if_usb_boot2_set); > - > /** > * if_usb_write_bulk_callback - callback function to handle the status > * of the URB > @@ -314,13 +245,10 @@ static int if_usb_probe(struct usb_interface *intf, > } > > /* Upload firmware */ > - kparam_block_sysfs_write(fw_name); > - if (__if_usb_prog_firmware(cardp, lbs_fw_name, BOOT_CMD_FW_BY_USB)) { > - kparam_unblock_sysfs_write(fw_name); > + if (if_usb_prog_firmware(cardp)) { > lbs_deb_usbd(&udev->dev, "FW upload failed\n"); > goto err_prog_firmware; > } > - kparam_unblock_sysfs_write(fw_name); > > if (!(priv = lbs_add_card(cardp, &intf->dev))) > goto err_prog_firmware; > @@ -349,14 +277,6 @@ static int if_usb_probe(struct usb_interface *intf, > usb_get_dev(udev); > usb_set_intfdata(intf, cardp); > > - if (device_create_file(&priv->dev->dev, &dev_attr_lbs_flash_fw)) > - netdev_err(priv->dev, > - "cannot register lbs_flash_fw attribute\n"); > - > - if (device_create_file(&priv->dev->dev, &dev_attr_lbs_flash_boot2)) > - netdev_err(priv->dev, > - "cannot register lbs_flash_boot2 attribute\n"); > - > /* > * EHS_REMOVE_WAKEUP is not supported on all versions of the firmware. > */ > @@ -389,9 +309,6 @@ static void if_usb_disconnect(struct usb_interface *intf) > > lbs_deb_enter(LBS_DEB_MAIN); > > - device_remove_file(&priv->dev->dev, &dev_attr_lbs_flash_boot2); > - device_remove_file(&priv->dev->dev, &dev_attr_lbs_flash_fw); > - > cardp->surprise_removed = 1; > > if (priv) { > @@ -912,58 +829,12 @@ static int check_fwfile_format(const uint8_t *data, uint32_t totlen) > return ret; > } > > - > -/** > -* if_usb_prog_firmware - programs the firmware subject to cmd > -* > -* @cardp: the if_usb_card descriptor > -* @fwname: firmware or boot2 image file name > -* @cmd: either BOOT_CMD_FW_BY_USB, BOOT_CMD_UPDATE_FW, > -* or BOOT_CMD_UPDATE_BOOT2. > -* returns: 0 or error code > -*/ > -static int if_usb_prog_firmware(struct if_usb_card *cardp, > - const char *fwname, int cmd) > -{ > - struct lbs_private *priv = cardp->priv; > - unsigned long flags, caps; > - int ret; > - > - caps = priv->fwcapinfo; > - if (((cmd == BOOT_CMD_UPDATE_FW) && !(caps & FW_CAPINFO_FIRMWARE_UPGRADE)) || > - ((cmd == BOOT_CMD_UPDATE_BOOT2) && !(caps & FW_CAPINFO_BOOT2_UPGRADE))) > - return -EOPNOTSUPP; > - > - /* Ensure main thread is idle. */ > - spin_lock_irqsave(&priv->driver_lock, flags); > - while (priv->cur_cmd != NULL || priv->dnld_sent != DNLD_RES_RECEIVED) { > - spin_unlock_irqrestore(&priv->driver_lock, flags); > - if (wait_event_interruptible(priv->waitq, > - (priv->cur_cmd == NULL && > - priv->dnld_sent == DNLD_RES_RECEIVED))) { > - return -ERESTARTSYS; > - } > - spin_lock_irqsave(&priv->driver_lock, flags); > - } > - priv->dnld_sent = DNLD_BOOTCMD_SENT; > - spin_unlock_irqrestore(&priv->driver_lock, flags); > - > - ret = __if_usb_prog_firmware(cardp, fwname, cmd); > - > - spin_lock_irqsave(&priv->driver_lock, flags); > - priv->dnld_sent = DNLD_RES_RECEIVED; > - spin_unlock_irqrestore(&priv->driver_lock, flags); > - > - wake_up(&priv->waitq); > - > - return ret; > -} > - > /* table of firmware file names */ > static const struct { > u32 model; > const char *fwname; > } fw_table[] = { > + { MODEL_8388, "libertas/usb8388_olpc.bin" }, > { MODEL_8388, "libertas/usb8388_v9.bin" }, > { MODEL_8388, "libertas/usb8388_v5.bin" }, > { MODEL_8388, "libertas/usb8388.bin" }, > @@ -971,35 +842,10 @@ static const struct { > { MODEL_8682, "libertas/usb8682.bin" } > }; > > -#ifdef CONFIG_OLPC > - > -static int try_olpc_fw(struct if_usb_card *cardp) > -{ > - int retval = -ENOENT; > - > - /* try the OLPC firmware first; fall back to fw_table list */ > - if (machine_is_olpc() && cardp->model == MODEL_8388) > - retval = request_firmware(&cardp->fw, > - "libertas/usb8388_olpc.bin", &cardp->udev->dev); > - return retval; > -} > - > -#else > -static int try_olpc_fw(struct if_usb_card *cardp) { return -ENOENT; } > -#endif /* !CONFIG_OLPC */ > - > -static int get_fw(struct if_usb_card *cardp, const char *fwname) > +static int get_fw(struct if_usb_card *cardp) > { > int i; > > - /* Try user-specified firmware first */ > - if (fwname) > - return request_firmware(&cardp->fw, fwname, &cardp->udev->dev); > - > - /* Handle OLPC firmware */ > - if (try_olpc_fw(cardp) == 0) > - return 0; > - > /* Otherwise search for firmware to use */ > for (i = 0; i < ARRAY_SIZE(fw_table); i++) { > if (fw_table[i].model != cardp->model) > @@ -1012,8 +858,7 @@ static int get_fw(struct if_usb_card *cardp, const char *fwname) > return -ENOENT; > } > > -static int __if_usb_prog_firmware(struct if_usb_card *cardp, > - const char *fwname, int cmd) > +static int if_usb_prog_firmware(struct if_usb_card *cardp) > { > int i = 0; > static int reset_count = 10; > @@ -1021,7 +866,7 @@ static int __if_usb_prog_firmware(struct if_usb_card *cardp, > > lbs_deb_enter(LBS_DEB_USB); > > - ret = get_fw(cardp, fwname); > + ret = get_fw(cardp); > if (ret) { > pr_err("failed to find firmware (%d)\n", ret); > goto done; > @@ -1053,7 +898,7 @@ restart: > do { > int j = 0; > i++; > - if_usb_issue_boot_command(cardp, cmd); > + if_usb_issue_boot_command(cardp, BOOT_CMD_FW_BY_USB); > /* wait for command response */ > do { > j++; > diff --git a/drivers/net/wireless/libertas/main.c b/drivers/net/wireless/libertas/main.c > index 957681d..fa09585 100644 > --- a/drivers/net/wireless/libertas/main.c > +++ b/drivers/net/wireless/libertas/main.c > @@ -1177,111 +1177,6 @@ void lbs_notify_command_response(struct lbs_private *priv, u8 resp_idx) > } > EXPORT_SYMBOL_GPL(lbs_notify_command_response); > > -/** > - * lbs_get_firmware - Retrieves two-stage firmware > - * > - * @dev: A pointer to &device structure > - * @user_helper: User-defined helper firmware file > - * @user_mainfw: User-defined main firmware file > - * @card_model: Bus-specific card model ID used to filter firmware table > - * elements > - * @fw_table: Table of firmware file names and device model numbers > - * terminated by an entry with a NULL helper name > - * @helper: On success, the helper firmware; caller must free > - * @mainfw: On success, the main firmware; caller must free > - * > - * returns: 0 on success, non-zero on failure > - */ > -int lbs_get_firmware(struct device *dev, const char *user_helper, > - const char *user_mainfw, u32 card_model, > - const struct lbs_fw_table *fw_table, > - const struct firmware **helper, > - const struct firmware **mainfw) > -{ > - const struct lbs_fw_table *iter; > - int ret; > - > - BUG_ON(helper == NULL); > - BUG_ON(mainfw == NULL); > - > - /* Try user-specified firmware first */ > - if (user_helper) { > - ret = request_firmware(helper, user_helper, dev); > - if (ret) { > - dev_err(dev, "couldn't find helper firmware %s\n", > - user_helper); > - goto fail; > - } > - } > - if (user_mainfw) { > - ret = request_firmware(mainfw, user_mainfw, dev); > - if (ret) { > - dev_err(dev, "couldn't find main firmware %s\n", > - user_mainfw); > - goto fail; > - } > - } > - > - if (*helper && *mainfw) > - return 0; > - > - /* Otherwise search for firmware to use. If neither the helper or > - * the main firmware were specified by the user, then we need to > - * make sure that found helper & main are from the same entry in > - * fw_table. > - */ > - iter = fw_table; > - while (iter && iter->helper) { > - if (iter->model != card_model) > - goto next; > - > - if (*helper == NULL) { > - ret = request_firmware(helper, iter->helper, dev); > - if (ret) > - goto next; > - > - /* If the device has one-stage firmware (ie cf8305) and > - * we've got it then we don't need to bother with the > - * main firmware. > - */ > - if (iter->fwname == NULL) > - return 0; > - } > - > - if (*mainfw == NULL) { > - ret = request_firmware(mainfw, iter->fwname, dev); > - if (ret && !user_helper) { > - /* Clear the helper if it wasn't user-specified > - * and the main firmware load failed, to ensure > - * we don't have mismatched firmware pairs. > - */ > - release_firmware(*helper); > - *helper = NULL; > - } > - } > - > - if (*helper && *mainfw) > - return 0; > - > - next: > - iter++; > - } > - > - fail: > - /* Failed */ > - if (*helper) { > - release_firmware(*helper); > - *helper = NULL; > - } > - if (*mainfw) { > - release_firmware(*mainfw); > - *mainfw = NULL; > - } > - > - return -ENOENT; > -} > -EXPORT_SYMBOL_GPL(lbs_get_firmware); > - > static int __init lbs_init_module(void) > { > lbs_deb_enter(LBS_DEB_MAIN);