[PATCH 0/3] libertas_spi fixes

[PATCH 0/3] libertas_spi fixes

[Vasily Khoruzhick]

This series attempts to fix libertas_spi bug (it calls functions that
may sleep from atomic context) and add pm support to libertas_spi.
Tested on Zipit Z2 device.

[PATCH 1/3] libertas_spi: Use workqueue in hw_host_to_card

[Vasily Khoruzhick]

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.

Signed-off-by: Vasily Khoruzhick <anarsoul@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, &param))
-		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);
-- 
1.7.4.rc1

[PATCH 2/3] libertas: Prepare stuff for if_spi.c pm support

[Vasily Khoruzhick]

To support suspend/resume in if_spi we need two things:
- re-setup fw in lbs_resume(), because if_spi powercycles card;
- don't touch hwaddr on second lbs_update_hw_spec() call for same
  reason;

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 drivers/net/wireless/libertas/cmd.c  |   10 +++-
 drivers/net/wireless/libertas/dev.h  |    2 +
 drivers/net/wireless/libertas/main.c |   77 +++++++++++++++++----------------
 3 files changed, 49 insertions(+), 40 deletions(-)

diff --git a/drivers/net/wireless/libertas/cmd.c b/drivers/net/wireless/libertas/cmd.c
index 78c4da1..7e8a658 100644
--- a/drivers/net/wireless/libertas/cmd.c
+++ b/drivers/net/wireless/libertas/cmd.c
@@ -145,9 +145,13 @@ int lbs_update_hw_spec(struct lbs_private *priv)
 	if (priv->current_addr[0] == 0xff)
 		memmove(priv->current_addr, cmd.permanentaddr, ETH_ALEN);
 
-	memcpy(priv->dev->dev_addr, priv->current_addr, ETH_ALEN);
-	if (priv->mesh_dev)
-		memcpy(priv->mesh_dev->dev_addr, priv->current_addr, ETH_ALEN);
+	if (!priv->copied_hwaddr) {
+		memcpy(priv->dev->dev_addr, priv->current_addr, ETH_ALEN);
+		if (priv->mesh_dev)
+			memcpy(priv->mesh_dev->dev_addr,
+				priv->current_addr, ETH_ALEN);
+		priv->copied_hwaddr = 1;
+	}
 
 out:
 	lbs_deb_leave(LBS_DEB_CMD);
diff --git a/drivers/net/wireless/libertas/dev.h b/drivers/net/wireless/libertas/dev.h
index 18dd9a0..bc461eb 100644
--- a/drivers/net/wireless/libertas/dev.h
+++ b/drivers/net/wireless/libertas/dev.h
@@ -90,6 +90,7 @@ struct lbs_private {
 	void *card;
 	u8 fw_ready;
 	u8 surpriseremoved;
+	u8 setup_fw_on_resume;
 	int (*hw_host_to_card) (struct lbs_private *priv, u8 type, u8 *payload, u16 nb);
 	void (*reset_card) (struct lbs_private *priv);
 	int (*enter_deep_sleep) (struct lbs_private *priv);
@@ -101,6 +102,7 @@ struct lbs_private {
 	u32 fwcapinfo;
 	u16 regioncode;
 	u8 current_addr[ETH_ALEN];
+	u8 copied_hwaddr;
 
 	/* Command download */
 	u8 dnld_sent;
diff --git a/drivers/net/wireless/libertas/main.c b/drivers/net/wireless/libertas/main.c
index 6836a6d..ca8149c 100644
--- a/drivers/net/wireless/libertas/main.c
+++ b/drivers/net/wireless/libertas/main.c
@@ -539,6 +539,43 @@ static int lbs_thread(void *data)
 	return 0;
 }
 
+/**
+ * @brief This function gets the HW spec from the firmware and sets
+ *        some basic parameters.
+ *
+ *  @param priv    A pointer to struct lbs_private structure
+ *  @return        0 or -1
+ */
+static int lbs_setup_firmware(struct lbs_private *priv)
+{
+	int ret = -1;
+	s16 curlevel = 0, minlevel = 0, maxlevel = 0;
+
+	lbs_deb_enter(LBS_DEB_FW);
+
+	/* Read MAC address from firmware */
+	memset(priv->current_addr, 0xff, ETH_ALEN);
+	ret = lbs_update_hw_spec(priv);
+	if (ret)
+		goto done;
+
+	/* Read power levels if available */
+	ret = lbs_get_tx_power(priv, &curlevel, &minlevel, &maxlevel);
+	if (ret == 0) {
+		priv->txpower_cur = curlevel;
+		priv->txpower_min = minlevel;
+		priv->txpower_max = maxlevel;
+	}
+
+	/* Send cmd to FW to enable 11D function */
+	ret = lbs_set_snmp_mib(priv, SNMP_MIB_OID_11D_ENABLE, 1);
+
+	lbs_set_mac_control(priv);
+done:
+	lbs_deb_leave_args(LBS_DEB_FW, "ret %d", ret);
+	return ret;
+}
+
 int lbs_suspend(struct lbs_private *priv)
 {
 	int ret;
@@ -584,47 +621,13 @@ int lbs_resume(struct lbs_private *priv)
 			lbs_pr_err("deep sleep activation failed: %d\n", ret);
 	}
 
-	lbs_deb_leave_args(LBS_DEB_FW, "ret %d", ret);
-	return ret;
-}
-EXPORT_SYMBOL_GPL(lbs_resume);
-
-/**
- * @brief This function gets the HW spec from the firmware and sets
- *        some basic parameters.
- *
- *  @param priv    A pointer to struct lbs_private structure
- *  @return 	   0 or -1
- */
-static int lbs_setup_firmware(struct lbs_private *priv)
-{
-	int ret = -1;
-	s16 curlevel = 0, minlevel = 0, maxlevel = 0;
-
-	lbs_deb_enter(LBS_DEB_FW);
-
-	/* Read MAC address from firmware */
-	memset(priv->current_addr, 0xff, ETH_ALEN);
-	ret = lbs_update_hw_spec(priv);
-	if (ret)
-		goto done;
-
-	/* Read power levels if available */
-	ret = lbs_get_tx_power(priv, &curlevel, &minlevel, &maxlevel);
-	if (ret == 0) {
-		priv->txpower_cur = curlevel;
-		priv->txpower_min = minlevel;
-		priv->txpower_max = maxlevel;
-	}
+	if (priv->setup_fw_on_resume)
+		ret = lbs_setup_firmware(priv);
 
-	/* Send cmd to FW to enable 11D function */
-	ret = lbs_set_snmp_mib(priv, SNMP_MIB_OID_11D_ENABLE, 1);
-
-	lbs_set_mac_control(priv);
-done:
 	lbs_deb_leave_args(LBS_DEB_FW, "ret %d", ret);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(lbs_resume);
 
 /**
  *  This function handles the timeout of command sending.
-- 
1.7.4.rc1

[PATCH 3/3] libertas_spi: Add support for suspend/resume

[Vasily Khoruzhick]

Add support for suspend/resume in if_spi.

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 drivers/net/wireless/libertas/if_spi.c |   55 ++++++++++++++++++++++++++++++++
 1 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/libertas/if_spi.c b/drivers/net/wireless/libertas/if_spi.c
index f6c2cd6..772da3a 100644
--- a/drivers/net/wireless/libertas/if_spi.c
+++ b/drivers/net/wireless/libertas/if_spi.c
@@ -57,6 +57,7 @@ struct if_spi_card {
 	/* Handles all SPI communication (except for FW load) */
 	struct workqueue_struct		*workqueue;
 	struct work_struct		packet_work;
+	struct work_struct		resume_work;
 
 	u8				cmd_buffer[IF_SPI_CMD_BUF_SIZE];
 
@@ -1057,6 +1058,24 @@ out:
 	return err;
 }
 
