netdev - Re: [PATCHv4 net-next 05/32] cxgb4: use spinlock_irqsave/spinlock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20140308.004312.1333337731427216882.davem@davemloft.net>
Date:	Sat, 08 Mar 2014 00:43:12 -0500 (EST)
From:	David Miller <davem@...emloft.net>
To:	hariprasad@...lsio.com
Cc:	netdev@...r.kernel.org, linux-rdma@...r.kernel.org,
	roland@...estorage.com, santosh@...lsio.com, dm@...lsio.com,
	kumaras@...lsio.com, swise@...ngridcomputing.com,
	leedom@...lsio.com, nirranjan@...lsio.com
Subject: Re: [PATCHv4 net-next 05/32] cxgb4: use
 spinlock_irqsave/spinlock_irqrestore for db lock

From: Hariprasad Shenai <hariprasad@...lsio.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html