netdev - Re: [Patch] Micrel KS8695 integrated ethernet driver

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-next>] [day] [month] [year] [list]

Message-Id: <1228843809.17317.34.camel@petitemort>
Date:	Tue, 09 Dec 2008 17:30:09 +0000
From:	Daniel Silverstone <dsilvers@...tec.co.uk>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	netdev@...r.kernel.org
Subject: Re: [Patch] Micrel KS8695 integrated ethernet driver

Ben pointed out that he had sent this to me but not CCed the list, and
that I'd missed it. I've worked through it now.

On Mon, 2008-12-08 at 19:35 +0000, Ben Hutchings wrote:
> There's a spelling error in your subject; it should say "integrated".

That's just a spello on the mailing list subject line, the subject line
in the patch is okay.

> On Mon, 2008-12-08 at 13:08 +0000, Daniel Silverstone wrote:
> [...]
> > +#define MODULENAME     "ks8695_ether"
> Shouldn't this be "ks8695net"?

I was replacing Andrew Victor's ks8695_ether driver, so the platform
data is for ks8695_ether, but for a while I had both drivers in-tree in
order to check behaviour. I am happy for it to be merged either as
ks8695_ether or ks8695net and if necessary I can change the platform
data for the boards. However I was trying for path of least resistance
on this particular front.

> > +/*
> > + * Transmit timeout, default 5 seconds.
> > + */
> > +static int watchdog = 5000;
> The comment should say this is also the reset timeout.

Yep, changed, thanks.

> > + *     enum ks8695_dtype - Device type
> > + *     @KS8695_DTYPE_WAN: This device is a WAN interface
> > + *     @KS8695_DTYPE_LAN: This device is a LAN interface
> > + *     @KS8695_DTYPE_HPNA: This device is an HPNA interfacew
> Typo: there's a rogue "w" on the end.

Argh, fixed.

> > +static inline struct ks8695_priv *
> > +ks8695_get_priv(struct net_device *ndev)
> > +{
> > +       return ndev->priv;
> > +}
> This function needs to be removed.

Yes, I did the netdev_priv() change but forgot to remove it once I had
compile-and-run-tested the kernel. I have removed it now, thanks.

> > +static void
> > +ks8695_init_partial_multicast(struct ks8695_priv *ksp,
> > +               /* Ran out of space in chip? */
> > +               if (i == KS8695_NR_ADDRESSES)
> > +                       break;
> Currently the caller already checks that you won't run out of space, so
> you can use BUG_ON() here.  Certainly I don't think this should quietly
> ignore the error.

Accepted. I'm still getting used to BUG_ON etc. Changed.

> > +       if (ndev->flags & IFF_ALLMULTI) {
> > +               /* enable all multicast mode */
> > +               ctrl |= DRXC_RM;
> > +       } else if (ndev->mc_count > KS8695_NR_ADDRESSES) {
> > +               /* more specific multicast addresses than can be
> > +                * handled in hardware
> > +                */
> > +               ctrl |= DRXC_RM;
> > +       } else if (ndev->mc_count > 0) {
> > +               /* enable specific multicasts */
> > +               ctrl &= ~DRXC_RM;
> > +               ks8695_init_partial_multicast(ksp, ndev->mc_list,
> > +                                             ndev->mc_count);
> > +       } else if (ndev->flags & ~IFF_ALLMULTI) {
> > +               /* disable multicast mode */
> > +               ctrl &= ~DRXC_RM;
> > +               ks8695_init_partial_multicast(ksp, NULL, 0);
> > +	}
> [...]
> 
> This last condition is bogus.  I think it's supposed to mean
> !(ndev->flags & IFF_ALLMULTI) but that will always be true here.
> 
> Further, I think the code for ndev->mc_count > 0 will also work for
> ndev->mc_count == 0, so you could remove both conditions and the last
> block.

Yep, makes sense. That'll teach me to blindly go "and now this case, and
now this case, and now this case" without trying to compress the cases.

Done.

New patch attached, which, regardless of my previous assertion, I *now*
believe covers everything (providing you're happy about the driver name
malarky). To celebrate this, I even changed the version a bit so we can
be sure we're all reviewing the same code.

Thanks again, and hopefully this will be good enough for merging. Sorry
for the repeated posting of patches, but I think I'm learning.

Regards,

Daniel.

-- 
Daniel Silverstone                              http://www.simtec.co.uk/
PGP mail accepted and encouraged.            Key Id: 2BC8 4016 2068 7895

View attachment "ks8695-ethernet-net-next-2.6.patch" of type "text/x-patch" (51819 bytes)