[PATCH 0/3]: via-velocity: Fixes for locking issues

[PATCH 0/3]: via-velocity: Fixes for locking issues

[Simon Kagstrom]

Hi!

This series of patches fixes a set of locking issues in the
via-velocity driver. Since some of the problems were introduced in
2.6.33-rc1 (with my patches, sorry!), it would be good if they could
get in before the final 2.6.33 release.

The patches are (the third one is the complex one):

  * A small cleanup to remove unused parameters for rx_srv and tx_srv

  * Take the spinlock from set_coalesce

  * Fix races that occur with shared interrupts

We have a system where the interrupt for the VIA card is shared with a
USB controller. The last patch fixes a deadlock situation, which we can
trigger on our UP with spinlock debugging, where the VIA driver is
executing in velocity_poll (the NAPI poll callback) when the USB
controller interrupts the CPU. Since the interrupt controller takes the
(already held) lock, it will deadlock.


Some questions about the implementation though:

The patch uses spin_trylock in the interrupt handler, and returns
IRQ_NONE if it's already held. Here is a place where I'm unsure about
the right way though. Another alternative is to use spin_lock_irqsave
in velocity_poll(), but I'd like to avoid turning the interrupts off
for "long" periods of time when the actual velocity interrupt can't
happen anyway (since it is turned off while executing velocity_poll().

With spin_trylock and spinlock debugging on our UP, we get a bug from
the debugging code:

  BUG: spinlock trylock failure on UP on CPU#0, swapper/0

which looking at the debug code isn't unexpected since spin_trylock is
called from an interrupt handler. In this case though, I don't think
this is an actual error.


We don't have access to any multiprocessor machine with a VIA NIC (I
guess they are not very common), so I haven't been able to test these
patches on actual SMP hardware.

The locking in via-velocity in general would be good to look over, I
think.

// Simon

[PATCH 1/3] via-velocity: Remove unused IRQ status parameter from rx_srv and tx_srv

[Simon Kagstrom]

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/net/via-velocity.c |   17 +++++++----------
 1 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/net/via-velocity.c b/drivers/net/via-velocity.c
index c963277..5c73673 100644
--- a/drivers/net/via-velocity.c
+++ b/drivers/net/via-velocity.c
@@ -1877,13 +1877,12 @@ static void velocity_error(struct velocity_info *vptr, int status)
 /**
  *	tx_srv		-	transmit interrupt service
  *	@vptr; Velocity
- *	@status:
  *
  *	Scan the queues looking for transmitted packets that
  *	we can complete and clean up. Update any statistics as
  *	necessary/
  */
