[PATCH] libertas: remove internal buffers from GSPI driver

[PATCH] libertas: remove internal buffers from GSPI driver

[Andrey Yurovsky]

This patch removes the internal command and data buffers that the GSPI driver
maintained and instead relies on the Libertas core to synchronize access
to the command and data ports as with the other interface drivers.  This
cleanup reduces the GSPI driver's memory footprint and should improve
performance by removing the need to copy to these internal buffers.
This also simplifies the bottom half of the interrupt handler.

This is an incremental cleanup: after removing the redundant buffers, we
can further improve the driver to use a threaded IRQ handler instead of
maintaining its own thread.  However I would like a few folks to test
the buffer removal first and make sure that I'm not introducing
regressions.

Tested on Blackfin BF527 with DMA disabled due to an issue with the SPI
host controller driver in the current bleeding-edge Blackfin kernel.  I
would appreciate it if someone with working DMA could test this patch
and provide feedback.

Signed-off-by: Andrey Yurovsky <andrey@cozybit.com>
---
 drivers/net/wireless/libertas/if_spi.c |  136 ++------------------------------
 1 files changed, 6 insertions(+), 130 deletions(-)

diff --git a/drivers/net/wireless/libertas/if_spi.c b/drivers/net/wireless/libertas/if_spi.c
index 06df2e1..9e0096d 100644
--- a/drivers/net/wireless/libertas/if_spi.c
+++ b/drivers/net/wireless/libertas/if_spi.c
@@ -32,12 +32,6 @@
 #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;
@@ -66,33 +60,10 @@ struct if_spi_card {
 	struct semaphore		spi_thread_terminated;
 
 	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;
-
-	BUG_ON(card->run_thread);
-	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);
 }
@@ -774,40 +745,6 @@ 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)
 {
@@ -837,8 +774,6 @@ static int lbs_spi_thread(void *data)
 	int err;
 	struct if_spi_card *card = data;
 	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
@@ -877,43 +812,9 @@ static int lbs_spi_thread(void *data)
 		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);
-
-			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 (packet)
-				if_spi_h2c(card, packet, MVMS_DAT);
-		}
 		if (hiStatus & IF_SPI_HIST_CARD_EVENT)
 			if_spi_e2h(card);
 
@@ -942,40 +843,18 @@ 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);
 
-	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);
+	nb = ALIGN(nb, 4);
 
 	switch (type) {
 	case MVMS_CMD:
-		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);
+		err = spu_write(card, IF_SPI_CMD_RDWRPORT_REG, buf, nb);
 		break;
 	case MVMS_DAT:
-		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);
+		err = spu_write(card, IF_SPI_DATA_RDWRPORT_REG, buf, nb);
 		break;
 	default:
 		lbs_pr_err("can't transfer buffer of type %d", type);
@@ -983,9 +862,6 @@ static int if_spi_host_to_card(struct lbs_private *priv,
 		break;
 	}
 
-	/* Wake up the spi thread */
-	up(&card->spi_ready);
-out:
 	lbs_deb_leave_args(LBS_DEB_SPI, "err=%d", err);
 	return err;
 }
@@ -1062,9 +938,6 @@ static int __devinit if_spi_probe(struct spi_device *spi)
 
 	sema_init(&card->spi_ready, 0);
 	sema_init(&card->spi_thread_terminated, 0);
-	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 */
 	err = spu_init(card, pdata->use_dummy_writes);
@@ -1141,6 +1014,9 @@ static int __devinit if_spi_probe(struct spi_device *spi)
 		goto terminate_thread;
 	}
 
+	/* 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... */
-- 
1.5.6.3

Re: [PATCH] libertas: remove internal buffers from GSPI driver

[Dan Williams]