+static void if_spi_resume_worker(struct work_struct *work)
+{
+	struct if_spi_card *card;
+
+	card = container_of(work, struct if_spi_card, resume_work);
+
+	if (card->pdata->setup)
+		card->pdata->setup(card->spi);
+
+	/* Init card ... */
+	if_spi_init_card(card);
+
+	enable_irq(card->spi->irq);
+
+	/* And resume it ... */
+	lbs_resume(card->priv);
+}
+
 static int __devinit if_spi_probe(struct spi_device *spi)
 {
 	struct if_spi_card *card;
@@ -1107,6 +1126,7 @@ static int __devinit if_spi_probe(struct spi_device *spi)
 		goto free_card;
 	}
 	card->priv = priv;
+	priv->setup_fw_on_resume = 1;
 	priv->card = card;
 	priv->hw_host_to_card = if_spi_host_to_card;
 	priv->enter_deep_sleep = NULL;
@@ -1117,6 +1137,7 @@ static int __devinit if_spi_probe(struct spi_device *spi)
 	/* Initialize interrupt handling stuff. */
 	card->workqueue = create_workqueue("libertas_spi");
 	INIT_WORK(&card->packet_work, if_spi_host_to_card_worker);
+	INIT_WORK(&card->resume_work, if_spi_resume_worker);
 
 	err = request_irq(spi->irq, if_spi_host_interrupt,
 			IRQF_TRIGGER_FALLING, "libertas_spi", card);
@@ -1161,6 +1182,8 @@ static int __devexit libertas_spi_remove(struct spi_device *spi)
 	lbs_deb_spi("libertas_spi_remove\n");
 	lbs_deb_enter(LBS_DEB_SPI);
 
+	cancel_work_sync(&card->resume_work);
+
 	lbs_stop_card(priv);
 	lbs_remove_card(priv); /* will call free_netdev */
 
@@ -1174,6 +1197,37 @@ static int __devexit libertas_spi_remove(struct spi_device *spi)
 	return 0;
 }
 
+static int if_spi_suspend(struct device *dev)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	struct if_spi_card *card = spi_get_drvdata(spi);
+
+	lbs_suspend(card->priv);
+	flush_workqueue(card->workqueue);
+	disable_irq(spi->irq);
+
+	if (card->pdata->teardown)
+		card->pdata->teardown(spi);
+
+	return 0;
+}
+
+static int if_spi_resume(struct device *dev)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	struct if_spi_card *card = spi_get_drvdata(spi);
+
+	/* Schedule delayed work */
+	schedule_work(&card->resume_work);
+
+	return 0;
+}
+
+static const struct dev_pm_ops if_spi_pm_ops = {
+	.suspend	= if_spi_suspend,
+	.resume		= if_spi_resume,
+};
+
 static struct spi_driver libertas_spi_driver = {
 	.probe	= if_spi_probe,
 	.remove = __devexit_p(libertas_spi_remove),
@@ -1181,6 +1235,7 @@ static struct spi_driver libertas_spi_driver = {
 		.name	= "libertas_spi",
 		.bus	= &spi_bus_type,
 		.owner	= THIS_MODULE,
+		.pm	= &if_spi_pm_ops,
 	},
 };
 
