Re: [PATCH] PA Semi PWRficient Ethernet driver

[olof@lixom.net (Olof Johansson)]

On Tue, Jan 30, 2007 at 10:45:18PM +0100, Francois Romieu wrote:
> Olof Johansson <olof@lixom.net> :
> > On Mon, Jan 29, 2007 at 11:35:06PM +0100, Francois Romieu wrote:
> [...]
> > > - The driver does not contain a single SMP locking instruction but
> > >   http://www.pasemi.com claims the platform to be multicore.
> > >   Is the driver really designed to be lockless ?
> > 
> > Unless I misunderstood something, NAPI drivers that don't set NETIF_F_LLTX
> > will have all locking taken care of by higher layers, no?
> 
> It is not necessarily _that_ simple (it would be cool though :o) ).
> 
> For instance, what does prevent pasemi_mac_clean_tx() to be issued
> from IRQ context (pasemi_mac_tx_intr) and from the xmit handler
> (pasemi_mac_start_tx) at the same time ?

You're right. Bummer. I'll add locking on the rings.

> [...]
> > > unsigned int is supposed to save some cycles on ppc.
> > 
> > Who told you that? That's not true.
> 
> Jon D Mason <jonmason@us.ibm.com> on 25/08/2004 about ppc64 (not ppc, sorry).

Interesting, I hadn't thought about that before.

There's nothing architectural in PPC that makes signed math slower than
unsigned, but in the case of modulo operations (which we do alot on the
rings), unsigned is per definition more complex to do the operations on.

It's pretty much within the noise on the current implementation, but
still an interesting tidbit. Thanks.

> [...]
> > > > +#define DESCR(ring, i) ((ring)->desc[i % ((ring)->count)])
> > > > +#define BUFF(ring, i) ((ring)->buffers[i % ((ring)->count)])
> > > > +#define INFO(ring, i) ((ring)->desc_info[i % ((ring)->count)])
> > > 
> > > A bit ugly/obfuscating/name clash prone imvho.
> > > 
> > > Use local variables ?
> > 
> > I'm open for suggestions here, not sure how local variables will help though?
> 
> 	struct pas_dma_xct_descr *desc = ring->desc[i % ring->count];

That makes sense. Done.


-Olof