* [PATCH] p54spi: get rid of busy-wait loops
@ 2009-04-05 21:03 Max Filippov
2009-04-05 22:30 ` Christian Lamparter
0 siblings, 1 reply; 4+ messages in thread
From: Max Filippov @ 2009-04-05 21:03 UTC (permalink / raw)
To: linux-wireless; +Cc: Christian Lamparter, Max Filippov
p54spi_wakeup and p54spi_tx_frame used busy-waiting loop
to poll for 'ready' bits in SPI_ADRS_HOST_INTERRUPTS register.
With this change in place 'WR_READY timeout' messages do not
show anymore.
Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
I wish I knew for sure why. My guess is that busy-waiting
somehow interferes with SPI transfer scheduling.
drivers/net/wireless/p54/p54spi.c | 41 ++++++++++++------------------------
1 files changed, 14 insertions(+), 27 deletions(-)
diff --git a/drivers/net/wireless/p54/p54spi.c b/drivers/net/wireless/p54/p54spi.c
index 2903672..e4e708e 100644
--- a/drivers/net/wireless/p54/p54spi.c
+++ b/drivers/net/wireless/p54/p54spi.c
@@ -153,14 +153,13 @@ static const struct p54spi_spi_reg p54spi_registers_array[] =
static int p54spi_wait_bit(struct p54s_priv *priv, u16 reg, __le32 bits)
{
int i;
- __le32 buffer;
for (i = 0; i < 2000; i++) {
- p54spi_spi_read(priv, reg, &buffer, sizeof(buffer));
+ __le32 buffer = p54spi_read32(priv, reg);
if ((buffer & bits) == bits)
return 1;
- msleep(1);
+ msleep(0);
}
return 0;
}
@@ -171,10 +170,10 @@ static int p54spi_spi_write_dma(struct p54s_priv *priv, __le32 base,
p54spi_write16(priv, SPI_ADRS_DMA_WRITE_CTRL,
cpu_to_le16(SPI_DMA_WRITE_CTRL_ENABLE));
- if (p54spi_wait_bit(priv, SPI_ADRS_DMA_WRITE_CTRL,
- cpu_to_le32(HOST_ALLOWED)) == 0) {
+ if (!p54spi_wait_bit(priv, SPI_ADRS_DMA_WRITE_CTRL,
+ cpu_to_le32(HOST_ALLOWED))) {
dev_err(&priv->spi->dev, "spi_write_dma not allowed "
- "to DMA write.");
+ "to DMA write.\n");
return -EAGAIN;
}
@@ -316,21 +315,15 @@ static inline void p54spi_int_ack(struct p54s_priv *priv, u32 val)
static void p54spi_wakeup(struct p54s_priv *priv)
{
- unsigned long timeout;
- u32 ints;
-
/* wake the chip */
p54spi_write32(priv, SPI_ADRS_ARM_INTERRUPTS,
cpu_to_le32(SPI_TARGET_INT_WAKEUP));
/* And wait for the READY interrupt */
- timeout = jiffies + HZ;
-
- ints = p54spi_read32(priv, SPI_ADRS_HOST_INTERRUPTS);
- while (!(ints & SPI_HOST_INT_READY)) {
- if (time_after(jiffies, timeout))
- goto out;
- ints = p54spi_read32(priv, SPI_ADRS_HOST_INTERRUPTS);
+ if (!p54spi_wait_bit(priv, SPI_ADRS_HOST_INTERRUPTS,
+ cpu_to_le32(SPI_HOST_INT_READY))) {
+ dev_err(&priv->spi->dev, "INT_READY timeout\n");
+ goto out;
}
p54spi_int_ack(priv, SPI_HOST_INT_READY);
@@ -418,9 +411,7 @@ static irqreturn_t p54spi_interrupt(int irq, void *config)
static int p54spi_tx_frame(struct p54s_priv *priv, struct sk_buff *skb)
{
struct p54_hdr *hdr = (struct p54_hdr *) skb->data;
- unsigned long timeout;
int ret = 0;
- u32 ints;
p54spi_wakeup(priv);
@@ -428,15 +419,11 @@ static int p54spi_tx_frame(struct p54s_priv *priv, struct sk_buff *skb)
if (ret < 0)
goto out;
- timeout = jiffies + 2 * HZ;
- ints = p54spi_read32(priv, SPI_ADRS_HOST_INTERRUPTS);
- while (!(ints & SPI_HOST_INT_WR_READY)) {
- if (time_after(jiffies, timeout)) {
- dev_err(&priv->spi->dev, "WR_READY timeout\n");
- ret = -1;
- goto out;
- }
- ints = p54spi_read32(priv, SPI_ADRS_HOST_INTERRUPTS);
+ if (!p54spi_wait_bit(priv, SPI_ADRS_HOST_INTERRUPTS,
+ cpu_to_le32(SPI_HOST_INT_WR_READY))) {
+ dev_err(&priv->spi->dev, "WR_READY timeout\n");
+ ret = -1;
+ goto out;
}
p54spi_int_ack(priv, SPI_HOST_INT_WR_READY);
--
1.5.4.3
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] p54spi: get rid of busy-wait loops
2009-04-05 21:03 [PATCH] p54spi: get rid of busy-wait loops Max Filippov
@ 2009-04-05 22:30 ` Christian Lamparter
2009-04-06 5:16 ` Max Filippov
0 siblings, 1 reply; 4+ messages in thread
From: Christian Lamparter @ 2009-04-05 22:30 UTC (permalink / raw)
To: Max Filippov; +Cc: linux-wireless
On Sunday 05 April 2009 23:03:09 Max Filippov wrote:
> p54spi_wakeup and p54spi_tx_frame used busy-waiting loop
> to poll for 'ready' bits in SPI_ADRS_HOST_INTERRUPTS register.
> With this change in place 'WR_READY timeout' messages do not
> show anymore.
>
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> ---
looks good to me.
Acked-by: Christian Lamparter <chunkeey@web.de>
> I wish I knew for sure why. My guess is that busy-waiting
> somehow interferes with SPI transfer scheduling.
>
> drivers/net/wireless/p54/p54spi.c | 41 ++++++++++++------------------------
> 1 files changed, 14 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/net/wireless/p54/p54spi.c b/drivers/net/wireless/p54/p54spi.c
> index 2903672..e4e708e 100644
> --- a/drivers/net/wireless/p54/p54spi.c
> +++ b/drivers/net/wireless/p54/p54spi.c
> @@ -153,14 +153,13 @@ static const struct p54spi_spi_reg p54spi_registers_array[] =
> static int p54spi_wait_bit(struct p54s_priv *priv, u16 reg, __le32 bits)
> {
> int i;
> - __le32 buffer;
>
> for (i = 0; i < 2000; i++) {
> - p54spi_spi_read(priv, reg, &buffer, sizeof(buffer));
> + __le32 buffer = p54spi_read32(priv, reg);
> if ((buffer & bits) == bits)
> return 1;
>
> - msleep(1);
> + msleep(0);
I never liked msleep in workqueues. So instead of msleep(0),
what about replacing it with udelay or remove it entirely?
OR: maybe you can get the chip to generate IRQs when its ready,
so we don't have to poll it at all?
> }
> return 0;
> }
> @@ -171,10 +170,10 @@ static int p54spi_spi_write_dma(struct p54s_priv *priv, __le32 base,
> p54spi_write16(priv, SPI_ADRS_DMA_WRITE_CTRL,
> cpu_to_le16(SPI_DMA_WRITE_CTRL_ENABLE));
>
> - if (p54spi_wait_bit(priv, SPI_ADRS_DMA_WRITE_CTRL,
> - cpu_to_le32(HOST_ALLOWED)) == 0) {
> + if (!p54spi_wait_bit(priv, SPI_ADRS_DMA_WRITE_CTRL,
> + cpu_to_le32(HOST_ALLOWED))) {
> dev_err(&priv->spi->dev, "spi_write_dma not allowed "
> - "to DMA write.");
> + "to DMA write.\n");
> return -EAGAIN;
> }
>
> @@ -316,21 +315,15 @@ static inline void p54spi_int_ack(struct p54s_priv *priv, u32 val)
>
> static void p54spi_wakeup(struct p54s_priv *priv)
> {
> - unsigned long timeout;
> - u32 ints;
> -
> /* wake the chip */
> p54spi_write32(priv, SPI_ADRS_ARM_INTERRUPTS,
> cpu_to_le32(SPI_TARGET_INT_WAKEUP));
>
> /* And wait for the READY interrupt */
> - timeout = jiffies + HZ;
> -
> - ints = p54spi_read32(priv, SPI_ADRS_HOST_INTERRUPTS);
> - while (!(ints & SPI_HOST_INT_READY)) {
> - if (time_after(jiffies, timeout))
> - goto out;
> - ints = p54spi_read32(priv, SPI_ADRS_HOST_INTERRUPTS);
> + if (!p54spi_wait_bit(priv, SPI_ADRS_HOST_INTERRUPTS,
> + cpu_to_le32(SPI_HOST_INT_READY))) {
> + dev_err(&priv->spi->dev, "INT_READY timeout\n");
> + goto out;
> }
>
> p54spi_int_ack(priv, SPI_HOST_INT_READY);
> @@ -418,9 +411,7 @@ static irqreturn_t p54spi_interrupt(int irq, void *config)
> static int p54spi_tx_frame(struct p54s_priv *priv, struct sk_buff *skb)
> {
> struct p54_hdr *hdr = (struct p54_hdr *) skb->data;
> - unsigned long timeout;
> int ret = 0;
> - u32 ints;
>
> p54spi_wakeup(priv);
>
> @@ -428,15 +419,11 @@ static int p54spi_tx_frame(struct p54s_priv *priv, struct sk_buff *skb)
> if (ret < 0)
> goto out;
>
> - timeout = jiffies + 2 * HZ;
> - ints = p54spi_read32(priv, SPI_ADRS_HOST_INTERRUPTS);
> - while (!(ints & SPI_HOST_INT_WR_READY)) {
> - if (time_after(jiffies, timeout)) {
> - dev_err(&priv->spi->dev, "WR_READY timeout\n");
> - ret = -1;
what about replacing the generic code "-1" with EBUSY/EIO or ETIMEDOUT?
> - goto out;
> - }
> - ints = p54spi_read32(priv, SPI_ADRS_HOST_INTERRUPTS);
> + if (!p54spi_wait_bit(priv, SPI_ADRS_HOST_INTERRUPTS,
> + cpu_to_le32(SPI_HOST_INT_WR_READY))) {
> + dev_err(&priv->spi->dev, "WR_READY timeout\n");
> + ret = -1;
here too?!
> + goto out;
> }
>
> p54spi_int_ack(priv, SPI_HOST_INT_WR_READY);
---
By the way a few days ago the libertas driver authors posted a patch
which "improve the average throughput 13%."
http://marc.info/?l=linux-wireless&m=123871643203161
I wonder if this would also work with p54spi.
But to test this theory, one has to rewrite the driver to use kthread
instead of hogging workqueues.... just in case you're looking for more
TODOs ;-)
Regards,
Chr
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] p54spi: get rid of busy-wait loops
2009-04-05 22:30 ` Christian Lamparter
@ 2009-04-06 5:16 ` Max Filippov
2009-04-06 11:47 ` Christian Lamparter
0 siblings, 1 reply; 4+ messages in thread
From: Max Filippov @ 2009-04-06 5:16 UTC (permalink / raw)
To: Christian Lamparter; +Cc: linux-wireless
> On Sunday 05 April 2009 23:03:09 Max Filippov wrote:
> > p54spi_wakeup and p54spi_tx_frame used busy-waiting loop
> > to poll for 'ready' bits in SPI_ADRS_HOST_INTERRUPTS register.
> > With this change in place 'WR_READY timeout' messages do not
> > show anymore.
> >
> > Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> > ---
>
> looks good to me.
> Acked-by: Christian Lamparter <chunkeey@web.de>
>
> > I wish I knew for sure why. My guess is that busy-waiting
> > somehow interferes with SPI transfer scheduling.
> >
> > drivers/net/wireless/p54/p54spi.c | 41
> > ++++++++++++------------------------ 1 files changed, 14 insertions(+),
> > 27 deletions(-)
> >
> > diff --git a/drivers/net/wireless/p54/p54spi.c
> > b/drivers/net/wireless/p54/p54spi.c index 2903672..e4e708e 100644
> > --- a/drivers/net/wireless/p54/p54spi.c
> > +++ b/drivers/net/wireless/p54/p54spi.c
> > @@ -153,14 +153,13 @@ static const struct p54spi_spi_reg
> > p54spi_registers_array[] = static int p54spi_wait_bit(struct p54s_priv
> > *priv, u16 reg, __le32 bits) {
> > int i;
> > - __le32 buffer;
> >
> > for (i = 0; i < 2000; i++) {
> > - p54spi_spi_read(priv, reg, &buffer, sizeof(buffer));
> > + __le32 buffer = p54spi_read32(priv, reg);
> > if ((buffer & bits) == bits)
> > return 1;
> >
> > - msleep(1);
> > + msleep(0);
>
> I never liked msleep in workqueues. So instead of msleep(0),
> what about replacing it with udelay or remove it entirely?
The whole point was to delay these repeated SPI reads (:
I'm not sure if kernel scheduling is important here. Will test and repost the patch.
> OR: maybe you can get the chip to generate IRQs when its ready,
> so we don't have to poll it at all?
Will try it.
> > }
> > return 0;
> > }
> > @@ -171,10 +170,10 @@ static int p54spi_spi_write_dma(struct p54s_priv
> > *priv, __le32 base, p54spi_write16(priv, SPI_ADRS_DMA_WRITE_CTRL,
> > cpu_to_le16(SPI_DMA_WRITE_CTRL_ENABLE));
> >
> > - if (p54spi_wait_bit(priv, SPI_ADRS_DMA_WRITE_CTRL,
> > - cpu_to_le32(HOST_ALLOWED)) == 0) {
> > + if (!p54spi_wait_bit(priv, SPI_ADRS_DMA_WRITE_CTRL,
> > + cpu_to_le32(HOST_ALLOWED))) {
> > dev_err(&priv->spi->dev, "spi_write_dma not allowed "
> > - "to DMA write.");
> > + "to DMA write.\n");
> > return -EAGAIN;
> > }
> >
> > @@ -316,21 +315,15 @@ static inline void p54spi_int_ack(struct p54s_priv
> > *priv, u32 val)
> >
> > static void p54spi_wakeup(struct p54s_priv *priv)
> > {
> > - unsigned long timeout;
> > - u32 ints;
> > -
> > /* wake the chip */
> > p54spi_write32(priv, SPI_ADRS_ARM_INTERRUPTS,
> > cpu_to_le32(SPI_TARGET_INT_WAKEUP));
> >
> > /* And wait for the READY interrupt */
> > - timeout = jiffies + HZ;
> > -
> > - ints = p54spi_read32(priv, SPI_ADRS_HOST_INTERRUPTS);
> > - while (!(ints & SPI_HOST_INT_READY)) {
> > - if (time_after(jiffies, timeout))
> > - goto out;
> > - ints = p54spi_read32(priv, SPI_ADRS_HOST_INTERRUPTS);
> > + if (!p54spi_wait_bit(priv, SPI_ADRS_HOST_INTERRUPTS,
> > + cpu_to_le32(SPI_HOST_INT_READY))) {
> > + dev_err(&priv->spi->dev, "INT_READY timeout\n");
> > + goto out;
> > }
> >
> > p54spi_int_ack(priv, SPI_HOST_INT_READY);
> > @@ -418,9 +411,7 @@ static irqreturn_t p54spi_interrupt(int irq, void
> > *config) static int p54spi_tx_frame(struct p54s_priv *priv, struct
> > sk_buff *skb) {
> > struct p54_hdr *hdr = (struct p54_hdr *) skb->data;
> > - unsigned long timeout;
> > int ret = 0;
> > - u32 ints;
> >
> > p54spi_wakeup(priv);
> >
> > @@ -428,15 +419,11 @@ static int p54spi_tx_frame(struct p54s_priv *priv,
> > struct sk_buff *skb) if (ret < 0)
> > goto out;
> >
> > - timeout = jiffies + 2 * HZ;
> > - ints = p54spi_read32(priv, SPI_ADRS_HOST_INTERRUPTS);
> > - while (!(ints & SPI_HOST_INT_WR_READY)) {
> > - if (time_after(jiffies, timeout)) {
> > - dev_err(&priv->spi->dev, "WR_READY timeout\n");
> > - ret = -1;
>
> what about replacing the generic code "-1" with EBUSY/EIO or ETIMEDOUT?
>
> > - goto out;
> > - }
> > - ints = p54spi_read32(priv, SPI_ADRS_HOST_INTERRUPTS);
> > + if (!p54spi_wait_bit(priv, SPI_ADRS_HOST_INTERRUPTS,
> > + cpu_to_le32(SPI_HOST_INT_WR_READY))) {
> > + dev_err(&priv->spi->dev, "WR_READY timeout\n");
> > + ret = -1;
>
> here too?!
Will do so.
> > + goto out;
> > }
> >
> > p54spi_int_ack(priv, SPI_HOST_INT_WR_READY);
>
> ---
>
> By the way a few days ago the libertas driver authors posted a patch
> which "improve the average throughput 13%."
>
> http://marc.info/?l=linux-wireless&m=123871643203161
>
> I wonder if this would also work with p54spi.
> But to test this theory, one has to rewrite the driver to use kthread
> instead of hogging workqueues.... just in case you're looking for more
> TODOs ;-)
I'm just getting it to work. And there are still problems in tx path, e.g.
it cannot withstand dd if=/dev/zero | ssh ... "cat >/dev/null".
Sure I will revisit performance issues once they remain the most important ones.
>
> Regards,
> Chr
--
Thanks.
-- Max
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] p54spi: get rid of busy-wait loops
2009-04-06 5:16 ` Max Filippov
@ 2009-04-06 11:47 ` Christian Lamparter
0 siblings, 0 replies; 4+ messages in thread
From: Christian Lamparter @ 2009-04-06 11:47 UTC (permalink / raw)
To: Max Filippov; +Cc: linux-wireless
On Monday 06 April 2009 07:16:02 Max Filippov wrote:
> >
> > By the way a few days ago the libertas driver authors posted a patch
> > which "improve the average throughput 13%."
> >
> > http://marc.info/?l=linux-wireless&m=123871643203161
> >
> > I wonder if this would also work with p54spi.
> > But to test this theory, one has to rewrite the driver to use kthread
> > instead of hogging workqueues.... just in case you're looking for more
> > TODOs ;-)
> I'm just getting it to work. And there are still problems in tx path, e.g.
> it cannot withstand dd if=/dev/zero | ssh ... "cat >/dev/null".
hmm, sounds familiar.
(" p54usb: fix random traffic stalls (LM87)" (43af18f06d5
and p54usb: fix packet loss with first generation devices ")
can you please run
for ((size=0; size < 9000;size++)); do ping $IP -s $size
and see if there's a connection between the stalls/hangs and the frame length?
Regards,
Chr
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2009-04-06 11:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-04-05 21:03 [PATCH] p54spi: get rid of busy-wait loops Max Filippov
2009-04-05 22:30 ` Christian Lamparter
2009-04-06 5:16 ` Max Filippov
2009-04-06 11:47 ` Christian Lamparter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).