-static int velocity_tx_srv(struct velocity_info *vptr, u32 status)
+static int velocity_tx_srv(struct velocity_info *vptr)
 {
 	struct tx_desc *td;
 	int qnum;
@@ -2090,14 +2089,12 @@ static int velocity_receive_frame(struct velocity_info *vptr, int idx)
 /**
  *	velocity_rx_srv		-	service RX interrupt
  *	@vptr: velocity
- *	@status: adapter status (unused)
  *
  *	Walk the receive ring of the velocity adapter and remove
  *	any received packets from the receive queue. Hand the ring
  *	slots back to the adapter for reuse.
  */
-static int velocity_rx_srv(struct velocity_info *vptr, int status,
-		int budget_left)
+static int velocity_rx_srv(struct velocity_info *vptr, int budget_left)
 {
 	struct net_device_stats *stats = &vptr->dev->stats;
 	int rd_curr = vptr->rx.curr;
@@ -2165,10 +2162,10 @@ static int velocity_poll(struct napi_struct *napi, int budget)
 	 * Do rx and tx twice for performance (taken from the VIA
 	 * out-of-tree driver).
 	 */
-	rx_done = velocity_rx_srv(vptr, isr_status, budget / 2);
-	velocity_tx_srv(vptr, isr_status);
-	rx_done += velocity_rx_srv(vptr, isr_status, budget - rx_done);
-	velocity_tx_srv(vptr, isr_status);
+	rx_done = velocity_rx_srv(vptr, budget / 2);
+	velocity_tx_srv(vptr);
+	rx_done += velocity_rx_srv(vptr, budget - rx_done);
+	velocity_tx_srv(vptr);
 
 	spin_unlock(&vptr->lock);
 
@@ -3100,7 +3097,7 @@ static int velocity_resume(struct pci_dev *pdev)
 	velocity_init_registers(vptr, VELOCITY_INIT_WOL);
 	mac_disable_int(vptr->mac_regs);
 
-	velocity_tx_srv(vptr, 0);
+	velocity_tx_srv(vptr);
 
 	for (i = 0; i < vptr->tx.numq; i++) {
 		if (vptr->tx.used[i])
-- 
1.6.0.4

[PATCH 2/3] via-velocity: Take spinlock on set coalesce

[Simon Kagstrom]

velocity_set_coalesce touches ISR and some other sensitive registers not
covered by the rtnl lock, so take the velocity spinlock.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/net/via-velocity.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/via-velocity.c b/drivers/net/via-velocity.c
index 5c73673..5e213f7 100644
--- a/drivers/net/via-velocity.c
+++ b/drivers/net/via-velocity.c
@@ -3341,6 +3341,7 @@ static int velocity_set_coalesce(struct net_device *dev,
 {
 	struct velocity_info *vptr = netdev_priv(dev);
 	int max_us = 0x3f * 64;
+	unsigned long flags;
 
 	/* 6 bits of  */
 	if (ecmd->tx_coalesce_usecs > max_us)
@@ -3362,6 +3363,7 @@ static int velocity_set_coalesce(struct net_device *dev,
 			ecmd->tx_coalesce_usecs);
 
 	/* Setup the interrupt suppression and queue timers */
+	spin_lock_irqsave(&vptr->lock, flags);
 	mac_disable_int(vptr->mac_regs);
 	setup_adaptive_interrupts(vptr);
 	setup_queue_timers(vptr);
@@ -3369,6 +3371,7 @@ static int velocity_set_coalesce(struct net_device *dev,
 	mac_write_int_mask(vptr->int_mask, vptr->mac_regs);
 	mac_clear_isr(vptr->mac_regs);
 	mac_enable_int(vptr->mac_regs);
+	spin_unlock_irqrestore(&vptr->lock, flags);
 
 	return 0;
 }
-- 
1.6.0.4

[PATCH 3/3] via-velocity: Fix races on shared interrupts

[Simon Kagstrom]

This patch fixes two potential races in the velocity driver:

* Move the ACK and error handler to the interrupt handler. This fixes a
  potential race with shared interrupts when the other device interrupts
  before the NAPI poll handler has finished. As the velocity driver hasn't
  acked it's own interrupt, it will then steal the interrupt from the
  other device.

* Use spin_trylock in the interrupt handler. To avoid having the
  interrupt off for long periods of time, velocity_poll uses non-irqsave
  spinlocks. In the current code, the interrupt handler will deadlock if
  e.g., the NAPI poll handler is executing when an interrupt (for another
  device) comes in since it tries to take the already held lock.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
Signed-off-by: Anders Grafstrom <anders.grafstrom@netinsight.net>
---
 drivers/net/via-velocity.c |   26 +++++++++++++++++---------
 1 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/net/via-velocity.c b/drivers/net/via-velocity.c
index 5e213f7..6882e7c 100644
--- a/drivers/net/via-velocity.c
+++ b/drivers/net/via-velocity.c
@@ -2148,16 +2148,8 @@ static int velocity_poll(struct napi_struct *napi, int budget)
 	struct velocity_info *vptr = container_of(napi,
 			struct velocity_info, napi);
 	unsigned int rx_done;
-	u32 isr_status;
 
 	spin_lock(&vptr->lock);
-	isr_status = mac_read_isr(vptr->mac_regs);
-
-	/* Ack the interrupt */
-	mac_write_isr(vptr->mac_regs, isr_status);
-	if (isr_status & (~(ISR_PRXI | ISR_PPRXI | ISR_PTXI | ISR_PPTXI)))
-		velocity_error(vptr, isr_status);
-
 	/*
 	 * Do rx and tx twice for performance (taken from the VIA
 	 * out-of-tree driver).
@@ -2194,7 +2186,16 @@ static irqreturn_t velocity_intr(int irq, void *dev_instance)
 	struct velocity_info *vptr = netdev_priv(dev);
 	u32 isr_status;
 
-	spin_lock(&vptr->lock);
+	/* Check if the lock is taken, and if so ignore the interrupt. This
+	 * can happen with shared interrupts, where the other device can
+	 * interrupt during velocity_poll (where the lock is held).
+	 *
+	 * With spinlock debugging active on a uniprocessor, this will give
+	 * a warning which can safely be ignored.
+	 */
+	if (!spin_trylock(&vptr->lock))
+		return IRQ_NONE;
+
 	isr_status = mac_read_isr(vptr->mac_regs);
 
 	/* Not us ? */
@@ -2203,10 +2204,17 @@ static irqreturn_t velocity_intr(int irq, void *dev_instance)
 		return IRQ_NONE;
 	}
 
+	/* Ack the interrupt */
+	mac_write_isr(vptr->mac_regs, isr_status);
+
 	if (likely(napi_schedule_prep(&vptr->napi))) {
 		mac_disable_int(vptr->mac_regs);
 		__napi_schedule(&vptr->napi);
 	}
+
+	if (isr_status & (~(ISR_PRXI | ISR_PPRXI | ISR_PTXI | ISR_PPTXI)))
+		velocity_error(vptr, isr_status);
+
 	spin_unlock(&vptr->lock);
 
 	return IRQ_HANDLED;
-- 
1.6.0.4

Re: [PATCH 0/3]: via-velocity: Fixes for locking issues

[David Miller]

From: Simon Kagstrom <simon.kagstrom@netinsight.net>
Date: Fri, 5 Feb 2010 16:52:53 +0100

> The patch uses spin_trylock in the interrupt handler, and returns
> IRQ_NONE if it's already held. Here is a place where I'm unsure about
> the right way though. Another alternative is to use spin_lock_irqsave
> in velocity_poll(), but I'd like to avoid turning the interrupts off
> for "long" periods of time when the actual velocity interrupt can't
> happen anyway (since it is turned off while executing velocity_poll().

It can happen in this situation and this is not the right way to
handle this.

You will lose interrupt events if the situation is that the interrupt
for the VIA has migrated from one cpu to another and another cpu is in
the VIA interrupt handler when we see the lock held.

Consider also the case where another cpu has the VIA lock as
a result of a device sharing the IRQ with VIA firing.

You'll lose events and jam the chip in that case too.

What this comes down to is not using the lock correctly, you
can't paper around it with this trylock stuff.

The proper thing to do is make the ->poll() handler use the lock
correctly by disabling interrupts.

If the VIA driver wants to avoid holding interrupts disabled for long
periods of time in ->poll(), it's locking mechanisms will need to be
rewritten completely.  The only safe way to do this is to do something
like how the tg3 driver works, wherein the interrupt handler runs
lockless, there is a special IRQ quiesce sequence when shutting down
the chip, and the ->poll() handler et al. use softirq safe locks
instead of hardirq safe ones.

For now you're going to have to accept interrupts being disabled in
order to fix this bug for the time being.

Please resubmit this patch series once you've worked this out.

Thanks!

Re: [PATCH 0/3]: via-velocity: Fixes for locking issues

[Simon Kagstrom]

Hi David!

Thanks for the comments, I understand the problem more clearly now.

On Tue, 09 Feb 2010 16:31:33 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> The proper thing to do is make the ->poll() handler use the lock
> correctly by disabling interrupts.

In reply to this message is an updated patch series which uses
spin_lock_irqsave/restore in velocity_poll instead.

> If the VIA driver wants to avoid holding interrupts disabled for long
> periods of time in ->poll(), it's locking mechanisms will need to be
> rewritten completely.  The only safe way to do this is to do something
> like how the tg3 driver works, wherein the interrupt handler runs
> lockless, there is a special IRQ quiesce sequence when shutting down
> the chip, and the ->poll() handler et al. use softirq safe locks
> instead of hardirq safe ones.

Yes, I guess that would be the long-term solution for this driver. The
main problem with running the interrupt handler unlocked now is the
error handler, which is executed directly from the interrupt handler.


Anyway, when you are happy with the patch series, I think it is 2.6.33
material as the velocity driver would otherwise contain a regression
compared to 2.6.32.

Changes in the patch series:

* Patch 1: Unchanged

* Patch 2: Unchanged

* Patch 3: Use spin_lock_irqsave in velocity_poll and remove
  spin_trylock from the interrupt handler.


As before, I'm only able to test it on a uniprocessor with spinlock
debugging turned on. With the patch set, I can no longer get it to
lockup when running heavy traffic for both interrupt sources (which was
easy without the patches).

Thanks,
// Simon

[PATCH v2 1/3] via-velocity: Remove unused IRQ status parameter from rx_srv and tx_srv

[Simon Kagstrom]

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/net/via-velocity.c |   17 +++++++----------
 1 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/net/via-velocity.c b/drivers/net/via-velocity.c
index c963277..5c73673 100644
--- a/drivers/net/via-velocity.c
+++ b/drivers/net/via-velocity.c
@@ -1877,13 +1877,12 @@ static void velocity_error(struct velocity_info *vptr, int status)
 /**
  *	tx_srv		-	transmit interrupt service
  *	@vptr; Velocity
- *	@status:
  *
  *	Scan the queues looking for transmitted packets that
  *	we can complete and clean up. Update any statistics as
  *	necessary/
  */
-static int velocity_tx_srv(struct velocity_info *vptr, u32 status)
+static int velocity_tx_srv(struct velocity_info *vptr)
 {
 	struct tx_desc *td;
 	int qnum;
@@ -2090,14 +2089,12 @@ static int velocity_receive_frame(struct velocity_info *vptr, int idx)
 /**
  *	velocity_rx_srv		-	service RX interrupt
  *	@vptr: velocity
- *	@status: adapter status (unused)
  *
  *	Walk the receive ring of the velocity adapter and remove
  *	any received packets from the receive queue. Hand the ring
  *	slots back to the adapter for reuse.
  */
-static int velocity_rx_srv(struct velocity_info *vptr, int status,
-		int budget_left)
+static int velocity_rx_srv(struct velocity_info *vptr, int budget_left)
 {
 	struct net_device_stats *stats = &vptr->dev->stats;
 	int rd_curr = vptr->rx.curr;
@@ -2165,10 +2162,10 @@ static int velocity_poll(struct napi_struct *napi, int budget)
 	 * Do rx and tx twice for performance (taken from the VIA
 	 * out-of-tree driver).
 	 */
-	rx_done = velocity_rx_srv(vptr, isr_status, budget / 2);
-	velocity_tx_srv(vptr, isr_status);
-	rx_done += velocity_rx_srv(vptr, isr_status, budget - rx_done);
-	velocity_tx_srv(vptr, isr_status);
+	rx_done = velocity_rx_srv(vptr, budget / 2);
+	velocity_tx_srv(vptr);
+	rx_done += velocity_rx_srv(vptr, budget - rx_done);
+	velocity_tx_srv(vptr);
 
 	spin_unlock(&vptr->lock);
 
@@ -3100,7 +3097,7 @@ static int velocity_resume(struct pci_dev *pdev)
 	velocity_init_registers(vptr, VELOCITY_INIT_WOL);
 	mac_disable_int(vptr->mac_regs);
 
-	velocity_tx_srv(vptr, 0);
+	velocity_tx_srv(vptr);
 
 	for (i = 0; i < vptr->tx.numq; i++) {
 		if (vptr->tx.used[i])
-- 
1.6.0.4

[PATCH v2 2/3] via-velocity: Take spinlock on set coalesce

[Simon Kagstrom]

velocity_set_coalesce touches ISR and some other sensitive registers not
covered by the rtnl lock, so take the velocity spinlock.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/net/via-velocity.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/via-velocity.c b/drivers/net/via-velocity.c
index 5c73673..5e213f7 100644
--- a/drivers/net/via-velocity.c
+++ b/drivers/net/via-velocity.c
@@ -3341,6 +3341,7 @@ static int velocity_set_coalesce(struct net_device *dev,
 {
 	struct velocity_info *vptr = netdev_priv(dev);
 	int max_us = 0x3f * 64;
+	unsigned long flags;
 
 	/* 6 bits of  */
 	if (ecmd->tx_coalesce_usecs > max_us)
@@ -3362,6 +3363,7 @@ static int velocity_set_coalesce(struct net_device *dev,
 			ecmd->tx_coalesce_usecs);
 
 	/* Setup the interrupt suppression and queue timers */
+	spin_lock_irqsave(&vptr->lock, flags);
 	mac_disable_int(vptr->mac_regs);
 	setup_adaptive_interrupts(vptr);
 	setup_queue_timers(vptr);
@@ -3369,6 +3371,7 @@ static int velocity_set_coalesce(struct net_device *dev,
 	mac_write_int_mask(vptr->int_mask, vptr->mac_regs);
 	mac_clear_isr(vptr->mac_regs);
 	mac_enable_int(vptr->mac_regs);
+	spin_unlock_irqrestore(&vptr->lock, flags);
 
 	return 0;
 }
-- 
1.6.0.4

[PATCH v2 3/3] via-velocity: Fix races on shared interrupts

[Simon Kagstrom]

This patch fixes two potential races in the velocity driver:

* Move the ACK and error handler to the interrupt handler. This fixes a
  potential race with shared interrupts when the other device interrupts
  before the NAPI poll handler has finished. As the velocity driver hasn't
  acked it's own interrupt, it will then steal the interrupt from the
  other device.

* Use spin_lock_irqsave in velocity_poll. In the current code, the
  interrupt handler will deadlock if e.g., the NAPI poll handler is
  executing when an interrupt (for another device) comes in since it
  tries to take the already held lock.

  Also unlock the spinlock only after enabling the interrupt in
  velocity_poll.

The error path is moved to the interrupt handler since this is where the
ISR is checked now.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
Signed-off-by: Anders Grafstrom <anders.grafstrom@netinsight.net>
---
ChangeLog:
	* (David Miller): Remove spin_trylock from the interrupt handler
	* (David Miller): Use spin_lock_irqsave in velocity_poll
	* Move spin_unlock_irqrestore below mac_enable_int in velocity_poll

 drivers/net/via-velocity.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/net/via-velocity.c b/drivers/net/via-velocity.c
index 5e213f7..18770ac 100644
--- a/drivers/net/via-velocity.c
+++ b/drivers/net/via-velocity.c
@@ -2148,16 +2148,9 @@ static int velocity_poll(struct napi_struct *napi, int budget)
 	struct velocity_info *vptr = container_of(napi,
 			struct velocity_info, napi);
 	unsigned int rx_done;
-	u32 isr_status;
-
-	spin_lock(&vptr->lock);
-	isr_status = mac_read_isr(vptr->mac_regs);
-
-	/* Ack the interrupt */
-	mac_write_isr(vptr->mac_regs, isr_status);
-	if (isr_status & (~(ISR_PRXI | ISR_PPRXI | ISR_PTXI | ISR_PPTXI)))
-		velocity_error(vptr, isr_status);
+	unsigned long flags;
 
+	spin_lock_irqsave(&vptr->lock, flags);
 	/*
 	 * Do rx and tx twice for performance (taken from the VIA
 	 * out-of-tree driver).
@@ -2167,13 +2160,12 @@ static int velocity_poll(struct napi_struct *napi, int budget)
 	rx_done += velocity_rx_srv(vptr, budget - rx_done);
 	velocity_tx_srv(vptr);
 
-	spin_unlock(&vptr->lock);
-
 	/* If budget not fully consumed, exit the polling mode */
 	if (rx_done < budget) {
 		napi_complete(napi);
 		mac_enable_int(vptr->mac_regs);
 	}
+	spin_unlock_irqrestore(&vptr->lock, flags);
 
 	return rx_done;
 }
@@ -2203,10 +2195,17 @@ static irqreturn_t velocity_intr(int irq, void *dev_instance)
 		return IRQ_NONE;
 	}
 
+	/* Ack the interrupt */
+	mac_write_isr(vptr->mac_regs, isr_status);
+
 	if (likely(napi_schedule_prep(&vptr->napi))) {
 		mac_disable_int(vptr->mac_regs);
 		__napi_schedule(&vptr->napi);
 	}
+
+	if (isr_status & (~(ISR_PRXI | ISR_PPRXI | ISR_PTXI | ISR_PPTXI)))
+		velocity_error(vptr, isr_status);
+
 	spin_unlock(&vptr->lock);
 
 	return IRQ_HANDLED;
-- 
1.6.0.4

Re: [PATCH 3/3] via-velocity: Fix races on shared interrupts

[Laurent Chavey]

On Fri, Feb 5, 2010 at 7:55 AM, Simon Kagstrom
<simon.kagstrom@netinsight.net> wrote:
> This patch fixes two potential races in the velocity driver:
>
> * Move the ACK and error handler to the interrupt handler. This fixes a
>  potential race with shared interrupts when the other device interrupts
>  before the NAPI poll handler has finished. As the velocity driver hasn't
>  acked it's own interrupt, it will then steal the interrupt from the
>  other device.
>
> * Use spin_trylock in the interrupt handler. To avoid having the
>  interrupt off for long periods of time, velocity_poll uses non-irqsave
>  spinlocks. In the current code, the interrupt handler will deadlock if
>  e.g., the NAPI poll handler is executing when an interrupt (for another
>  device) comes in since it tries to take the already held lock.
>
> Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
> Signed-off-by: Anders Grafstrom <anders.grafstrom@netinsight.net>
> ---
>  drivers/net/via-velocity.c |   26 +++++++++++++++++---------
>  1 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/via-velocity.c b/drivers/net/via-velocity.c
> index 5e213f7..6882e7c 100644
> --- a/drivers/net/via-velocity.c
> +++ b/drivers/net/via-velocity.c
> @@ -2148,16 +2148,8 @@ static int velocity_poll(struct napi_struct *napi, int budget)
>        struct velocity_info *vptr = container_of(napi,
>                        struct velocity_info, napi);
>        unsigned int rx_done;
> -       u32 isr_status;
>
>        spin_lock(&vptr->lock);
> -       isr_status = mac_read_isr(vptr->mac_regs);
> -
> -       /* Ack the interrupt */
> -       mac_write_isr(vptr->mac_regs, isr_status);
> -       if (isr_status & (~(ISR_PRXI | ISR_PPRXI | ISR_PTXI | ISR_PPTXI)))
> -               velocity_error(vptr, isr_status);
> -
>        /*
>         * Do rx and tx twice for performance (taken from the VIA
>         * out-of-tree driver).
> @@ -2194,7 +2186,16 @@ static irqreturn_t velocity_intr(int irq, void *dev_instance)
>        struct velocity_info *vptr = netdev_priv(dev);
>        u32 isr_status;
>
> -       spin_lock(&vptr->lock);
> +       /* Check if the lock is taken, and if so ignore the interrupt. This
> +        * can happen with shared interrupts, where the other device can
> +        * interrupt during velocity_poll (where the lock is held).
> +        *
> +        * With spinlock debugging active on a uniprocessor, this will give
> +        * a warning which can safely be ignored.
> +        */
> +       if (!spin_trylock(&vptr->lock))
> +               return IRQ_NONE;

does the thread handling the interrupts check that an new
interrupts was received while it was servicing a previous one ?
wondering if there is a potential for an event that generates the interrupt
to be missed.


> +
>        isr_status = mac_read_isr(vptr->mac_regs);
>
>        /* Not us ? */
> @@ -2203,10 +2204,17 @@ static irqreturn_t velocity_intr(int irq, void *dev_instance)
>                return IRQ_NONE;
>        }
>
> +       /* Ack the interrupt */
> +       mac_write_isr(vptr->mac_regs, isr_status);
> +
>        if (likely(napi_schedule_prep(&vptr->napi))) {
>                mac_disable_int(vptr->mac_regs);
>                __napi_schedule(&vptr->napi);
>        }
> +
> +       if (isr_status & (~(ISR_PRXI | ISR_PPRXI | ISR_PTXI | ISR_PPTXI)))
> +               velocity_error(vptr, isr_status);
> +
>        spin_unlock(&vptr->lock);
>
>        return IRQ_HANDLED;
> --
> 1.6.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" 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]: via-velocity: Fixes for locking issues

[David Miller]

From: Simon Kagstrom <simon.kagstrom@netinsight.net>
Date: Wed, 10 Feb 2010 10:33:58 +0100

> Anyway, when you are happy with the patch series, I think it is 2.6.33
> material as the velocity driver would otherwise contain a regression
> compared to 2.6.32.
> 
> Changes in the patch series:
> 
> * Patch 1: Unchanged
> 
> * Patch 2: Unchanged
> 
> * Patch 3: Use spin_lock_irqsave in velocity_poll and remove
>   spin_trylock from the interrupt handler.

Looks good, all applied to net-2.6, thanks!

Re: [PATCH 3/3] via-velocity: Fix races on shared interrupts

[Simon Kagstrom]

Hi Laurent!

On Wed, 10 Feb 2010 09:41:59 -0800
Laurent Chavey <chavey@google.com> wrote:

> > -       spin_lock(&vptr->lock);
> > +       /* Check if the lock is taken, and if so ignore the interrupt. This
> > +        * can happen with shared interrupts, where the other device can
> > +        * interrupt during velocity_poll (where the lock is held).
> > +        *
> > +        * With spinlock debugging active on a uniprocessor, this will give
> > +        * a warning which can safely be ignored.
> > +        */
> > +       if (!spin_trylock(&vptr->lock))
> > +               return IRQ_NONE;
> 
> does the thread handling the interrupts check that an new
> interrupts was received while it was servicing a previous one ?
> wondering if there is a potential for an event that generates the interrupt
> to be missed.

I should say that this particular part of the patch was reworked in
version 2, which David took in here:
  
  http://git.kernel.org/?p=linux/kernel/git/davem/net-2.6.git;a=commitdiff;h=3f2e8d9f13246382fbda6f03178eef867a9bfbe2

Anyway, velocity_poll will try to empty all events within it's budget
from the device and is executing with the device interrupt turned off
(and now also with the local processor interrupts off). If something
would be posted when it's exiting, that's fine since it either

  1) Consumed it's entire budget, in which case it will stay in the
  polling mode anyway

or,

  2) Didn't consume the budget and will then turn on the interrupt
  again and get the new event promptly.


You are right about the code above though, that one is racy as David
explained here:

  http://permalink.gmane.org/gmane.linux.network/151578

// Simon