netdev - Re: [RFC PATCH 05/17] phy_driver: Make .read_status()/.config

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]

Date:	Fri, 21 Oct 2011 01:13:30 -0400
From:	Kyle Moffett <kyle@...fetthome.net>
To:	Mike Frysinger <vapier@...too.org>
Cc:	Kyle Moffett <Kyle.D.Moffett@...ing.com>,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	Randy Dunlap <rdunlap@...otime.net>,
	Stephen Hemminger <shemminger@...tta.com>,
	"David S. Miller" <davem@...emloft.net>,
	Greg Dietsche <Gregory.Dietsche@....edu>,
	Giuseppe Cavallaro <peppe.cavallaro@...com>,
	David Daney <ddaney@...iumnetworks.com>,
	Arnaud Patard <arnaud.patard@...-net.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	Baruch Siach <baruch@...s.co.il>,
	Thorsten Schubert <tshu@...-ge.com>,
	David Decotigny <decot@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Lucas De Marchi <lucas.demarchi@...fusion.mobi>,
	Marc Kleine-Budde <mkl@...gutronix.de>,
	linux-doc@...r.kernel.org
Subject: Re: [RFC PATCH 05/17] phy_driver: Make .read_status()/.config_aneg() optional

Oops, I just realized that I may have misused
scripts/get_maintainer.pl... it seems to have added about 20 CCs to
this email, sorry!

On Thu, Oct 20, 2011 at 17:14, Mike Frysinger <vapier@...too.org> wrote:
> On Thursday 20 October 2011 17:10:12 Mike Frysinger wrote:
>> On Thursday 20 October 2011 17:00:12 Kyle Moffett wrote:
>> > Approximately 90% of the PHY drivers follow the PHY layer docs and
>> > simply use &genphy_read_status and &genphy_config_aneg.  There would
>> > seem to be little point in requiring them all to manually specify those
>> > functions.
>>
>> well, it does make sense if you think about the compile vs build time
>> overhead.  yes, your patch does make things much nicer to read, and a
>> little easier to maintain the source.  however, it adds runtime overhead
>> (checking the func pointers) while the func pointer storage is unchanged
>> (it's now a NULL pointer instead of pointing to the genphy funcs).
>> personally, i think the savings in runtime and smaller compiled code is
>> more important.  so i'm going to NAK this.  sorry.
>
> ah, sorry, i was thinking this was u-boot since we were just having
> conversations there.
>
> since this is Linux, and i don't have real standing in the general netdev
> community, i can't really NAK here.  but i think my comment still stands in
> that this patch makes things much worse than the minor code style improvement.

I would argue that the PHY layer itself is not remotely a hot-path,
especially not to the level of caring about an extra if statement.  A
single phy_read() is a sleeping call which goes over a slow serial
bus, so code maintainability is much more important.

At the higher level, the reason for this change is that currently
genphy_config_aneg() is currently responsible both for configuring
auto-negotiation *AND* counterintuitively for configuring forced-mode
links.  A lot of PHY drivers need to override just one or the other,
which results in either a lot of copy-pasted code in individual PHY
drivers or in a duplicate test for autoneg-vs-fixed-mode before
calling the primary genphy_config_aneg() function.

In order to split that up and refactor it, I would like to minimize
the number of callers and references to config_aneg() to minimize the
number of places that have to be changed to match.  Since all the rest
of the PHY functions are already conditionals, making these two also
conditional didn't seem to be a big deal.

Cheers,
Kyle Moffett

-- 
Curious about my work on the Debian powerpcspe port?
I'm keeping a blog here: http://pureperl.blogspot.com/
--
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