On Tue, 2009-10-27 at 16:51 -0700, Andrey Yurovsky wrote:
> This patch removes the internal command and data buffers that the GSPI driver
> maintained and instead relies on the Libertas core to synchronize access
> to the command and data ports as with the other interface drivers.  This
> cleanup reduces the GSPI driver's memory footprint and should improve
> performance by removing the need to copy to these internal buffers.
> This also simplifies the bottom half of the interrupt handler.
> 
> This is an incremental cleanup: after removing the redundant buffers, we
> can further improve the driver to use a threaded IRQ handler instead of
> maintaining its own thread.  However I would like a few folks to test
> the buffer removal first and make sure that I'm not introducing
> regressions.
> 
> Tested on Blackfin BF527 with DMA disabled due to an issue with the SPI
> host controller driver in the current bleeding-edge Blackfin kernel.  I
> would appreciate it if someone with working DMA could test this patch
> and provide feedback.
> 
> Signed-off-by: Andrey Yurovsky <andrey@cozybit.com>

Looks OK to me, but I don't have any of the SPI hardware so when you get
a Tested-by:, let me know and I'll ack.

Dan

> ---
>  drivers/net/wireless/libertas/if_spi.c |  136 ++------------------------------
>  1 files changed, 6 insertions(+), 130 deletions(-)
> 
> diff --git a/drivers/net/wireless/libertas/if_spi.c b/drivers/net/wireless/libertas/if_spi.c
> index 06df2e1..9e0096d 100644
> --- a/drivers/net/wireless/libertas/if_spi.c
> +++ b/drivers/net/wireless/libertas/if_spi.c
> @@ -32,12 +32,6 @@
>  #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;
> @@ -66,33 +60,10 @@ struct if_spi_card {
>  	struct semaphore		spi_thread_terminated;
>  
>  	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;
> -
> -	BUG_ON(card->run_thread);
> -	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);
>  }
> @@ -774,40 +745,6 @@ 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)
>  {
> @@ -837,8 +774,6 @@ static int lbs_spi_thread(void *data)
>  	int err;
>  	struct if_spi_card *card = data;
>  	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
> @@ -877,43 +812,9 @@ static int lbs_spi_thread(void *data)
>  		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);
> -
> -			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 (packet)
> -				if_spi_h2c(card, packet, MVMS_DAT);
> -		}
>  		if (hiStatus & IF_SPI_HIST_CARD_EVENT)
>  			if_spi_e2h(card);
>  
> @@ -942,40 +843,18 @@ 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);
>  
> -	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);
> +	nb = ALIGN(nb, 4);
>  
>  	switch (type) {
>  	case MVMS_CMD:
> -		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);
> +		err = spu_write(card, IF_SPI_CMD_RDWRPORT_REG, buf, nb);
>  		break;
>  	case MVMS_DAT:
> -		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);
> +		err = spu_write(card, IF_SPI_DATA_RDWRPORT_REG, buf, nb);
>  		break;
>  	default:
>  		lbs_pr_err("can't transfer buffer of type %d", type);
> @@ -983,9 +862,6 @@ static int if_spi_host_to_card(struct lbs_private *priv,
>  		break;
>  	}
>  
> -	/* Wake up the spi thread */
> -	up(&card->spi_ready);
> -out:
>  	lbs_deb_leave_args(LBS_DEB_SPI, "err=%d", err);
>  	return err;
>  }
> @@ -1062,9 +938,6 @@ static int __devinit if_spi_probe(struct spi_device *spi)
>  
>  	sema_init(&card->spi_ready, 0);
>  	sema_init(&card->spi_thread_terminated, 0);
> -	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 */
>  	err = spu_init(card, pdata->use_dummy_writes);
> @@ -1141,6 +1014,9 @@ static int __devinit if_spi_probe(struct spi_device *spi)
>  		goto terminate_thread;
>  	}
>  
> +	/* 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... */

RE: [PATCH] libertas: remove internal buffers from GSPI driver

[George Shore]

> -----Original Message-----

> From: linux-wireless-owner@vger.kernel.org [mailto:linux-wireless-

> owner@vger.kernel.org] On Behalf Of Andrey Yurovsky

> Sent: 27 October 2009 23:52

> To: linux-wireless@vger.kernel.org

> Cc: libertas-dev@lists.infradead.org; sebatian@breakpoint.cc;

> dcbw@redhat.com; Andrey Yurovsky

> Subject: [PATCH] libertas: remove internal buffers from GSPI driver