-- 
1.7.4.rc1

Re: [PATCH 1/3] libertas_spi: Use workqueue in hw_host_to_card

[Marek Vasut]

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@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, &param))
> -		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);

Re: [PATCH 1/3] libertas_spi: Use workqueue in hw_host_to_card

[Vasily Khoruzhick]

On Friday 21 January 2011 23:22:37 Marek Vasut wrote:
> 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

Because it's not easy to ensure that kthread did its job in suspend handler,
and to make if_spi.c look similar to if_sdio.c.

> 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

Actually just reverting commit does not make driver work (it will fail on 
rmmod), and it can impact on future bisect (if it'll be necessary). But if 
it's requirement - ok.

> Cheers

Re: [PATCH 3/3] libertas_spi: Add support for suspend/resume

[Marek Vasut]

On Friday 21 January 2011 21:44:50 Vasily Khoruzhick wrote:
> Add support for suspend/resume in if_spi.

Isn't there some pin to put the card asleep ? I think these LBS ones have it.

Also, do you need to do this via workqueue ? Can't you just do it in the resume 
handler?

Cheers
> 
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  drivers/net/wireless/libertas/if_spi.c |   55
> ++++++++++++++++++++++++++++++++ 1 files changed, 55 insertions(+), 0
> deletions(-)
> 
> diff --git a/drivers/net/wireless/libertas/if_spi.c
> b/drivers/net/wireless/libertas/if_spi.c index f6c2cd6..772da3a 100644
> --- a/drivers/net/wireless/libertas/if_spi.c
> +++ b/drivers/net/wireless/libertas/if_spi.c
> @@ -57,6 +57,7 @@ struct if_spi_card {
>  	/* Handles all SPI communication (except for FW load) */
>  	struct workqueue_struct		*workqueue;
>  	struct work_struct		packet_work;
> +	struct work_struct		resume_work;
> 
>  	u8				cmd_buffer[IF_SPI_CMD_BUF_SIZE];
> 
> @@ -1057,6 +1058,24 @@ out:
>  	return err;
>  }
> 
> +static void if_spi_resume_worker(struct work_struct *work)
> +{
> +	struct if_spi_card *card;
> +
> +	card = container_of(work, struct if_spi_card, resume_work);
> +
> +	if (card->pdata->setup)
> +		card->pdata->setup(card->spi);
> +
> +	/* Init card ... */
> +	if_spi_init_card(card);
> +
> +	enable_irq(card->spi->irq);
> +
> +	/* And resume it ... */
> +	lbs_resume(card->priv);
> +}
> +
>  static int __devinit if_spi_probe(struct spi_device *spi)
>  {
>  	struct if_spi_card *card;
> @@ -1107,6 +1126,7 @@ static int __devinit if_spi_probe(struct spi_device
> *spi) goto free_card;
>  	}
>  	card->priv = priv;
> +	priv->setup_fw_on_resume = 1;
>  	priv->card = card;
>  	priv->hw_host_to_card = if_spi_host_to_card;
>  	priv->enter_deep_sleep = NULL;
> @@ -1117,6 +1137,7 @@ static int __devinit if_spi_probe(struct spi_device
> *spi) /* Initialize interrupt handling stuff. */
>  	card->workqueue = create_workqueue("libertas_spi");
>  	INIT_WORK(&card->packet_work, if_spi_host_to_card_worker);
> +	INIT_WORK(&card->resume_work, if_spi_resume_worker);
> 
>  	err = request_irq(spi->irq, if_spi_host_interrupt,
>  			IRQF_TRIGGER_FALLING, "libertas_spi", card);
> @@ -1161,6 +1182,8 @@ static int __devexit libertas_spi_remove(struct
> spi_device *spi) lbs_deb_spi("libertas_spi_remove\n");
>  	lbs_deb_enter(LBS_DEB_SPI);
> 
> +	cancel_work_sync(&card->resume_work);
> +
>  	lbs_stop_card(priv);
>  	lbs_remove_card(priv); /* will call free_netdev */
> 
> @@ -1174,6 +1197,37 @@ static int __devexit libertas_spi_remove(struct
> spi_device *spi) return 0;
>  }
> 
> +static int if_spi_suspend(struct device *dev)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +	struct if_spi_card *card = spi_get_drvdata(spi);
> +
> +	lbs_suspend(card->priv);
> +	flush_workqueue(card->workqueue);
> +	disable_irq(spi->irq);
> +
> +	if (card->pdata->teardown)
> +		card->pdata->teardown(spi);
> +
> +	return 0;
> +}
> +
> +static int if_spi_resume(struct device *dev)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +	struct if_spi_card *card = spi_get_drvdata(spi);
> +
> +	/* Schedule delayed work */
> +	schedule_work(&card->resume_work);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops if_spi_pm_ops = {
> +	.suspend	= if_spi_suspend,
> +	.resume		= if_spi_resume,
> +};
> +
>  static struct spi_driver libertas_spi_driver = {
>  	.probe	= if_spi_probe,
>  	.remove = __devexit_p(libertas_spi_remove),
> @@ -1181,6 +1235,7 @@ static struct spi_driver libertas_spi_driver = {
>  		.name	= "libertas_spi",
>  		.bus	= &spi_bus_type,
>  		.owner	= THIS_MODULE,
> +		.pm	= &if_spi_pm_ops,
>  	},
>  };

