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: <1371766157.8333.25.camel@jlt4.sipsolutions.net>
Date:	Fri, 21 Jun 2013 00:09:17 +0200
From:	Johannes Berg <johannes@...solutions.net>
To:	Ben Hutchings <ben@...adent.org.uk>
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 22:58 +0100, Ben Hutchings wrote:

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

Ok, I can kick out the 'select NET_CORE' here anyway as MDIO doesn't
need it. Or did you want to do a more general cleanup on this, then I'll
leave it to you?

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

Yes, though here it was really used only internally... Anyway, I killed
it, makes some code nicer and some a bit more awkward (more parameters)

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

Hmm, ok. I'll do it that way.

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

Ok, makes sense. I guess a bunch of these things are done in
alx_post_phy_link() or so, but yeah I don't see flow control either.

How would I even test that? Find random devices that have flow control?
I don't think I have anything that would be programmable, unless I don't
know about it being so :-)

johannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html