> 

> This patch removes the internal command and data buffers that the GSPI

> driver

> maintained and instead relies on the Libertas core to synchronize

access

> to the command and data ports as with the other interface drivers.

This

> cleanup reduces the GSPI driver's memory footprint and should improve

> performance by removing the need to copy to these internal buffers.

> This also simplifies the bottom half of the interrupt handler.

> 

> This is an incremental cleanup: after removing the redundant buffers,

we

> can further improve the driver to use a threaded IRQ handler instead

of

> maintaining its own thread.  However I would like a few folks to test

> the buffer removal first and make sure that I'm not introducing

> regressions.

> 

> Tested on Blackfin BF527 with DMA disabled due to an issue with the

SPI

> host controller driver in the current bleeding-edge Blackfin kernel.

I

> would appreciate it if someone with working DMA could test this patch

> and provide feedback.

> 

I've tested this on our in-house dev board with our in-house CPU, on the

8686 in GSPI mode. Everything works as expected :)



Tested-by: George Shore <george.shore@imgtec.com>



> Signed-off-by: Andrey Yurovsky <andrey@cozybit.com>

> ---

>  drivers/net/wireless/libertas/if_spi.c |  136

++-------------------------

> -----

>  1 files changed, 6 insertions(+), 130 deletions(-)

> 

> diff --git a/drivers/net/wireless/libertas/if_spi.c

> b/drivers/net/wireless/libertas/if_spi.c

> index 06df2e1..9e0096d 100644

> --- a/drivers/net/wireless/libertas/if_spi.c

> +++ b/drivers/net/wireless/libertas/if_spi.c

> @@ -32,12 +32,6 @@

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

> @@ -66,33 +60,10 @@ struct if_spi_card {

>  	struct semaphore		spi_thread_terminated;

> 

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

> -

> -	BUG_ON(card->run_thread);

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

>  }

> @@ -774,40 +745,6 @@ 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)

>  {

> @@ -837,8 +774,6 @@ static int lbs_spi_thread(void *data)

>  	int err;

>  	struct if_spi_card *card = data;

>  	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

> @@ -877,43 +812,9 @@ static int lbs_spi_thread(void *data)

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

> -

> -			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 (packet)

> -				if_spi_h2c(card, packet, MVMS_DAT);

> -		}

>  		if (hiStatus & IF_SPI_HIST_CARD_EVENT)

>  			if_spi_e2h(card);

> 

> @@ -942,40 +843,18 @@ 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);

> 

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

> +	nb = ALIGN(nb, 4);

> 

>  	switch (type) {

>  	case MVMS_CMD:

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

> +		err = spu_write(card, IF_SPI_CMD_RDWRPORT_REG, buf, nb);

>  		break;

>  	case MVMS_DAT:

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

> +		err = spu_write(card, IF_SPI_DATA_RDWRPORT_REG, buf,

nb);

>  		break;

>  	default:

>  		lbs_pr_err("can't transfer buffer of type %d", type);

> @@ -983,9 +862,6 @@ static int if_spi_host_to_card(struct lbs_private

> *priv,

>  		break;

>  	}

> 

> -	/* Wake up the spi thread */

> -	up(&card->spi_ready);

> -out:

>  	lbs_deb_leave_args(LBS_DEB_SPI, "err=%d", err);

>  	return err;

>  }

> @@ -1062,9 +938,6 @@ static int __devinit if_spi_probe(struct

spi_device

> *spi)

> 

>  	sema_init(&card->spi_ready, 0);

>  	sema_init(&card->spi_thread_terminated, 0);

> -	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 */

>  	err = spu_init(card, pdata->use_dummy_writes);

> @@ -1141,6 +1014,9 @@ static int __devinit if_spi_probe(struct

spi_device

> *spi)

>  		goto terminate_thread;

>  	}

> 

> +	/* 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... */

> --

> 1.5.6.3

> 

> --

> 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

-

This message is subject to Imagination Technologies' e-mail terms: http://www.imgtec.com/e-mail.htm



Imagination Technologies Ltd is a limited company registered in England No:  1306335 