Re: [PATCH 3/3] libertas_spi: Add support for suspend/resume

[Vasily Khoruzhick]

On Friday 21 January 2011 23:25:02 Marek Vasut wrote:
> On Friday 21 January 2011 21:44:50 Vasily Khoruzhick wrote:
> > Add support for suspend/resume in if_spi.
> 
> Isn't there some pin to put the card asleep ? I think these LBS ones have
> it.

Is there datasheet on these cards?
 
> Also, do you need to do this via workqueue ? Can't you just do it in the
> resume handler?

Hmm, is it OK to request firmware if userspace is not ready yet?
 
> Cheers

Regards
Vasily

Re: [PATCH 1/3] libertas_spi: Use workqueue in hw_host_to_card

[Dan Williams]

On Fri, 2011-01-21 at 23:24 +0200, Vasily Khoruzhick wrote:
> On Friday 21 January 2011 23:22:37 Marek Vasut wrote:
> > 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
> 
> Because it's not easy to ensure that kthread did its job in suspend handler,
> and to make if_spi.c look similar to if_sdio.c.
> 
> > 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
> 
> Actually just reverting commit does not make driver work (it will fail on 
> rmmod), and it can impact on future bisect (if it'll be necessary). But if 
> it's requirement - ok.

I'll disagree with Marek; I don't think it's a hard requirement as long
as there's a good reason.  And if the driver doesn't work by reverting,
then you're right bisect is broken and that sucks.  I don't have a
problem with the "mega" patch (given that it's not really that large).

Dan

Re: [PATCH 3/3] libertas_spi: Add support for suspend/resume

[Dan Williams]

On Fri, 2011-01-21 at 23:27 +0200, Vasily Khoruzhick wrote:
> On Friday 21 January 2011 23:25:02 Marek Vasut wrote:
> > On Friday 21 January 2011 21:44:50 Vasily Khoruzhick wrote:
> > > Add support for suspend/resume in if_spi.
> > 
> > Isn't there some pin to put the card asleep ? I think these LBS ones have
> > it.
> 
> Is there datasheet on these cards?

Not really, no.  With SPI and SDIO a lot of the suspend/resume handling
depends on how you wire up the card, and what you do with the SD
controller.  Often this stuff is connected to some GPIO, and that gets
handled via the platform stuff, not libertas directly (otherwise we'd
have libertas littered with board-specific code).

> > Also, do you need to do this via workqueue ? Can't you just do it in the
> > resume handler?
> 
> Hmm, is it OK to request firmware if userspace is not ready yet?

Maybe, but not safely. There's always been problems with this and the
typical thing to do is cache the firmware so you *know* you can reload
it no matter what, even before userspace has mounted the disks or
whatever.  But given that SPI is used on mainly resource-constrained
platforms, and that the libertas firmware is ~100K, that might not be
desirable.  

Dan

> > Cheers
> 
> Regards
> Vasily
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] libertas_spi fixes

[John W. Linville]

On Fri, Jan 21, 2011 at 10:44:47PM +0200, Vasily Khoruzhick wrote:
> This series attempts to fix libertas_spi bug (it calls functions that
> may sleep from atomic context) and add pm support to libertas_spi.
> Tested on Zipit Z2 device.

Are the libertas folks OK with this series?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH 0/3] libertas_spi fixes

[Dan Williams]

On Mon, 2011-01-31 at 14:52 -0500, John W. Linville wrote:
> On Fri, Jan 21, 2011 at 10:44:47PM +0200, Vasily Khoruzhick wrote:
> > This series attempts to fix libertas_spi bug (it calls functions that
> > may sleep from atomic context) and add pm support to libertas_spi.
> > Tested on Zipit Z2 device.
> 
> Are the libertas folks OK with this series?

1 & 2 yes, I'll ack them if you like.  But I think we were waiting on a
response from Vasily about #3, specifically Marek's question about "can
this be done from the resume handler instead of a workqueue".

Dan

Re: [PATCH 0/3] libertas_spi fixes

[Vasily Khoruzhick]

On Thursday 03 February 2011 20:54:52 Dan Williams wrote:
> On Mon, 2011-01-31 at 14:52 -0500, John W. Linville wrote:
> > On Fri, Jan 21, 2011 at 10:44:47PM +0200, Vasily Khoruzhick wrote:
> > > This series attempts to fix libertas_spi bug (it calls functions that
> > > may sleep from atomic context) and add pm support to libertas_spi.
> > > Tested on Zipit Z2 device.
> > 
> > Are the libertas folks OK with this series?
> 
> 1 & 2 yes, I'll ack them if you like.  But I think we were waiting on a
> response from Vasily about #3, specifically Marek's question about "can
> this be done from the resume handler instead of a workqueue".

Nope, it can't, I tried to do so and it hangs on resume (console cursor 
flickers, so it's not complete hang, but it does not return control to shell). 
Can investigate why if you want.

Regards
Vasily

Re: [PATCH 0/3] libertas_spi fixes

[Marek Vasut]

On Thursday 03 February 2011 19:59:26 Vasily Khoruzhick wrote:
> On Thursday 03 February 2011 20:54:52 Dan Williams wrote:
> > On Mon, 2011-01-31 at 14:52 -0500, John W. Linville wrote:
> > > On Fri, Jan 21, 2011 at 10:44:47PM +0200, Vasily Khoruzhick wrote:
> > > > This series attempts to fix libertas_spi bug (it calls functions that
> > > > may sleep from atomic context) and add pm support to libertas_spi.
> > > > Tested on Zipit Z2 device.
> > > 
> > > Are the libertas folks OK with this series?
> > 
> > 1 & 2 yes, I'll ack them if you like.  But I think we were waiting on a
> > response from Vasily about #3, specifically Marek's question about "can
> > this be done from the resume handler instead of a workqueue".
> 
> Nope, it can't, I tried to do so and it hangs on resume (console cursor
> flickers, so it's not complete hang, but it does not return control to
> shell). Can investigate why if you want.

