[gPXE-devel] [PATCH] [jme] Adding JMicron Ethernet driver

[Guo-Fu Tseng]

On Fri, 28 May 2010 11:01:02 -0700, Joshua Oreman wrote
> On Fri, May 28, 2010 at 10:27 AM,  <cooldavid at cooldavid.org> wrote:
> > From: Guo-Fu Tseng <cooldavid at cooldavid.org>
> >
> > A new driver for JMicron Ethernet controller.
> >
> > Signed-off-by: Guo-Fu Tseng <cooldavid at cooldavid.org>
> 
> I'm very happy with this driver. It's very modular and easy to understand.
> 
> > +#if __x86_64__
> > +       rxdesc->desc1.flags     = RXFLAG_64BIT;
> > +#endif
> > +       wmb();
> > +       rxdesc->desc1.flags     |= RXFLAG_OWN | RXFLAG_INT;
> 
> The wmb() seems like it should be either before all the flags
> modifications or after all of them. Is there a reason this isn't so?
The hardware will consider this entry of ring-buffer as useable
while it sees RXFLAG_OWN, RXFLAG_INT flags are set. The wmb() here
is to prevent the hardware getting incorrectly initiallized ringbuffer
due to the re-ordering.

> 
> > +static int
> > +jme_check_link(struct net_device *netdev, int testonly)
> > +{
> > +       struct jme_adapter *jme = netdev->priv;
> > +       u32 phylink, ghc, cnt = JME_SPDRSV_TIMEOUT, gpreg1;
> > +       char linkmsg[64];
> > +       int rc = 0;
> 
> In this function, I believe the code and string data used in
> constructing linkmsg will make it into even non-debug versions of the
> executable. If that's true it would be better if you could perform all
> the manipulations in the final DBG() line, using the ?: operator and
> such. It's possible the compiler is clever enough to know what
> strcat() does and see that linkmsg[] is not used in non-debug, though,
> so this may be moot.
Good point, I missed that while I was porting. I'll do that. :)

> 
> Other than that, looks excellent!
> 
> -- Josh


Guo-Fu Tseng