Registered Office: Imagination House, Home Park Estate, Kings Langley, Hertfordshire, WD4 8LZ.  



Email to and from the company may be monitored for compliance and other administrative purposes.  

-

Re: [PATCH] libertas: remove internal buffers from GSPI driver

[Andrey Yurovsky]

Thanks George,

On Thu, Oct 29, 2009 at 7:27 AM, George Shore <George.Shore@imgtec.com> wrote:
> I've tested this on our in-house dev board with our in-house CPU, on the
> 8686 in GSPI mode. Everything works as expected :)
>
> Tested-by: George Shore <george.shore@imgtec.com>

Can you please tell us what you were able to test?  I'm curious about:
1) were you able to turn IEEE PS on and off?
2) were you able to use WPA2?
3) were you able to use WPA2 with IEEE PS at the same time?

(3) is really tricky and may need additional work.  One issue that I'm
seeing is that the driver gets out of sync with the device, the way
I'm reproducing this is:

1) use wpa_supplicant to associate to an AP with WPA2 security.
2) iwconfig eth1 power on -- you should see the LED blinking to
indicate IEEE PS (the LED is off while the device is sleeping)
3) issue a command or two to temporarily take the device out of IEEE
PS mode (ex: iwconfig with no arguments to look at the configuration
and stats)
4) the LED should be temporarily solid-on and then return to blinking
after the driver re-enables IEEE PS mode

Thanks again,

  -Andrey

Re: [PATCH] libertas: remove internal buffers from GSPI driver

[Andrey Yurovsky]

On Wed, Oct 28, 2009 at 10:13 AM, Dan Williams <dcbw@redhat.com> wrote:
> Looks OK to me, but I don't have any of the SPI hardware so when you get
> a Tested-by:, let me know and I'll ack.

Thanks Dan, by the way the issue with WPA2 and IEEE PS is unrelated to
this patch (that is, I can reproduce it without the patch applied) so
we'll need to address that separately.  So at this point we have:

Tested-by: George Shore <george.shore@imgtec.com>
Signed-off-by: Andrey Yurovsky <andrey@cozybit.com>

RE: [PATCH] libertas: remove internal buffers from GSPI driver

[George Shore]

Andrey,



I've been unable to fully test the WPA2 functionality - I've not got

access to a WPA2 enabled access point - however, I have had a chance to

try the PS mode while connected to a WEP access point (something I

neglected to test first time around - my apologies :-S )



After connecting to the AP, I enabled PS mode and then tried the

following commands (in order):



* iwconfig (no arguments)

  This reurned the connection information, and indicated that power

namagement had been enabled;



* iwlist wlan0 scan

  This gave the following in my log:

[  741.343474] libertas: if_spi_c2h_cmd: error: response packet too

large: 4616 bytes, but maximum is 2400

[  741.370373] libertas: if_spi_c2h_cmd: err=-22

[  741.396573] libertas: lbs_spi_thread: got error -22

[  741.426908] libertas: if_spi_c2h_cmd: error: response packet too

large: 4616 bytes, but maximum is 2400

[  741.453290] libertas: if_spi_c2h_cmd: err=-22

[  741.466275] libertas: lbs_spi_thread: got error -22

[  743.454090] libertas: if_spi_c2h_cmd: error: response packet too

large: 4616 bytes, but maximum is 2400

[  743.480131] libertas: if_spi_c2h_cmd: err=-22

[  743.505172] libertas: lbs_spi_thread: got error -22

[  747.896416] libertas: if_spi_c2h_cmd: error: response packet too

large: 4616 bytes, but maximum is 2400

[  747.909623] libertas: if_spi_c2h_cmd: err=-22

[  747.934910] libertas: lbs_spi_thread: got error -22



  And then returned a 'Failed to read scan data : Resource temporarily

unavailable' error.



* iwconfig wlan0 power off

  After the previous command, this hung with no output on the console or

log.



* iwconfig

  After displaying the 'lo' entry (with no wireless extensions) this too

hung with no output on the console or log.



I apologise for not testing this functionality of the driver, our use