Please do. It's not that I want to block this, but the code seems quick enough.

Btw. Dan, sorry for not looking into this as I promised, but as I can see, it 
isn't needed anymore. Open Source is so awesome at this :-)
> 
> Regards
> Vasily

Re: [PATCH 0/3] libertas_spi fixes

[Vasily Khoruzhick]

On Tuesday 08 February 2011 14:52:50 Marek Vasut wrote:
 
> > Nope, it can't, I tried to do so and it hangs on resume (console cursor
> > flickers, so it's not complete hang, but it does not return control to
> > shell). Can investigate why if you want.
> 
> Please do. It's not that I want to block this, but the code seems quick
> enough.
> 
> Btw. Dan, sorry for not looking into this as I promised, but as I can see,
> it isn't needed anymore. Open Source is so awesome at this :-)

Hi,

and sorry for a long delay.

It fails on firmware request, looks like pxa2xx-mci driver is not ready yet. 
It works OK without workqueue if I compile firmware into kernel (but it's not 
a way to go, right?)

Regards
Vasily

Re: [PATCH 0/3] libertas_spi fixes

[Vasily Khoruzhick]

On Saturday 26 February 2011 11:44:43 Vasily Khoruzhick wrote:
> On Tuesday 08 February 2011 14:52:50 Marek Vasut wrote:
> > > Nope, it can't, I tried to do so and it hangs on resume (console cursor
> > > flickers, so it's not complete hang, but it does not return control to
> > > shell). Can investigate why if you want.
> > 
> > Please do. It's not that I want to block this, but the code seems quick
> > enough.
> > 
> > Btw. Dan, sorry for not looking into this as I promised, but as I can
> > see, it isn't needed anymore. Open Source is so awesome at this :-)
> 
> Hi,
> 
> and sorry for a long delay.
> 
> It fails on firmware request, looks like pxa2xx-mci driver is not ready
> yet. It works OK without workqueue if I compile firmware into kernel (but
> it's not a way to go, right?)

Ping :) should I resend 3rd patch?

Regards
Vasily

Re: [PATCH 0/3] libertas_spi fixes

[John W. Linville]

On Tue, Mar 15, 2011 at 10:31:25PM +0200, Vasily Khoruzhick wrote:
> On Saturday 26 February 2011 11:44:43 Vasily Khoruzhick wrote:
> > On Tuesday 08 February 2011 14:52:50 Marek Vasut wrote:
> > > > Nope, it can't, I tried to do so and it hangs on resume (console cursor
> > > > flickers, so it's not complete hang, but it does not return control to
> > > > shell). Can investigate why if you want.
> > > 
> > > Please do. It's not that I want to block this, but the code seems quick
> > > enough.
> > > 
> > > Btw. Dan, sorry for not looking into this as I promised, but as I can
> > > see, it isn't needed anymore. Open Source is so awesome at this :-)
> > 
> > Hi,
> > 
> > and sorry for a long delay.
> > 
> > It fails on firmware request, looks like pxa2xx-mci driver is not ready
> > yet. It works OK without workqueue if I compile firmware into kernel (but
> > it's not a way to go, right?)
> 
> Ping :) should I resend 3rd patch?

It would seem so...

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

[PATCH RESEND] libertas_spi: Add support for suspend/resume

[Vasily Khoruzhick]

Add support for suspend/resume in if_spi.

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 drivers/net/wireless/libertas/if_spi.c |   65 ++++++++++++++++++++++++++++++++
 1 files changed, 65 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/libertas/if_spi.c b/drivers/net/wireless/libertas/if_spi.c
index f6c2cd6..078ef43 100644
--- a/drivers/net/wireless/libertas/if_spi.c
+++ b/drivers/net/wireless/libertas/if_spi.c
@@ -57,6 +57,7 @@ struct if_spi_card {
 	/* Handles all SPI communication (except for FW load) */
 	struct workqueue_struct		*workqueue;
 	struct work_struct		packet_work;
+	struct work_struct		resume_work;
 
 	u8				cmd_buffer[IF_SPI_CMD_BUF_SIZE];
 
@@ -68,6 +69,9 @@ struct if_spi_card {
 
 	/* Protects cmd_packet_list and data_packet_list */
 	spinlock_t			buffer_lock;
+
+	/* True is card suspended */
+	u8				suspended;
 };
 
 static void free_if_spi_card(struct if_spi_card *card)
@@ -1057,6 +1061,28 @@ out:
 	return err;
 }
 
+static void if_spi_resume_worker(struct work_struct *work)
+{
+	struct if_spi_card *card;
+
+	card = container_of(work, struct if_spi_card, resume_work);
+
+	if (card->suspended) {
+		if (card->pdata->setup)
+			card->pdata->setup(card->spi);
+
+		/* Init card ... */
+		if_spi_init_card(card);
+
+		enable_irq(card->spi->irq);
+
+		/* And resume it ... */
+		lbs_resume(card->priv);
+
+		card->suspended = 0;
+	}
+}
+
 static int __devinit if_spi_probe(struct spi_device *spi)
 {
 	struct if_spi_card *card;
@@ -1107,6 +1133,7 @@ static int __devinit if_spi_probe(struct spi_device *spi)
 		goto free_card;
 	}
 	card->priv = priv;
