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: <55A95F83.8010900@list.ru>
Date:	Fri, 17 Jul 2015 23:03:15 +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

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.

> So is different is that I use a link_update callback, and so we rely on
> at least one call of this function to initialize the hardware in
> drivers/net/dsa/bcm_sf2.c
Do you mean this?:
core_writel(priv, reg, CORE_STS_OVERRIDE_GMIIP_PORT(port));
Maybe just moving the HW initialization bits to some init func
will be a quick fix?

>   for this to work, after that, the hardware
> reflects the fixed link parameters we configured, and we feed the
> fixed_phy_status information from the hardware directly.
>
> >From there I see two different ways to fix this:
>
> - we ignore the fixed_phy_update_regs return value in fixed_phy_add(),
> but that will make us avoid doing verification on the speed, which is
> not so great, but is essentially what your patch does anyway
No, it does not. All it does is to allow no speed _when link is down_,
which is IMHO a very logical fix. The speed checks for the link-up
case are all still there.

> - we update the use of the fixed PHY link_update in drivers using it
IMHO just 2 drivers: bcmii.c and bcm_sf2.c, and the change
is likely trivial, although of course I am not sure in details.

>   and
> convert them to use fixed_phy_update_state instead, which can take some
> time and effort to convert
Maybe just move the initialization bits out of the link_update
callback, but still use the callback for now? Should be simple, no?
--
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