case for the device doesn't include power management so I didn't test it

(to be honest, I'm not sure how our usage of the device would affect any

results wrt power management - I'm still pretty new at this whole

testing and driver development gig!)



Hope this helps, and if I can get onto a WPA2 AP I'll do some more

tests,



George.



> -----Original Message-----

> From: Andrey Yurovsky [mailto:andrey@cozybit.com]

> Sent: 29 October 2009 17:49

> To: Dan Williams

> Cc: linux-wireless@vger.kernel.org; libertas-dev@lists.infradead.org;

> sebatian@breakpoint.cc; George Shore

> Subject: Re: [PATCH] libertas: remove internal buffers from GSPI

driver

> 

> On Wed, Oct 28, 2009 at 10:13 AM, Dan Williams <dcbw@redhat.com>

wrote:

> > Looks OK to me, but I don't have any of the SPI hardware so when you

get

> > a Tested-by:, let me know and I'll ack.

> 

> Thanks Dan, by the way the issue with WPA2 and IEEE PS is unrelated to

> this patch (that is, I can reproduce it without the patch applied) so

> we'll need to address that separately.  So at this point we have:

> 

> Tested-by: George Shore <george.shore@imgtec.com>

> Signed-off-by: Andrey Yurovsky <andrey@cozybit.com>

-

This message is subject to Imagination Technologies' e-mail terms: http://www.imgtec.com/e-mail.htm



Imagination Technologies Ltd is a limited company registered in England No:  1306335 

Registered Office: Imagination House, Home Park Estate, Kings Langley, Hertfordshire, WD4 8LZ.  



Email to and from the company may be monitored for compliance and other administrative purposes.  

-

Re: [PATCH] libertas: remove internal buffers from GSPI driver

[Dan Williams]

On Thu, 2009-10-29 at 10:48 -0700, Andrey Yurovsky wrote:
> On Wed, Oct 28, 2009 at 10:13 AM, Dan Williams <dcbw@redhat.com> wrote:
> > Looks OK to me, but I don't have any of the SPI hardware so when you get
> > a Tested-by:, let me know and I'll ack.
> 
> Thanks Dan, by the way the issue with WPA2 and IEEE PS is unrelated to
> this patch (that is, I can reproduce it without the patch applied) so
> we'll need to address that separately.  So at this point we have:
> 
> Tested-by: George Shore <george.shore@imgtec.com>
> Signed-off-by: Andrey Yurovsky <andrey@cozybit.com>

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

Re: [PATCH] libertas: remove internal buffers from GSPI driver

[Colin McCabe]

On Tue, Oct 27, 2009 at 3:51 PM, Andrey Yurovsky <andrey@cozybit.com> wrote:
> This patch removes the internal command and data buffers that the GSPI driver
> maintained and instead relies on the Libertas core to synchronize access
> to the command and data ports as with the other interface drivers.

Hi all,

I'm glad to see you're improving stuff. Good catch with some of the
recent bugfixes.

However... it seems like you are headed for a "scheduling while
atomic" with this patch.

The original reason why if_spi_host_to_card copied the data into a
buffer, rather than simply calling spu_write and being done with it,
was that spu_write needed to sleep. spu_write invokes spi_sync, which
has a big comment that says "This call may only be used from a context
that may sleep."

Unfortunately, if_spi_host_to_card is called from contexts where we
hold a spinlock. For example, lbs_main does

