diff mbox

[1/3] libertas_spi: Use workqueue in hw_host_to_card

Message ID 1295642690-16646-2-git-send-email-anarsoul@gmail.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Vasily Khoruzhick Jan. 21, 2011, 8:44 p.m. UTC
None

Comments

Colin McCabe April 8, 2011, 5:44 a.m. UTC | #1
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
>
--
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
diff mbox

Patch

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);