+	priv->setup_fw_on_resume = 1;
 	priv->card = card;
 	priv->hw_host_to_card = if_spi_host_to_card;
 	priv->enter_deep_sleep = NULL;
@@ -1117,6 +1144,7 @@ static int __devinit if_spi_probe(struct spi_device *spi)
 	/* Initialize interrupt handling stuff. */
 	card->workqueue = create_workqueue("libertas_spi");
 	INIT_WORK(&card->packet_work, if_spi_host_to_card_worker);
+	INIT_WORK(&card->resume_work, if_spi_resume_worker);
 
 	err = request_irq(spi->irq, if_spi_host_interrupt,
 			IRQF_TRIGGER_FALLING, "libertas_spi", card);
@@ -1161,6 +1189,8 @@ static int __devexit libertas_spi_remove(struct spi_device *spi)
 	lbs_deb_spi("libertas_spi_remove\n");
 	lbs_deb_enter(LBS_DEB_SPI);
 
+	cancel_work_sync(&card->resume_work);
+
 	lbs_stop_card(priv);
 	lbs_remove_card(priv); /* will call free_netdev */
 
@@ -1174,6 +1204,40 @@ static int __devexit libertas_spi_remove(struct spi_device *spi)
 	return 0;
 }
 
+static int if_spi_suspend(struct device *dev)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	struct if_spi_card *card = spi_get_drvdata(spi);
+
+	if (!card->suspended) {
+		lbs_suspend(card->priv);
+		flush_workqueue(card->workqueue);
+		disable_irq(spi->irq);
+
+		if (card->pdata->teardown)
+			card->pdata->teardown(spi);
+		card->suspended = 1;
+	}
+
+	return 0;
+}
+
+static int if_spi_resume(struct device *dev)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	struct if_spi_card *card = spi_get_drvdata(spi);
+
+	/* Schedule delayed work */
+	schedule_work(&card->resume_work);
+
+	return 0;
+}
+
+static const struct dev_pm_ops if_spi_pm_ops = {
+	.suspend	= if_spi_suspend,
+	.resume		= if_spi_resume,
+};
+
 static struct spi_driver libertas_spi_driver = {
 	.probe	= if_spi_probe,
 	.remove = __devexit_p(libertas_spi_remove),
@@ -1181,6 +1245,7 @@ static struct spi_driver libertas_spi_driver = {
 		.name	= "libertas_spi",
 		.bus	= &spi_bus_type,
 		.owner	= THIS_MODULE,
+		.pm	= &if_spi_pm_ops,
 	},
 };
 
-- 
1.7.4.1

Re: [PATCH RESEND] libertas_spi: Add support for suspend/resume

[Dan Williams]

On Wed, 2011-03-16 at 16:41 +0200, Vasily Khoruzhick wrote:
> Add support for suspend/resume in if_spi.
> 
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>

Unless Marek chimes in, I don't have a problem with this patch if it
fixes the problem.  The one issue I can see is that if userspace isn't
ready yet, the request_firmware() call will fail because we're not
caching firmware anywhere, and we probably shouldn't be since the
firmware is large.  I'm not sure there's a good solution without caching
firmware though, but I don't want that to block the patch...

Acked-by: Dan Williams <dcbw@redhat.com>

