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

[Joshua Oreman]

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?

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

Other than that, looks excellent!

-- Josh