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  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]
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)

Powered by blists - more mailing lists