netdev - Re: [PATCH 1/3] fixed_phy: handle link-down case

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: <55A0DACC.7070804@list.ru>
Date:	Sat, 11 Jul 2015 11:58:52 +0300
From:	Stas Sergeev <stsp@...t.ru>
To:	Florian Fainelli <f.fainelli@...il.com>,
	netdev <netdev@...r.kernel.org>
Cc:	Linux kernel <linux-kernel@...r.kernel.org>,
	Sebastien Rannou <mxs@...k.org>,
	Arnaud Ebalard <arno@...isbad.org>,
	Stas Sergeev <stsp@...rs.sourceforge.net>
Subject: Re: [PATCH 1/3] fixed_phy: handle link-down case

11.07.2015 03:15, Florian Fainelli пишет:
> On 10/07/15 14:14, Stas Sergeev wrote:
>> 10.07.2015 23:44, Florian Fainelli пишет:
>>> On 10/07/15 09:41, Stas Sergeev wrote:
>>>> Currently fixed_phy driver recognizes only the link-up state.
>>>> This simple patch adds an implementation of link-down state.
>>>> The actual change is 1-line, the rest is an indentation.
>>> It is not clear to me how this is useful, if you have a link_update
>>> callback manipulating the link state, the fixed PHY driver returns
>>> appropriate MII_BMSR values and always re-initializes everything.
>> It returns the appropriate values only for link status (when its down),
>> but it still returns speed, duplex etc as if the link is up. I had hard
>> times finding the relevant specs, but from what I have googled,
>> when link is down, the speed/duplex/etc status fields should _also_
>> be zero, which is what my patch does.
>> What is more important is that fixed_phy_add() would return
>> -EINVAL if you didn't specify speed while the link is down.
>> This is an absolute must-fix, or I will have to add an arbitrary
>> speed value again, on which you already objected.
> Ok, but that does not seems to be a code path that you can hit, unless
> you are already modifying
> drivers/of/of_mdio.c::of_fixed_phy_register_link() and overriding how
> status.link is defined, am I missing something?
I think you can.
The drivers that do autonegotiation (eg mvneta) should take a
special care to not reset speed when link is down.
Or to nor read the speed when link is down (this is discouraged
anyway of course, but better to follow the real MDIO hw in that).
So while the work-arounds are simple, you can nevertheless hit
the bug if you try to.

>>> Is this meant to be some sort of optimization? If so, you could just
>>> avoid the re-intendation completely and do a goto instead?
>> Oh, c'mon... Adding goto just to keep the _patch_ smaller?
> Well, yes, so it's easy to audit the changes?
So you don't trust me that I only indented the code? OKey. :)

>> (not smaller code, just a smaller patch)
>> Well, this is certainly something that can be done, feel free
>> to request that explicitly and I'll release v3 next week.
> I hereby explicitly request that you make this a new iteration using a goto.
OKey, will do in v3.
Of course if you point me to the coding guidelines that explain
this part, I'll be more comfortable. But this is purely optional, I
simply don't like to add gotos where unneeded, but its not a big
deal at all.
--
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