diff mbox

[PATCHv4,net-next,05/32] cxgb4: use spinlock_irqsave/spinlock_irqrestore for db lock

Message ID 1394188409-9739-6-git-send-email-hariprasad@chelsio.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Hariprasad Shenai March 7, 2014, 10:33 a.m. UTC 
From: Kumar Sanghvi <kumaras@chelsio.com>

Currently ring_tx_db() can deadlock if a db_full interrupt fires and is
run on the same while ring_tx_db() has the db lock held.  It needs to
disable interrupts since it serializes with an interrupt handler.

Based on original work by Steve Wise <swise@opengridcomputing.com>

Signed-off-by: Kumar Sanghvi <kumaras@chelsio.com>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 11 +++++++----
 drivers/net/ethernet/chelsio/cxgb4/sge.c        |  5 +++--
 2 files changed, 10 insertions(+), 6 deletions(-)

Comments

David Miller March 8, 2014, 5:43 a.m. UTC | #1 
From: Hariprasad Shenai <hariprasad@chelsio.com>
Date: Fri,  7 Mar 2014 16:03:02 +0530

> @@ -3585,9 +3585,11 @@ static void disable_txq_db(struct sge_txq *q)
>  
>  static void enable_txq_db(struct sge_txq *q)
>  {
> -	spin_lock_irq(&q->db_lock);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&q->db_lock, flags);
>  	q->db_disabled = 0;
> -	spin_unlock_irq(&q->db_lock);
> +	spin_unlock_irqrestore(&q->db_lock, flags);
>  }
>  
>  static void disable_dbs(struct adapter *adap)
> @@ -3617,9 +3619,10 @@ static void enable_dbs(struct adapter *adap)

At least be consistent when making changes like this.

You are changing from spin_{un,}lock_irq() to
spin_{un,}lock_irq{save,restore}() here in enable_txq_db()
but not in disable_txq_db().

But both of those functions are invoked, via one level of
indirection, from the same exact function: process_db_drop()

Futhermore, this function process_db_drop() runs via a workqueue, and
therefore always runs with interrupts enabled.  So you shouldn't need
to use the save/restore spinlock variants at all.  Plain
spin_lock_irq() and spin_unlock_irq(), as is currently coded, is
perfectly fine.
--
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
diff mbox

Patch

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index 0ac53dd..a95668e 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -3585,9 +3585,11 @@  static void disable_txq_db(struct sge_txq *q)
 
 static void enable_txq_db(struct sge_txq *q)
 {
-	spin_lock_irq(&q->db_lock);
+	unsigned long flags;
+
+	spin_lock_irqsave(&q->db_lock, flags);
 	q->db_disabled = 0;
-	spin_unlock_irq(&q->db_lock);
+	spin_unlock_irqrestore(&q->db_lock, flags);
 }
 
 static void disable_dbs(struct adapter *adap)
@@ -3617,9 +3619,10 @@  static void enable_dbs(struct adapter *adap)
 static void sync_txq_pidx(struct adapter *adap, struct sge_txq *q)
 {
 	u16 hw_pidx, hw_cidx;
+	unsigned long flags;
 	int ret;
 
-	spin_lock_bh(&q->db_lock);
+	spin_lock_irqsave(&q->db_lock, flags);
 	ret = read_eq_indices(adap, (u16)q->cntxt_id, &hw_pidx, &hw_cidx);
 	if (ret)
 		goto out;
@@ -3636,7 +3639,7 @@  static void sync_txq_pidx(struct adapter *adap, struct sge_txq *q)
 	}
 out:
 	q->db_disabled = 0;
-	spin_unlock_bh(&q->db_lock);
+	spin_unlock_irqrestore(&q->db_lock, flags);
 	if (ret)
 		CH_WARN(adap, "DB drop recovery failed.\n");
 }
diff --git a/drivers/net/ethernet/chelsio/cxgb4/sge.c b/drivers/net/ethernet/chelsio/cxgb4/sge.c
index a7c56b3..c6fa96a 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/sge.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/sge.c
@@ -860,9 +860,10 @@  static void cxgb_pio_copy(u64 __iomem *dst, u64 *src)
 static inline void ring_tx_db(struct adapter *adap, struct sge_txq *q, int n)
 {
 	unsigned int *wr, index;
+	unsigned long flags;
 
 	wmb();            /* write descriptors before telling HW */
-	spin_lock(&q->db_lock);
+	spin_lock_irqsave(&q->db_lock, flags);
 	if (!q->db_disabled) {
 		if (is_t4(adap->params.chip)) {
 			t4_write_reg(adap, MYPF_REG(SGE_PF_KDOORBELL),
@@ -880,7 +881,7 @@  static inline void ring_tx_db(struct adapter *adap, struct sge_txq *q, int n)
 		}
 	}
 	q->db_pidx = q->pidx;
-	spin_unlock(&q->db_lock);
+	spin_unlock_irqrestore(&q->db_lock, flags);
 }
 
 /**