spin_lock_irq(&priv->driver_lock);
if (!priv->dnld_sent && priv->tx_pending_len > 0) {
    int ret = priv->hw_host_to_card(priv, MVMS_DAT,

priv->tx_pending_buf,

priv->tx_pending_len);
...etc...

Perhaps the easiest thing would be to get rid of this usage of hw_host_to_card?
spu_write will always have to busy-wait from time to time because the
SPU requires a delay between successive transactions. Busy-waiting
under a spinlock, with interrupts disabled, just doesn't seem right.

Also the comment in if_spi_c2h_cmd that "we need a buffer big enough
to handle whatever people send to hw_host_to_card" should go away if
the buffer is not used in hw_host_to_card.

> This is an incremental cleanup: after removing the redundant buffers, we
> can further improve the driver to use a threaded IRQ handler instead of
> maintaining its own thread.  However I would like a few folks to test
> the buffer removal first and make sure that I'm not introducing
> regressions.

Threaded IRQs, cool. I'm waiting for that patch. :)

Colin


>
> Tested on Blackfin BF527 with DMA disabled due to an issue with the SPI
> host controller driver in the current bleeding-edge Blackfin kernel.  I
> would appreciate it if someone with working DMA could test this patch
> and provide feedback.
>
> Signed-off-by: Andrey Yurovsky <andrey@cozybit.com>
> ---
>  drivers/net/wireless/libertas/if_spi.c |  136 ++------------------------------
>  1 files changed, 6 insertions(+), 130 deletions(-)
>
> diff --git a/drivers/net/wireless/libertas/if_spi.c b/drivers/net/wireless/libertas/if_spi.c
> index 06df2e1..9e0096d 100644
> --- a/drivers/net/wireless/libertas/if_spi.c
> +++ b/drivers/net/wireless/libertas/if_spi.c
> @@ -32,12 +32,6 @@
>  #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;
> @@ -66,33 +60,10 @@ struct if_spi_card {
>        struct semaphore                spi_thread_terminated;
>
>        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;
> -
> -       BUG_ON(card->run_thread);
> -       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);
>  }
> @@ -774,40 +745,6 @@ 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)
>  {
> @@ -837,8 +774,6 @@ static int lbs_spi_thread(void *data)
>        int err;
>        struct if_spi_card *card = data;
>        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
> @@ -877,43 +812,9 @@ static int lbs_spi_thread(void *data)
>                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);
> -
> -                       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 (packet)
> -                               if_spi_h2c(card, packet, MVMS_DAT);
> -               }
>                if (hiStatus & IF_SPI_HIST_CARD_EVENT)
>                        if_spi_e2h(card);
>
> @@ -942,40 +843,18 @@ 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);
>
> -       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);
> +       nb = ALIGN(nb, 4);
>
>        switch (type) {
>        case MVMS_CMD:
> -               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);
> +               err = spu_write(card, IF_SPI_CMD_RDWRPORT_REG, buf, nb);
>                break;
>        case MVMS_DAT:
> -               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);
> +               err = spu_write(card, IF_SPI_DATA_RDWRPORT_REG, buf, nb);
>                break;
>        default:
>                lbs_pr_err("can't transfer buffer of type %d", type);
> @@ -983,9 +862,6 @@ static int if_spi_host_to_card(struct lbs_private *priv,
>                break;
>        }
>
> -       /* Wake up the spi thread */
> -       up(&card->spi_ready);
> -out:
>        lbs_deb_leave_args(LBS_DEB_SPI, "err=%d", err);
>        return err;
>  }
> @@ -1062,9 +938,6 @@ static int __devinit if_spi_probe(struct spi_device *spi)
>
>        sema_init(&card->spi_ready, 0);
>        sema_init(&card->spi_thread_terminated, 0);
> -       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 */
>        err = spu_init(card, pdata->use_dummy_writes);
> @@ -1141,6 +1014,9 @@ static int __devinit if_spi_probe(struct spi_device *spi)
>                goto terminate_thread;
>        }
>
> +       /* 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... */
> --
> 1.5.6.3
>
>
> _______________________________________________
> libertas-dev mailing list
> libertas-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/libertas-dev
>

Re: [PATCH] libertas: remove internal buffers from GSPI driver

[Dan Williams]