> ---
>  drivers/net/wireless/libertas/if_spi.c |   65 ++++++++++++++++++++++++++++++++
>  1 files changed, 65 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/wireless/libertas/if_spi.c b/drivers/net/wireless/libertas/if_spi.c
> index f6c2cd6..078ef43 100644
> --- a/drivers/net/wireless/libertas/if_spi.c
> +++ b/drivers/net/wireless/libertas/if_spi.c
> @@ -57,6 +57,7 @@ struct if_spi_card {
>  	/* Handles all SPI communication (except for FW load) */
>  	struct workqueue_struct		*workqueue;
>  	struct work_struct		packet_work;
> +	struct work_struct		resume_work;
>  
>  	u8				cmd_buffer[IF_SPI_CMD_BUF_SIZE];
>  
> @@ -68,6 +69,9 @@ struct if_spi_card {
>  
>  	/* Protects cmd_packet_list and data_packet_list */
>  	spinlock_t			buffer_lock;
> +
> +	/* True is card suspended */
> +	u8				suspended;
>  };
>  
>  static void free_if_spi_card(struct if_spi_card *card)
> @@ -1057,6 +1061,28 @@ out:
>  	return err;
>  }
>  
> +static void if_spi_resume_worker(struct work_struct *work)
> +{
> +	struct if_spi_card *card;
> +
> +	card = container_of(work, struct if_spi_card, resume_work);
> +
> +	if (card->suspended) {
> +		if (card->pdata->setup)
> +			card->pdata->setup(card->spi);
> +
> +		/* Init card ... */
> +		if_spi_init_card(card);
> +
> +		enable_irq(card->spi->irq);
> +
> +		/* And resume it ... */
> +		lbs_resume(card->priv);
> +
> +		card->suspended = 0;
> +	}
> +}
> +
>  static int __devinit if_spi_probe(struct spi_device *spi)
>  {
>  	struct if_spi_card *card;
> @@ -1107,6 +1133,7 @@ static int __devinit if_spi_probe(struct spi_device *spi)
>  		goto free_card;
>  	}
>  	card->priv = priv;
> +	priv->setup_fw_on_resume = 1;
>  	priv->card = card;
>  	priv->hw_host_to_card = if_spi_host_to_card;
>  	priv->enter_deep_sleep = NULL;
> @@ -1117,6 +1144,7 @@ static int __devinit if_spi_probe(struct spi_device *spi)
>  	/* Initialize interrupt handling stuff. */
>  	card->workqueue = create_workqueue("libertas_spi");
>  	INIT_WORK(&card->packet_work, if_spi_host_to_card_worker);
> +	INIT_WORK(&card->resume_work, if_spi_resume_worker);
>  
>  	err = request_irq(spi->irq, if_spi_host_interrupt,
>  			IRQF_TRIGGER_FALLING, "libertas_spi", card);
> @@ -1161,6 +1189,8 @@ static int __devexit libertas_spi_remove(struct spi_device *spi)
>  	lbs_deb_spi("libertas_spi_remove\n");
>  	lbs_deb_enter(LBS_DEB_SPI);
>  
> +	cancel_work_sync(&card->resume_work);
> +
>  	lbs_stop_card(priv);
>  	lbs_remove_card(priv); /* will call free_netdev */
>  
> @@ -1174,6 +1204,40 @@ static int __devexit libertas_spi_remove(struct spi_device *spi)
>  	return 0;
>  }
>  
> +static int if_spi_suspend(struct device *dev)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +	struct if_spi_card *card = spi_get_drvdata(spi);
> +
> +	if (!card->suspended) {
> +		lbs_suspend(card->priv);
> +		flush_workqueue(card->workqueue);
> +		disable_irq(spi->irq);
> +
> +		if (card->pdata->teardown)
> +			card->pdata->teardown(spi);
> +		card->suspended = 1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int if_spi_resume(struct device *dev)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +	struct if_spi_card *card = spi_get_drvdata(spi);
> +
> +	/* Schedule delayed work */
> +	schedule_work(&card->resume_work);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops if_spi_pm_ops = {
> +	.suspend	= if_spi_suspend,
> +	.resume		= if_spi_resume,
> +};
> +
>  static struct spi_driver libertas_spi_driver = {
>  	.probe	= if_spi_probe,
>  	.remove = __devexit_p(libertas_spi_remove),
> @@ -1181,6 +1245,7 @@ static struct spi_driver libertas_spi_driver = {
>  		.name	= "libertas_spi",
>  		.bus	= &spi_bus_type,
>  		.owner	= THIS_MODULE,
> +		.pm	= &if_spi_pm_ops,
>  	},
>  };
>  

Re: [PATCH RESEND] libertas_spi: Add support for suspend/resume

[Marek Vasut]

On Thursday 17 March 2011 17:44:27 Dan Williams wrote:
> On Wed, 2011-03-16 at 16:41 +0200, Vasily Khoruzhick wrote:
> > Add support for suspend/resume in if_spi.
> > 
> > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> 
> Unless Marek chimes in, I don't have a problem with this patch if it
> fixes the problem.  The one issue I can see is that if userspace isn't
> ready yet, the request_firmware() call will fail because we're not
> caching firmware anywhere, and we probably shouldn't be since the
> firmware is large.  I'm not sure there's a good solution without caching
> firmware though, but I don't want that to block the patch...

Maybe we should implement in-kernel FW cache for this kind of situations. Such 
feature could be configurable too ?

Cheers
> 
> Acked-by: Dan Williams <dcbw@redhat.com>
> 
> > ---
> > 
> >  drivers/net/wireless/libertas/if_spi.c |   65
> >  ++++++++++++++++++++++++++++++++ 1 files changed, 65 insertions(+), 0
> >  deletions(-)
> > 
> > diff --git a/drivers/net/wireless/libertas/if_spi.c
> > b/drivers/net/wireless/libertas/if_spi.c index f6c2cd6..078ef43 100644
> > --- a/drivers/net/wireless/libertas/if_spi.c
> > +++ b/drivers/net/wireless/libertas/if_spi.c
> > @@ -57,6 +57,7 @@ struct if_spi_card {
> > 
> >  	/* Handles all SPI communication (except for FW load) */
> >  	struct workqueue_struct		*workqueue;
> >  	struct work_struct		packet_work;
> > 
> > +	struct work_struct		resume_work;
> > 
> >  	u8				cmd_buffer[IF_SPI_CMD_BUF_SIZE];
> > 
> > @@ -68,6 +69,9 @@ struct if_spi_card {
> > 
> >  	/* Protects cmd_packet_list and data_packet_list */
> >  	spinlock_t			buffer_lock;
> > 
> > +
> > +	/* True is card suspended */
> > +	u8				suspended;
> > 
> >  };
> >  
> >  static void free_if_spi_card(struct if_spi_card *card)
> > 
> > @@ -1057,6 +1061,28 @@ out:
> >  	return err;
> >  
> >  }
> > 
> > +static void if_spi_resume_worker(struct work_struct *work)
> > +{
> > +	struct if_spi_card *card;
> > +
> > +	card = container_of(work, struct if_spi_card, resume_work);
> > +
> > +	if (card->suspended) {
> > +		if (card->pdata->setup)
> > +			card->pdata->setup(card->spi);
> > +
> > +		/* Init card ... */
> > +		if_spi_init_card(card);
> > +
> > +		enable_irq(card->spi->irq);
> > +
> > +		/* And resume it ... */
> > +		lbs_resume(card->priv);
> > +
> > +		card->suspended = 0;
> > +	}
> > +}
> > +
> > 
> >  static int __devinit if_spi_probe(struct spi_device *spi)
> >  {
> >  
> >  	struct if_spi_card *card;
> > 
> > @@ -1107,6 +1133,7 @@ static int __devinit if_spi_probe(struct spi_device
> > *spi)
> > 
> >  		goto free_card;
> >  	
> >  	}
> >  	card->priv = priv;
> > 
> > +	priv->setup_fw_on_resume = 1;
> > 
> >  	priv->card = card;
> >  	priv->hw_host_to_card = if_spi_host_to_card;
> >  	priv->enter_deep_sleep = NULL;
> > 
> > @@ -1117,6 +1144,7 @@ static int __devinit if_spi_probe(struct spi_device
> > *spi)
> > 
> >  	/* Initialize interrupt handling stuff. */
> >  	card->workqueue = create_workqueue("libertas_spi");
> >  	INIT_WORK(&card->packet_work, if_spi_host_to_card_worker);
> > 
> > +	INIT_WORK(&card->resume_work, if_spi_resume_worker);
> > 
> >  	err = request_irq(spi->irq, if_spi_host_interrupt,
> >  	
> >  			IRQF_TRIGGER_FALLING, "libertas_spi", card);
> > 
> > @@ -1161,6 +1189,8 @@ static int __devexit libertas_spi_remove(struct
> > spi_device *spi)
> > 
> >  	lbs_deb_spi("libertas_spi_remove\n");
> >  	lbs_deb_enter(LBS_DEB_SPI);
> > 
> > +	cancel_work_sync(&card->resume_work);
> > +
> > 
> >  	lbs_stop_card(priv);
> >  	lbs_remove_card(priv); /* will call free_netdev */
> > 
> > @@ -1174,6 +1204,40 @@ static int __devexit libertas_spi_remove(struct
> > spi_device *spi)
> > 
> >  	return 0;
> >  
> >  }
> > 
> > +static int if_spi_suspend(struct device *dev)
> > +{
> > +	struct spi_device *spi = to_spi_device(dev);
> > +	struct if_spi_card *card = spi_get_drvdata(spi);
> > +
> > +	if (!card->suspended) {
> > +		lbs_suspend(card->priv);
> > +		flush_workqueue(card->workqueue);
> > +		disable_irq(spi->irq);
> > +
> > +		if (card->pdata->teardown)
> > +			card->pdata->teardown(spi);
> > +		card->suspended = 1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int if_spi_resume(struct device *dev)
> > +{
> > +	struct spi_device *spi = to_spi_device(dev);
> > +	struct if_spi_card *card = spi_get_drvdata(spi);
> > +
> > +	/* Schedule delayed work */
> > +	schedule_work(&card->resume_work);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops if_spi_pm_ops = {
> > +	.suspend	= if_spi_suspend,
> > +	.resume		= if_spi_resume,
> > +};
> > +
> > 
> >  static struct spi_driver libertas_spi_driver = {
> >  
> >  	.probe	= if_spi_probe,
> >  	.remove = __devexit_p(libertas_spi_remove),
> > 
> > @@ -1181,6 +1245,7 @@ static struct spi_driver libertas_spi_driver = {
> > 
> >  		.name	= "libertas_spi",
> >  		.bus	= &spi_bus_type,
> >  		.owner	= THIS_MODULE,
> > 
> > +		.pm	= &if_spi_pm_ops,
> > 
> >  	},
> >  
> >  };

Re: [PATCH 1/3] libertas_spi: Use workqueue in hw_host_to_card

[Colin McCabe]

Thanks for fixing up if_spi.c, Vasily. I was aware that breakage had
entered the driver, but I didn't have access to the hardware any more,
so I couldn't fix it.

Together with Andrey, I implemented the original driver in a week or
two. We always hoped to find a better solution than the giant list of
buffers that ended up being created in if_spi_host_to_card. However,
time was a factor and we ended up merging the giant list solution.
(Yes, I'm aware that if_sdio.c uses the same approach, but that
doesn't make it right.)

At the very least, we should be keeping track of the length of that
list, and failing in if_spi_host_to_card if it gets too long.
Otherwise, we could theoretically allocate an unlimited amount of
kernel memory in that function. It's especially bad because the
allocations are being done with GFP_ATOMIC. Buffering the packets in
this way also introduces unwanted latency (also called bufferbloat).

I wish that there were some way to just put the sk_buff into a list of
our own in the GSPI driver, rather than copying its entire contents.
It might also be feasible to avoid calling if_spi_host_to_card (and
the other libertas host-to-card functions) from atomic contexts. In
that case, we could just send the data immediately. You'd have to
change the core driver somewhat to make either change happen, though.

Such a change would improve both latency and throughput, of course :)

cheers,
Colin


On Fri, Jan 21, 2011 at 1:24 PM, Vasily Khoruzhick <anarsoul@gmail.com> wrote:
> On Friday 21 January 2011 23:22:37 Marek Vasut wrote:
>> 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
>
> Because it's not easy to ensure that kthread did its job in suspend handler,
> and to make if_spi.c look similar to if_sdio.c.
>
>> 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
>
> Actually just reverting commit does not make driver work (it will fail on
> rmmod), and it can impact on future bisect (if it'll be necessary). But if
> it's requirement - ok.
>
>> Cheers
>
> _______________________________________________
> libertas-dev mailing list
> libertas-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/libertas-dev
>