netdev - Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY

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: <CAGVrzcaN=noWOoMj0w720H5TUBypOv2HAFPPzJRAfA=R4RLUmw@mail.gmail.com>
Date:	Fri, 31 Jan 2014 16:40:06 -0800
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Max Filippov <jcmvbkbc@...il.com>
Cc:	Marc Gauthier <marc@...ence.com>,
	Ben Hutchings <ben@...adent.org.uk>,
	LKML <linux-kernel@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Chris Zankel <chris@...kel.net>,
	"linux-xtensa@...ux-xtensa.org" <linux-xtensa@...ux-xtensa.org>,
	netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on
 attached PHY

2014-01-30 Max Filippov <jcmvbkbc@...il.com>:
> On Wed, Jan 29, 2014 at 10:32 PM, Max Filippov <jcmvbkbc@...il.com> wrote:
>> On Wed, Jan 29, 2014 at 9:12 PM, Florian Fainelli <f.fainelli@...il.com> wrote:
>>> On Jan 28, 2014 11:01 PM, "Max Filippov" <jcmvbkbc@...il.com> wrote:
>>>>
>>>> On Wed, Jan 29, 2014 at 10:47 AM, Florian Fainelli <f.fainelli@...il.com>
>>>> wrote:
>>>> > Hi Max,
>>>> >
>>>> > Le 28/01/2014 22:00, Max Filippov a écrit :
>>>> >
>>>> >> OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but
>>>> >> does
>>>> >> not disable advertisement when PHY supports them. This results in
>>>> >> non-functioning network when the MAC is connected to a gigabit PHY
>>>> >> connected
>>>> >> to a gigabit switch.
>>>> >>
>>>> >> The fix is to disable gigabit speed advertisement on attached PHY
>>>> >> unconditionally.
>>>> >>
>>>> >> Signed-off-by: Max Filippov <jcmvbkbc@...il.com>
>>>> >> ---
>>>> >> Changes v1->v2:
>>>> >> - disable both gigabit advertisement and support.
>>>> >>
>>>> >>   drivers/net/ethernet/ethoc.c | 8 ++++++++
>>>> >>   1 file changed, 8 insertions(+)
>>>> >>
>>>> >> diff --git a/drivers/net/ethernet/ethoc.c
>>>> >> b/drivers/net/ethernet/ethoc.c
>>>> >> index 4de8cfd..5643b2d 100644
>>>> >> --- a/drivers/net/ethernet/ethoc.c
>>>> >> +++ b/drivers/net/ethernet/ethoc.c
>>>> >> @@ -688,6 +688,14 @@ static int ethoc_mdio_probe(struct net_device
>>>> >> *dev)
>>>> >>         }
>>>> >>
>>>> >>         priv->phy = phy;
>>>> >> +       phy_update_advert(phy,
>>>> >> +                         ADVERTISED_1000baseT_Full |
>>>> >> +                         ADVERTISED_1000baseT_Half, 0);
>>>> >> +       phy_start_aneg(phy);
>>>> >
>>>> >
>>>> > This does not look necessary, you should not have to call
>>>> > phy_start_aneg()
>>>> > because the PHY state machine is not yet started, at best this calls
>>>> > does
>>>> > nothing.
>>>>
>>>> This call actually makes the whole thing work, because otherwise once
>>>> gigabit
>>>> support is cleared from the supported mask genphy_config_advert does not
>>>> update gigabit advertisement register, leaving it enabled.
>>>
>>> OK, then we need to figure out what is wrong with ethoc since this is
>>> unusual.
>>
>> Maybe they boot up with gigabit advertisement disabled in their PHY
>> and thus they don't see the problem?
>>
>>> Other drivers do the following:
>>>
>>> - connect to the PHY
>>> - phydev->supported = PHY_BASIC_FEATURES
>>> - phydev->advertising &= phydev->supported
>>> - start the PHY state machine
>>>
>>> And they work just fine. Is the PHY driver you are bound to the "Generic
>>> PHY" or something else which does something funky in config_aneg()?
>>
>> It's marvell 88E1111 from the KC-705 board, but the behaviour doesn't
>> change if I disable it and the generic phy is used.
>
> Florian,
>
> I don't see how the generic genphy_config_advert can ever change
> gigabit advertisement if phydev->supported has gigabit speeds masked
> off.

It does not, but it makes sure that phydev->advertising gets masked
with phydev->supported. So, prior to starting your PHY state machine,
if you do:

phydev->supported &= PHY_BASIC_FEATURES;
phydev->advertising = phydev->supported;

you should get the expected results. Just make sure that
phydev->autoneg is not set to AUTONEG_DISABLE, otherwise
phy_start_aneg() will not call phy_sanitize_settings(), and this might
be the problem.

> So I'm pretty sure that other 10/100 cards would exhibit the same
> issue if their PHY started with gigabit advertisement enabled.

If the speed is not restricted, yes that will be the case, but most
drivers seem to correctly set both phydev->supported and
phydev->advertising.

> Maybe
> we need to fix those other drivers? Or maybe we need to track what
> PHY really supports vs. what we report it supports, so that gigabit
> advertisement could be changed even when the PHY no longer
> appears to support gigabit?

This is supposedly already taken care of provided that you correctly
restrict the PHY supported/advertising bits with respect to what the
HW really supports.
-- 
Florian
--
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