On Mon, 2009-11-30 at 16:53 -0800, Colin McCabe wrote:
> On Tue, Oct 27, 2009 at 3:51 PM, Andrey Yurovsky <andrey@cozybit.com> wrote:
> > This patch removes the internal command and data buffers that the GSPI driver
> > maintained and instead relies on the Libertas core to synchronize access
> > to the command and data ports as with the other interface drivers.
> 
> Hi all,
> 
> I'm glad to see you're improving stuff. Good catch with some of the
> recent bugfixes.
> 
> However... it seems like you are headed for a "scheduling while
> atomic" with this patch.
> 
> The original reason why if_spi_host_to_card copied the data into a
> buffer, rather than simply calling spu_write and being done with it,
> was that spu_write needed to sleep. spu_write invokes spi_sync, which
> has a big comment that says "This call may only be used from a context
> that may sleep."
> 
> Unfortunately, if_spi_host_to_card is called from contexts where we
> hold a spinlock. For example, lbs_main does
> 
> spin_lock_irq(&priv->driver_lock);
> if (!priv->dnld_sent && priv->tx_pending_len > 0) {
>     int ret = priv->hw_host_to_card(priv, MVMS_DAT,
> 
> priv->tx_pending_buf,
> 
> priv->tx_pending_len);
> ...etc...
> 
> Perhaps the easiest thing would be to get rid of this usage of hw_host_to_card?
> spu_write will always have to busy-wait from time to time because the
> SPU requires a delay between successive transactions. Busy-waiting
> under a spinlock, with interrupts disabled, just doesn't seem right.

I so want to kill dnld_sent.  That variable is pretty confusing and I
think it can be architected better.  That still leaves tx_pending_len;
the problem there is that we do have a TX queue and we need to protect
it against concurrent access.  Incoming packets to TX can be queued at
any time by the upper layers of the network stack.

The fix for this is to make tx_pending_buf a *pointer* to a chunk of
memory in lbs_private, say "u8 tx_buf_real[LBS_UPLD_SIZE]".  When there
is no packet to TX, tx_pending_buf points to tx_buf_real[].  Thus
incoming packets to lbs_hard_start_xmit() can be copied into that buffer
immediately, and the buffer is still protected by the lock.  When a
packet is currently being TXed, tx_pending_buf would be NULL, and
hard_start_xmit() would return NETDEV_TX_BUSY like it currently does.

When the transmit is triggered, the lock is taken, tx_pending_buf is set
to NULL, the lock is released, and the address tx_buf_real[] is sent to
hw_host_to_card().  Thus hw_host_to_card() doesn't have to be under the
lock anymore, because nothing will be writing to tx_buf_real[].

We need to keep tx_buf_real[] as a statically allocated array so that if
the module is removed during TX, we don't need to kfree() any buffers
(which the TX might be using).  Obviously we could just spin until the
TX was done and free the buffer, but it's already statically allocated
and keeping that way is just easier.

At least that's my 10 minute thought...  There might be other variables
that hw_host_to_card uses (like dnld_sent) that are protected by the
lock, but dnld_sent (and cur_cmd_retcode) needs to die die die.

Dan

> Also the comment in if_spi_c2h_cmd that "we need a buffer big enough
> to handle whatever people send to hw_host_to_card" should go away if
> the buffer is not used in hw_host_to_card.
> 
> > This is an incremental cleanup: after removing the redundant buffers, we
> > can further improve the driver to use a threaded IRQ handler instead of
> > maintaining its own thread.  However I would like a few folks to test
> > the buffer removal first and make sure that I'm not introducing
> > regressions.
> 
> Threaded IRQs, cool. I'm waiting for that patch. :)
> 
> Colin
> 
> 
> >
> > Tested on Blackfin BF527 with DMA disabled due to an issue with the SPI
> > host controller driver in the current bleeding-edge Blackfin kernel.  I
> > would appreciate it if someone with working DMA could test this patch
> > and provide feedback.
> >
> > Signed-off-by: Andrey Yurovsky <andrey@cozybit.com>
> > ---
> >  drivers/net/wireless/libertas/if_spi.c |  136 ++------------------------------
> >  1 files changed, 6 insertions(+), 130 deletions(-)
> >
> > diff --git a/drivers/net/wireless/libertas/if_spi.c b/drivers/net/wireless/libertas/if_spi.c
> > index 06df2e1..9e0096d 100644
> > --- a/drivers/net/wireless/libertas/if_spi.c
> > +++ b/drivers/net/wireless/libertas/if_spi.c
> > @@ -32,12 +32,6 @@
> >  #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;
> > @@ -66,33 +60,10 @@ struct if_spi_card {
> >        struct semaphore                spi_thread_terminated;
> >
> >        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;
> > -
> > -       BUG_ON(card->run_thread);
> > -       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);
> >  }
> > @@ -774,40 +745,6 @@ 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)
> >  {
> > @@ -837,8 +774,6 @@ static int lbs_spi_thread(void *data)
> >        int err;
> >        struct if_spi_card *card = data;
> >        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
> > @@ -877,43 +812,9 @@ static int lbs_spi_thread(void *data)
> >                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);
> > -
> > -                       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 (packet)
> > -                               if_spi_h2c(card, packet, MVMS_DAT);
> > -               }
> >                if (hiStatus & IF_SPI_HIST_CARD_EVENT)
> >                        if_spi_e2h(card);
> >
> > @@ -942,40 +843,18 @@ 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);
> >
> > -       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);
> > +       nb = ALIGN(nb, 4);
> >
> >        switch (type) {
> >        case MVMS_CMD:
> > -               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);
> > +               err = spu_write(card, IF_SPI_CMD_RDWRPORT_REG, buf, nb);
> >                break;
> >        case MVMS_DAT:
> > -               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);
> > +               err = spu_write(card, IF_SPI_DATA_RDWRPORT_REG, buf, nb);
> >                break;
> >        default:
> >                lbs_pr_err("can't transfer buffer of type %d", type);
> > @@ -983,9 +862,6 @@ static int if_spi_host_to_card(struct lbs_private *priv,
> >                break;
> >        }
> >
> > -       /* Wake up the spi thread */
> > -       up(&card->spi_ready);
> > -out:
> >        lbs_deb_leave_args(LBS_DEB_SPI, "err=%d", err);
> >        return err;
> >  }
> > @@ -1062,9 +938,6 @@ static int __devinit if_spi_probe(struct spi_device *spi)
> >
> >        sema_init(&card->spi_ready, 0);
> >        sema_init(&card->spi_thread_terminated, 0);
> > -       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 */
> >        err = spu_init(card, pdata->use_dummy_writes);
> > @@ -1141,6 +1014,9 @@ static int __devinit if_spi_probe(struct spi_device *spi)
> >                goto terminate_thread;
> >        }
> >
> > +       /* 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... */
> > --
> > 1.5.6.3
> >
> >
> > _______________________________________________
> > libertas-dev mailing list
> > libertas-dev@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/libertas-dev
> >
> 
> _______________________________________________
> libertas-dev mailing list
> libertas-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/libertas-dev

