netdev - Re: [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access

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: <1277426759.26161.179.camel@localhost>
Date:	Fri, 25 Jun 2010 01:45:59 +0100
From:	Ben Hutchings <ben@...adent.org.uk>
To:	Steffen Klassert <steffen.klassert@...unet.com>
Cc:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
	Chase Douglas <chase.douglas@...onical.com>,
	Arne Nordmark <nordmark@...h.kth.se>
Subject: Re: [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII
 and windowed register access

On Thu, 2010-06-24 at 16:00 +0200, Steffen Klassert wrote:
> On Thu, Jun 24, 2010 at 01:57:19PM +0100, Ben Hutchings wrote:
> > > 
> > > This adds a lot of calls to spin_lock_irqsave/spin_unlock_irqrestore to many
> > > places where this is not necessary at all. For example during device probe and
> > > device open, window_read/window_write are called multiple times, each time
> > > disabling the interrupts. I'd suggest to have unlocked, locked and irqsave
> > > versions of window_read/window_write and use them in appropriate places.
> > 
> > So what?  These are not speed-critical.  The fast-path functions do
> > acquire the lock just once.
> > 
> 
> The point is that we should not disable the interrupts if we don't need to
> do so. It is not speed critical for the 3c59x driver but disabling the
> interrupts should be avoided whenever possible. For example during device
> probe and device open we can't race against an interrupt handler because
> the device is not yet running.
> 
> An example from vortex_probe1() is:
> 
> for (i = 0; i < 6; i++)
> 	window_write8(vp, dev->dev_addr[i], 2, i);
> 
> which expands to someting like:
> 
> for (i = 0; i < 6; i++) {
> 	unsigned long flags;
> 	spin_lock_irqsave(&vp->window_lock, flags);
> 	window_set(vp, window);
> 	iowrite8(dev->dev_addr[i], vp->ioaddr  + i);
> 	spin_unlock_irqrestore(&vp->window_lock, flags);
> 	return ret;
> }
[...]

I still fail to see why this matters.

The sfc driver which I look after in my day job also uses
spin_lock_irqsave() for each CSR update, when this could be avoided in
the initialisation path.  None of the many customers using rt kernels
has ever complained about this.  It's just not an important issue.

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)