linux-kernel - RE: [patch 3/3] Add tsi108 On Chip Ethernet device driver support

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] [thread-next>] [day] [month] [year] [list]

Message-Id: <1158741958.2921.3.camel@laptopd505.fenrus.org>
Date:	Wed, 20 Sep 2006 10:45:57 +0200
From:	Arjan van de Ven <arjan@...radead.org>
To:	Zang Roy-r61911 <tie-fei.zang@...escale.com>
Cc:	Andrew Morton <akpm@...l.org>, jgarzik <jgarzik@...ox.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: RE: [patch 3/3] Add tsi108 On Chip Ethernet device driver support

On Tue, 2006-09-19 at 15:39 +0800, Zang Roy-r61911 wrote:

> > 
> > > +		spin_unlock_irq(&phy_lock);
> > > +		msleep(10);
> > > +		spin_lock_irq(&phy_lock);
> > > +	}
> > 
> > hmm some places take phy_lock with disabling interrupts, while others
> > don't. I sort of fear "the others" may be buggy.... are you sure those
> > are ok?
> Could you interpret your comments in detail?
> Roy

Hi,

sorry for being unclear/too short in the review.

The phy_lock lock is sometimes taken as spin_lock() and sometimes as
spin_lock_irq(). It looks likes it can be used in interrupt context, in
which case the spin_lock_irq() version is correct and the places where
spin_lock() is used would be a deadlock bug (just think what happens if
the interrupt happens while spin_lock(&phy_lock) is helt, and the
spinlock then again tries to take the lock!)

If there is no way this lock is used in interrupt context, then the
spin_lock_irq() version is doing something which is not needed and also
a bit expensive; so could be optimized.

But my impression is that the _irq() is needed. Also, please consider
switching from spin_lock_irq() to spin_lock_irqsave() version instead;
spin_unlock_irq() has some side effects (interrupts get enabled
unconditionally) so it is generally safer to use
spin_lock_irqsave()/spin_unlock_irqrestore() API.

If you have more questions please do not hesitate to ask!

Greetings,
   Arjan van de Ven 
-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/