Re: [PATCH] libertas: remove internal buffers from GSPI driver

[Holger Schurig]

> Perhaps the easiest thing would be to get rid of this usage of
> hw_host_to_card? 

That would mean to rewrite this not only for SPI, but also for
USB and CS. If you come up with patches, I'm willing to test this
against USB and/or CS, in case you don't have this hardware.

-- 
http://www.holgerschurig.de

Re: [PATCH] libertas: remove internal buffers from GSPI driver

[Colin McCabe]

Yes, getting rid of the spinlock around hw_host_to_card would mean
that concurrent calls to hw_host_to_card could happen.

I don't _think_ this would be a problem in if_spi, at least. Now that
we've moved to using complete spi_message structures, each message
should be sent atomically by the SPI layer.

The only question is whether certain messages have to happen in
sequence. For example, in if_spi_c2h_cmd, we do spu_read_u16 from
scratch reg 2 to find out how many bytes to read. Then we read the
bytes from the RDWRPORT register. If someone else did some other SPU
operations between those two, would the module get confused? I don't
_think_ so, but I haven't tested it.

Colin


On Tue, Dec 1, 2009 at 1:52 AM, Holger Schurig <holgerschurig@gmail.com> wrote:
>> Perhaps the easiest thing would be to get rid of this usage of
>> hw_host_to_card?
>
> That would mean to rewrite this not only for SPI, but also for
> USB and CS. If you come up with patches, I'm willing to test this
> against USB and/or CS, in case you don't have this hardware.
>
> --
> http://www.holgerschurig.de
>