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

From: Ben Hutchings <[email-masked]>
Date: Fri, 25 Jun 2010 01:45:59 +0100

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.