[PATCH 1/3] libertas_spi: Use workqueue in hw_host_to_card
Marek Vasut
marek.vasut at gmail.com
Fri Jan 21 16:22:37 EST 2011
On Friday 21 January 2011 21:44:48 Vasily Khoruzhick wrote:
> Use workqueue to perform SPI xfers, it's necessary to fix
> nasty "BUG: scheduling while atomic", because
> spu_write() calls spi_sync() and spi_sync() may sleep, but
> hw_host_to_card() callback can be called from atomic context.
> Remove kthread completely, workqueue now does its job.
> Restore intermediate buffers which were removed in commit
> 86c34fe89e9cad9e1ba4d1a8bbf98259035f4caf that introduced
> mentioned bug.
I have two questions:
1) Why not leave kthread there? ie. why switch to workqueue
2) This should be split into two patches I guess -- a) revert the change b)
convert to workqueue -- so they can be (N)ACKed separatedly
Cheers
>
> Signed-off-by: Vasily Khoruzhick <anarsoul at gmail.com>
> ---
> drivers/net/wireless/libertas/if_spi.c | 368
> +++++++++++++++++++++----------- 1 files changed, 239 insertions(+), 129
> deletions(-)
>
> diff --git a/drivers/net/wireless/libertas/if_spi.c
> b/drivers/net/wireless/libertas/if_spi.c index 0060023..f6c2cd6 100644
> --- a/drivers/net/wireless/libertas/if_spi.c
> +++ b/drivers/net/wireless/libertas/if_spi.c
> @@ -20,10 +20,8 @@
> #include <linux/moduleparam.h>
> #include <linux/firmware.h>
> #include <linux/jiffies.h>
> -#include <linux/kthread.h>
> #include <linux/list.h>
> #include <linux/netdevice.h>
> -#include <linux/semaphore.h>
> #include <linux/slab.h>
> #include <linux/spi/libertas_spi.h>
> #include <linux/spi/spi.h>
> @@ -34,6 +32,12 @@
> #include "dev.h"
> #include "if_spi.h"
>
> +struct if_spi_packet {
> + struct list_head list;
> + u16 blen;
> + u8 buffer[0] __attribute__((aligned(4)));
> +};
> +
> struct if_spi_card {
> struct spi_device *spi;
> struct lbs_private *priv;
> @@ -51,18 +55,36 @@ struct if_spi_card {
> unsigned long spu_reg_delay;
>
> /* Handles all SPI communication (except for FW load) */
> - struct task_struct *spi_thread;
> - int run_thread;
> -
> - /* Used to wake up the spi_thread */
> - struct semaphore spi_ready;
> - struct semaphore spi_thread_terminated;
> + struct workqueue_struct *workqueue;
> + struct work_struct packet_work;
>
> u8 cmd_buffer[IF_SPI_CMD_BUF_SIZE];
> +
> + /* A buffer of incoming packets from libertas core.
> + * Since we can't sleep in hw_host_to_card, we have to buffer
> + * them. */
> + struct list_head cmd_packet_list;
> + struct list_head data_packet_list;
> +
> + /* Protects cmd_packet_list and data_packet_list */
> + spinlock_t buffer_lock;
> };
>
> static void free_if_spi_card(struct if_spi_card *card)
> {
> + struct list_head *cursor, *next;
> + struct if_spi_packet *packet;
> +
> + list_for_each_safe(cursor, next, &card->cmd_packet_list) {
> + packet = container_of(cursor, struct if_spi_packet, list);
> + list_del(&packet->list);
> + kfree(packet);
> + }
> + list_for_each_safe(cursor, next, &card->data_packet_list) {
> + packet = container_of(cursor, struct if_spi_packet, list);
> + list_del(&packet->list);
> + kfree(packet);
> + }
> spi_set_drvdata(card->spi, NULL);
> kfree(card);
> }
> @@ -622,7 +644,7 @@ out:
> /*
> * SPI Transfer Thread
> *
> - * The SPI thread handles all SPI transfers, so there is no need for a
> lock. + * The SPI worker handles all SPI transfers, so there is no need
> for a lock. */
>
> /* Move a command from the card to the host */
> @@ -742,6 +764,40 @@ out:
> return err;
> }
>
> +/* Move data or a command from the host to the card. */
> +static void if_spi_h2c(struct if_spi_card *card,
> + struct if_spi_packet *packet, int type)
> +{
> + int err = 0;
> + u16 int_type, port_reg;
> +
> + switch (type) {
> + case MVMS_DAT:
> + int_type = IF_SPI_CIC_TX_DOWNLOAD_OVER;
> + port_reg = IF_SPI_DATA_RDWRPORT_REG;
> + break;
> + case MVMS_CMD:
> + int_type = IF_SPI_CIC_CMD_DOWNLOAD_OVER;
> + port_reg = IF_SPI_CMD_RDWRPORT_REG;
> + break;
> + default:
> + lbs_pr_err("can't transfer buffer of type %d\n", type);
> + err = -EINVAL;
> + goto out;
> + }
> +
> + /* Write the data to the card */
> + err = spu_write(card, port_reg, packet->buffer, packet->blen);
> + if (err)
> + goto out;
> +
> +out:
> + kfree(packet);
> +
> + if (err)
> + lbs_pr_err("%s: error %d\n", __func__, err);
> +}
> +
> /* Inform the host about a card event */
> static void if_spi_e2h(struct if_spi_card *card)
> {
> @@ -766,71 +822,88 @@ out:
> lbs_pr_err("%s: error %d\n", __func__, err);
> }
>
> -static int lbs_spi_thread(void *data)
> +static void if_spi_host_to_card_worker(struct work_struct *work)
> {
> int err;
> - struct if_spi_card *card = data;
> + struct if_spi_card *card;
> u16 hiStatus;
> + unsigned long flags;
> + struct if_spi_packet *packet;
>
> - while (1) {
> - /* Wait to be woken up by one of two things. First, our ISR
> - * could tell us that something happened on the WLAN.
> - * Secondly, libertas could call hw_host_to_card with more
> - * data, which we might be able to send.
> - */
> - do {
> - err = down_interruptible(&card->spi_ready);
> - if (!card->run_thread) {
> - up(&card->spi_thread_terminated);
> - do_exit(0);
> - }
> - } while (err == -EINTR);
> + card = container_of(work, struct if_spi_card, packet_work);
>
> - /* Read the host interrupt status register to see what we
> - * can do. */
> - err = spu_read_u16(card, IF_SPI_HOST_INT_STATUS_REG,
> - &hiStatus);
> - if (err) {
> - lbs_pr_err("I/O error\n");
> + lbs_deb_enter(LBS_DEB_SPI);
> +
> + /* Read the host interrupt status register to see what we
> + * can do. */
> + err = spu_read_u16(card, IF_SPI_HOST_INT_STATUS_REG,
> + &hiStatus);
> + if (err) {
> + lbs_pr_err("I/O error\n");
> + goto err;
> + }
> +
> + if (hiStatus & IF_SPI_HIST_CMD_UPLOAD_RDY) {
> + err = if_spi_c2h_cmd(card);
> + if (err)
> goto err;
> - }
> + }
> + if (hiStatus & IF_SPI_HIST_RX_UPLOAD_RDY) {
> + err = if_spi_c2h_data(card);
> + if (err)
> + goto err;
> + }
>
> - if (hiStatus & IF_SPI_HIST_CMD_UPLOAD_RDY) {
> - err = if_spi_c2h_cmd(card);
> - if (err)
> - goto err;
> - }
> - if (hiStatus & IF_SPI_HIST_RX_UPLOAD_RDY) {
> - err = if_spi_c2h_data(card);
> - if (err)
> - goto err;
> + /* workaround: in PS mode, the card does not set the Command
> + * Download Ready bit, but it sets TX Download Ready. */
> + if (hiStatus & IF_SPI_HIST_CMD_DOWNLOAD_RDY ||
> + (card->priv->psstate != PS_STATE_FULL_POWER &&
> + (hiStatus & IF_SPI_HIST_TX_DOWNLOAD_RDY))) {
> + /* This means two things. First of all,
> + * if there was a previous command sent, the card has
> + * successfully received it.
> + * Secondly, it is now ready to download another
> + * command.
> + */
> + lbs_host_to_card_done(card->priv);
> +
> + /* Do we have any command packets from the host to
> + * send? */
> + packet = NULL;
> + spin_lock_irqsave(&card->buffer_lock, flags);
> + if (!list_empty(&card->cmd_packet_list)) {
> + packet = (struct if_spi_packet *)(card->
> + cmd_packet_list.next);
> + list_del(&packet->list);
> }
> + spin_unlock_irqrestore(&card->buffer_lock, flags);
>
> - /* workaround: in PS mode, the card does not set the Command
> - * Download Ready bit, but it sets TX Download Ready. */
> - if (hiStatus & IF_SPI_HIST_CMD_DOWNLOAD_RDY ||
> - (card->priv->psstate != PS_STATE_FULL_POWER &&
> - (hiStatus & IF_SPI_HIST_TX_DOWNLOAD_RDY))) {
> - lbs_host_to_card_done(card->priv);
> + if (packet)
> + if_spi_h2c(card, packet, MVMS_CMD);
> + }
> + if (hiStatus & IF_SPI_HIST_TX_DOWNLOAD_RDY) {
> + /* Do we have any data packets from the host to
> + * send? */
> + packet = NULL;
> + spin_lock_irqsave(&card->buffer_lock, flags);
> + if (!list_empty(&card->data_packet_list)) {
> + packet = (struct if_spi_packet *)(card->
> + data_packet_list.next);
> + list_del(&packet->list);
> }
> + spin_unlock_irqrestore(&card->buffer_lock, flags);
>
> - if (hiStatus & IF_SPI_HIST_CARD_EVENT)
> - if_spi_e2h(card);
> + if (packet)
> + if_spi_h2c(card, packet, MVMS_DAT);
> + }
> + if (hiStatus & IF_SPI_HIST_CARD_EVENT)
> + if_spi_e2h(card);
>
> err:
> - if (err)
> - lbs_pr_err("%s: got error %d\n", __func__, err);
> - }
> -}
> + if (err)
> + lbs_pr_err("%s: got error %d\n", __func__, err);
>
> -/* Block until lbs_spi_thread thread has terminated */
> -static void if_spi_terminate_spi_thread(struct if_spi_card *card)
> -{
> - /* It would be nice to use kthread_stop here, but that function
> - * can't wake threads waiting for a semaphore. */
> - card->run_thread = 0;
> - up(&card->spi_ready);
> - down(&card->spi_thread_terminated);
> + lbs_deb_leave(LBS_DEB_SPI);
> }
>
> /*
> @@ -842,18 +915,40 @@ static int if_spi_host_to_card(struct lbs_private
> *priv, u8 type, u8 *buf, u16 nb)
> {
> int err = 0;
> + unsigned long flags;
> struct if_spi_card *card = priv->card;
> + struct if_spi_packet *packet;
> + u16 blen;
>
> lbs_deb_enter_args(LBS_DEB_SPI, "type %d, bytes %d", type, nb);
>
> - nb = ALIGN(nb, 4);
> + if (nb == 0) {
> + lbs_pr_err("%s: invalid size requested: %d\n", __func__, nb);
> + err = -EINVAL;
> + goto out;
> + }
> + blen = ALIGN(nb, 4);
> + packet = kzalloc(sizeof(struct if_spi_packet) + blen, GFP_ATOMIC);
> + if (!packet) {
> + err = -ENOMEM;
> + goto out;
> + }
> + packet->blen = blen;
> + memcpy(packet->buffer, buf, nb);
> + memset(packet->buffer + nb, 0, blen - nb);
>
> switch (type) {
> case MVMS_CMD:
> - err = spu_write(card, IF_SPI_CMD_RDWRPORT_REG, buf, nb);
> + priv->dnld_sent = DNLD_CMD_SENT;
> + spin_lock_irqsave(&card->buffer_lock, flags);
> + list_add_tail(&packet->list, &card->cmd_packet_list);
> + spin_unlock_irqrestore(&card->buffer_lock, flags);
> break;
> case MVMS_DAT:
> - err = spu_write(card, IF_SPI_DATA_RDWRPORT_REG, buf, nb);
> + priv->dnld_sent = DNLD_DATA_SENT;
> + spin_lock_irqsave(&card->buffer_lock, flags);
> + list_add_tail(&packet->list, &card->data_packet_list);
> + spin_unlock_irqrestore(&card->buffer_lock, flags);
> break;
> default:
> lbs_pr_err("can't transfer buffer of type %d", type);
> @@ -861,6 +956,9 @@ static int if_spi_host_to_card(struct lbs_private
> *priv, break;
> }
>
> + /* Queue spi xfer work */
> + queue_work(card->workqueue, &card->packet_work);
> +out:
> lbs_deb_leave_args(LBS_DEB_SPI, "err=%d", err);
> return err;
> }
> @@ -869,13 +967,14 @@ static int if_spi_host_to_card(struct lbs_private
> *priv, * Host Interrupts
> *
> * Service incoming interrupts from the WLAN device. We can't sleep here,
> so - * don't try to talk on the SPI bus, just wake up the SPI thread. + *
> don't try to talk on the SPI bus, just queue the SPI xfer work. */
> static irqreturn_t if_spi_host_interrupt(int irq, void *dev_id)
> {
> struct if_spi_card *card = dev_id;
>
> - up(&card->spi_ready);
> + queue_work(card->workqueue, &card->packet_work);
> +
> return IRQ_HANDLED;
> }
>
> @@ -883,56 +982,26 @@ static irqreturn_t if_spi_host_interrupt(int irq,
> void *dev_id) * SPI callbacks
> */
>
> -static int __devinit if_spi_probe(struct spi_device *spi)
> +static int if_spi_init_card(struct if_spi_card *card)
> {
> - struct if_spi_card *card;
> - struct lbs_private *priv = NULL;
> - struct libertas_spi_platform_data *pdata = spi->dev.platform_data;
> - int err = 0, i;
> + struct spi_device *spi = card->spi;
> + int err, i;
> u32 scratch;
> - struct sched_param param = { .sched_priority = 1 };
> const struct firmware *helper = NULL;
> const struct firmware *mainfw = NULL;
>
> lbs_deb_enter(LBS_DEB_SPI);
>
> - if (!pdata) {
> - err = -EINVAL;
> - goto out;
> - }
> -
> - if (pdata->setup) {
> - err = pdata->setup(spi);
> - if (err)
> - goto out;
> - }
> -
> - /* Allocate card structure to represent this specific device */
> - card = kzalloc(sizeof(struct if_spi_card), GFP_KERNEL);
> - if (!card) {
> - err = -ENOMEM;
> - goto out;
> - }
> - spi_set_drvdata(spi, card);
> - card->pdata = pdata;
> - card->spi = spi;
> - card->prev_xfer_time = jiffies;
> -
> - sema_init(&card->spi_ready, 0);
> - sema_init(&card->spi_thread_terminated, 0);
> -
> - /* Initialize the SPI Interface Unit */
> - err = spu_init(card, pdata->use_dummy_writes);
> + err = spu_init(card, card->pdata->use_dummy_writes);
> if (err)
> - goto free_card;
> + goto out;
> err = spu_get_chip_revision(card, &card->card_id, &card->card_rev);
> if (err)
> - goto free_card;
> + goto out;
>
> - /* Firmware load */
> err = spu_read_u32(card, IF_SPI_SCRATCH_4_REG, &scratch);
> if (err)
> - goto free_card;
> + goto out;
> if (scratch == SUCCESSFUL_FW_DOWNLOAD_MAGIC)
> lbs_deb_spi("Firmware is already loaded for "
> "Marvell WLAN 802.11 adapter\n");
> @@ -946,7 +1015,7 @@ static int __devinit if_spi_probe(struct spi_device
> *spi) lbs_pr_err("Unsupported chip_id: 0x%02x\n",
> card->card_id);
> err = -ENODEV;
> - goto free_card;
> + goto out;
> }
>
> err = lbs_get_firmware(&card->spi->dev, NULL, NULL,
> @@ -954,7 +1023,7 @@ static int __devinit if_spi_probe(struct spi_device
> *spi) &mainfw);
> if (err) {
> lbs_pr_err("failed to find firmware (%d)\n", err);
> - goto free_card;
> + goto out;
> }
>
> lbs_deb_spi("Initializing FW for Marvell WLAN 802.11 adapter "
> @@ -966,15 +1035,68 @@ static int __devinit if_spi_probe(struct spi_device
> *spi) spi->max_speed_hz);
> err = if_spi_prog_helper_firmware(card, helper);
> if (err)
> - goto free_card;
> + goto out;
> err = if_spi_prog_main_firmware(card, mainfw);
> if (err)
> - goto free_card;
> + goto out;
> lbs_deb_spi("loaded FW for Marvell WLAN 802.11 adapter\n");
> }
>
> err = spu_set_interrupt_mode(card, 0, 1);
> if (err)
> + goto out;
> +
> +out:
> + if (helper)
> + release_firmware(helper);
> + if (mainfw)
> + release_firmware(mainfw);
> +
> + lbs_deb_leave_args(LBS_DEB_SPI, "err %d\n", err);
> +
> + return err;
> +}
> +
> +static int __devinit if_spi_probe(struct spi_device *spi)
> +{
> + struct if_spi_card *card;
> + struct lbs_private *priv = NULL;
> + struct libertas_spi_platform_data *pdata = spi->dev.platform_data;
> + int err = 0;
> +
> + lbs_deb_enter(LBS_DEB_SPI);
> +
> + if (!pdata) {
> + err = -EINVAL;
> + goto out;
> + }
> +
> + if (pdata->setup) {
> + err = pdata->setup(spi);
> + if (err)
> + goto out;
> + }
> +
> + /* Allocate card structure to represent this specific device */
> + card = kzalloc(sizeof(struct if_spi_card), GFP_KERNEL);
> + if (!card) {
> + err = -ENOMEM;
> + goto teardown;
> + }
> + spi_set_drvdata(spi, card);
> + card->pdata = pdata;
> + card->spi = spi;
> + card->prev_xfer_time = jiffies;
> +
> + INIT_LIST_HEAD(&card->cmd_packet_list);
> + INIT_LIST_HEAD(&card->data_packet_list);
> + spin_lock_init(&card->buffer_lock);
> +
> + /* Initialize the SPI Interface Unit */
> +
> + /* Firmware load */
> + err = if_spi_init_card(card);
> + if (err)
> goto free_card;
>
> /* Register our card with libertas.
> @@ -993,27 +1115,16 @@ static int __devinit if_spi_probe(struct spi_device
> *spi) priv->fw_ready = 1;
>
> /* Initialize interrupt handling stuff. */
> - card->run_thread = 1;
> - card->spi_thread = kthread_run(lbs_spi_thread, card, "lbs_spi_thread");
> - if (IS_ERR(card->spi_thread)) {
> - card->run_thread = 0;
> - err = PTR_ERR(card->spi_thread);
> - lbs_pr_err("error creating SPI thread: err=%d\n", err);
> - goto remove_card;
> - }
> - if (sched_setscheduler(card->spi_thread, SCHED_FIFO, ¶m))
> - lbs_pr_err("Error setting scheduler, using default.\n");
> + card->workqueue = create_workqueue("libertas_spi");
> + INIT_WORK(&card->packet_work, if_spi_host_to_card_worker);
>
> err = request_irq(spi->irq, if_spi_host_interrupt,
> IRQF_TRIGGER_FALLING, "libertas_spi", card);
> if (err) {
> lbs_pr_err("can't get host irq line-- request_irq failed\n");
> - goto terminate_thread;
> + goto terminate_workqueue;
> }
>
> - /* poke the IRQ handler so that we don't miss the first interrupt */
> - up(&card->spi_ready);
> -
> /* Start the card.
> * This will call register_netdev, and we'll start
> * getting interrupts... */
> @@ -1028,18 +1139,16 @@ static int __devinit if_spi_probe(struct spi_device
> *spi)
>
> release_irq:
> free_irq(spi->irq, card);
> -terminate_thread:
> - if_spi_terminate_spi_thread(card);
> -remove_card:
> +terminate_workqueue:
> + flush_workqueue(card->workqueue);
> + destroy_workqueue(card->workqueue);
> lbs_remove_card(priv); /* will call free_netdev */
> free_card:
> free_if_spi_card(card);
> +teardown:
> + if (pdata->teardown)
> + pdata->teardown(spi);
> out:
> - if (helper)
> - release_firmware(helper);
> - if (mainfw)
> - release_firmware(mainfw);
> -
> lbs_deb_leave_args(LBS_DEB_SPI, "err %d\n", err);
> return err;
> }
> @@ -1056,7 +1165,8 @@ static int __devexit libertas_spi_remove(struct
> spi_device *spi) lbs_remove_card(priv); /* will call free_netdev */
>
> free_irq(spi->irq, card);
> - if_spi_terminate_spi_thread(card);
> + flush_workqueue(card->workqueue);
> + destroy_workqueue(card->workqueue);
> if (card->pdata->teardown)
> card->pdata->teardown(spi);
> free_if_spi_card(card);
More information about the libertas-dev
mailing list