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: <55A9956E.4070205@list.ru>
Date:	Sat, 18 Jul 2015 02:53:18 +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

18.07.2015 02:35, Florian Fainelli пишет:
> On 17/07/15 16:24, Stas Sergeev wrote:
>> 18.07.2015 01:01, Florian Fainelli пишет:
>>> On 17/07/15 13:03, Stas Sergeev wrote:
>>>> 17.07.2015 21:50, Florian Fainelli пишет:
>>>>> On 17/07/15 04:26, Stas Sergeev wrote:
>>>>>> 17.07.2015 02:25, Florian Fainelli пишет:
>>>>>>> On 16/07/15 07:50, Stas Sergeev wrote:
>>>>>>>> Currently fixed_phy driver recognizes only the link-up state.
>>>>>>>> This simple patch adds an implementation of link-down state.
>>>>>>>> It fixes the status registers when link is down, and also allows
>>>>>>>> to register the fixed-phy with link down without specifying the
>>>>>>>> speed.
>>>>>>> This patch still breaks my setups here, e.g:
>>>>>>> drivers/net/dsa/bcm_sf2.c,
>>>>>>> but I will look into it.
>>>>>>>
>>>>>>> Do we really need this for now for your two other patches to work
>>>>>>> properly, or is it just nicer to have?
>>>>>> Yes, absolutely.
>>>>>> Otherwise registering fixed phy will return -EINVAL
>>>>>> because of the missing link speed (even though the link
>>>>>> is down).
>>>>> Ok, I see the problem that you have now. Arguably you could say that
>>>>> according to the fixed-link binding, speed needs to be specified and
>>>>> the
>>>>> code correctly errors out with such an error if you do not specify
>>>>> it. I
>>>> Aren't you missing the fact that .link=0?
>>>> I think what you say is true only for the link-up case, no?
>>>> .speed==0 is valid for link-down IMHO: no link - zero speed.
>>> Pardon me being very dense and stupid here, but your problem is that the
>>> "speed" parameter is not specified in your DT,
>> Not even a fixed-link at all, since the latest patches.
>> I removed fixed-link defs from my DT.
> Hummm, okay, so you just have the inband-status property and that's it,
> not even a fixed-link node anymore, right? How does
> mvneta_fixed_link_update() work then since it needs a fixed PHY to be
> registered?
You can see it from my patch:
---

+	err = of_property_read_string(np, "managed", &managed);
+	if (err == 0) {
+		if (strcmp(managed, "in-band-status") == 0) {
+			/* status is zeroed, namely its .link member */
+			phy = fixed_phy_register(PHY_POLL, &status, np);
+			return IS_ERR(phy) ? PTR_ERR(phy) : 0;
+		}
+	}

---
which is the hunk added to the of_phy_register_fixed_link().
So in that case we register fixed-phy, but do not parse the fixed-link.

>> AFAIK when link is down, you are not allowed to rely on the PHY
>> status registers to read speed from, or am I wrong? So if my
>> understanding is correct, this was working by a pure luck.
> Well, it's more like it is undefined, and before this patch, the fixed
> PHY would update everything except the link status indication.
And what about the real MDIO PHY? Or does it never hit this
"undefined" code path?
Anyway, if you call it undefined, I guess you automatically agree
this needs to be fixed. :)

>> As for the quick fix - why not to do this pre-init in fixed_link_update()
>> instead of adjust_link()? In fixed_link_update() you'll get the speed
>> right from DT, so it will be correct.
> fixed_link_update() only gets called once you start your PHY state
> machine, unfortunately, not upon fixed PHY device registration, and it
> runs before your adjust_link callback does,
So you say fixed_link_update() runs before adjust_link callback does,
which looks logical. Why would you need it to run on device registration,
if it runs earlier than adjust_link (which you use for init) even now?
--
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