netdev - Re: [PATCH] NET: bcm63xx_enet: move 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] [day] [month] [year] [list]

Message-ID: <CAKWjMd4e07+guAJAEh1w2MrMGu5+deOYzBbmSMGPdjyE5jZjFQ@mail.gmail.com>
Date:	Tue, 24 Apr 2012 14:05:58 -0500
From:	Andy Fleming <afleming@...il.com>
To:	Jonas Gorski <jonas.gorski@...il.com>
Cc:	mbizon@...ebox.fr, Andy Fleming <afleming@...escale.com>,
	netdev@...r.kernel.org, Florian Fainelli <florian@...nwrt.org>,
	Eric Dumazet <eric.dumazet@...il.com>,
	"David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH] NET: bcm63xx_enet: move phy_(dis)connect into probe/remove

On Sun, Apr 22, 2012 at 6:31 AM, Jonas Gorski <jonas.gorski@...il.com> wrote:
> On 19 April 2012 18:17, Maxime Bizon <mbizon@...ebox.fr> wrote:
>>
>> On Thu, 2012-04-19 at 16:52 +0200, Jonas Gorski wrote:
>>
>>> Yes, but none of the ethtool functions cause register writes in the
>>> priv->has_phy = true case when in PHY_READY or PHY_HALTED state. All
>>> they do is modify the phy_device's settings.
>>
>> unless I'm mistaken:
>>
>> phy_ethtool_sset() => phy_start_aneg()
>>
>> will kick the state machine even when state is PHY_READY
>
> Hmm. I see what you mean. I wonder if it is intended that you can do
> that without having phy_start() called first.
>
> @Andy, can you perhaps shed some light on this? How are ethernet
> drivers supposed to behave/when should they call
> phy_connect()/phy_start()? Currently most drivers call phy_connect()
> in their _probe(), and phy_start() in _open(), so many seem to have
> the issue that the phy state machine is in PHY_READY after _probe(),
> and can be kicked into running through ethtool even if the interface
> is down.
>
> This problem goes away after the first ifup/ifdown cycle, since the
> phy state machine is then in PHY_HALTED, which gets properly caught in
> phy_start_aneg().
>
> To me it looks like phy_start_aneg() should check for some more
> states, as it currently would also overwrite a PHY_STARTING or
> PHY_PENDING state, which looks definitely wrong to me.

Ugh, it looks like much has gone wrong with the state machine
(possibly from when I wrote it). I'm thinking that what we should
probably do is eliminate PHY_UP and PHY_READY, and have the PHY come
up to PHY_HALTED. For some reason, PHY_RESUMING always enables
interrupts, even if phydev->irq is in POLL mode, so that should be
fixed.

Other than that, it looks like PHY_RESUMING should work in the place
of PHY_UP, and PHY_HALTED should be the same as PHY_READY...

Andy
--
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