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]

Date:	Fri, 25 Jun 2010 10:24:47 +0200
From:	Steffen Klassert <steffen.klassert@...unet.com>
To:	Ben Hutchings <ben@...adent.org.uk>
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 Fri, Jun 25, 2010 at 01:45:59AM +0100, Ben Hutchings wrote:
> > 
> > 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.
> 

These locks are not needed, why you want to have them arround?
It adds overhead, even if they are not in a fast-path function.
And much more important, this gives a reader of the code the
impression that these locks are needed. For somebody who tries
to understand the code, it will be probaply not that easy to
figure out which of these locks are needed and for what reason.

> 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.

Perhaps I'm fussy, personally I prefer to use the appropriate
lock in any case. But that's just my opinion, other people might
think different about that.

Steffen
--
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