netdev - Re: [PATCH v2 1/1] alx: add a simple AR816x/AR817x device 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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1371765508.8771.17.camel@deadeye.wl.decadent.org.uk>
Date:	Thu, 20 Jun 2013 22:58:28 +0100
From:	Ben Hutchings <ben@...adent.org.uk>
To:	Johannes Berg <johannes@...solutions.net>
Cc:	netdev@...r.kernel.org, mcgrof@...not-panic.com, kvalo@...rom.com,
	adrian.chadd@...il.com, Johannes Stezenbach <js@...21.net>
Subject: Re: [PATCH v2 1/1] alx: add a simple AR816x/AR817x device driver

On Thu, 2013-06-20 at 23:40 +0200, Johannes Berg wrote:
> (+Johannes, I have a question for you almost at the end of this email)
> 
> On Tue, 2013-06-18 at 02:18 +0100, Ben Hutchings wrote:
> 
> > > +config ALX
> > > +	tristate "Qualcomm Atheros AR816x/AR817x support"
> > > +	depends on PCI
> > > +	select CRC32
> > > +	select NET_CORE
> > 
> > Why NET_CORE?  It looks like a lot of drivers are selecting it because
> > MII depends on it, but this doesn't select MII.  (Actually, it's
> > ridiculous that MII depends on NET_CORE.  I'll see what I can do about
> > that.)
> 
> Yeah, no idea, just copy/paste. Lots of drivers seem to do this, for no
> particular reason it seems?

As I said, most of them select MII which depended on NET_CORE, so they
really did need to select NET_CORE too.  I've now fixed that.

[...]
> > > +	if (hw->link_speed != SPEED_UNKNOWN) {
> > > +		ethtool_cmd_speed_set(ecmd,
> > > +				      hw->link_speed - hw->link_speed % 10);
> > > +		ecmd->duplex = hw->link_speed % 10;
> > 
> > Please use separate fields for speed and duplex.
> 
> Heh, ok. I actually played with this a bit and didn't like it much
> either way ... There doesn't seem to be any combined enum, and if you
> use separate fields then one is valid only if the other has a certain
> value, etc... Anyway, I'll change it.

It seems to be a widely used trick, but it means that you may accept
silly settings like speed = 99 duplex = 1.

[...]
> > You should break out the code that
> > sets ecmd->supported from alx_get_settings() and then do something like:
> > 
> > 	if (ecmd->advertising & ~alx_get_supported_mask())
> > 		return -EINVAL;
> > 
> > here.
> 
> That... seems strange? It would rely on ADVERTISED_* and SUPPORTED_*
> having the same value, which is probably true, but it still seems
> confusing?

It is true and should remain true.  I don't know why they were ever
defined separately in the first place.

[...]
> > I couldn't find where auto-negotiated flow control is programmed after
> > the link comes up.  Maybe it isn't?
> 
> No idea, sorry. I clearly have a lot to learn about how ethernet devices
> typically work, should it be programmed?

When autoneg is enabled, the PHY should perform autonegotiation whenever
it detects an electrical link; if successful it will configure itself
for the negotiated link speed and duplex, and trigger a link interrupt.
But unless there is firmware running on the controller, the driver
usually has to read PHY registers to get the result and then configure
the MAC speed, duplex and flow control accordingly.

Ben.

-- 
Ben Hutchings
Lowery's Law:
             If it jams, force it. If it breaks, it needed replacing anyway.

Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)