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] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 18 Apr 2012 21:30:47 +0200
From:	Jonas Gorski <jonas.gorski@...il.com>
To:	mbizon@...ebox.fr
Cc:	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

Hi Maxime,

On 18 April 2012 14:48, Maxime Bizon <mbizon@...ebox.fr> wrote:
>
> On Wed, 2012-04-18 at 14:02 +0200, Jonas Gorski wrote:
>
>> Only connect/disconnect the phy during probe and remove, not during open
>> and close. The phy seldom changes during the runtime, and disconnecting
>> the phy during close will prevent the phy driver from keeping any
>> configuration over a down/up cycle.
>>
>> Signed-off-by: Jonas Gorski <jonas.gorski@...il.com>
>
> please CC me, I wrote this driver

Oops, sorry, that wasn't intentional. I used get_maintainer.pl, which
didn't catch you, and I never checked the actual file. Duly noted for
v2.

>
>> -             phydev = phy_connect(dev, phy_id, bcm_enet_adjust_phy_link, 0,
>> -                                  PHY_INTERFACE_MODE_MII);
>
> bcm_enet_adjust_link() may modify some dma registers that are reset by
> bcm_enet_open(), since it can now be called after probe, we may end up
> with broken flow control depending on whatever was called first.

I assume you mean bcm_enet_adjust_phy_link()? bcm_enet_adjust_link()
gets only called if there is no phydev.

I have to admit, I fail to see the race (but feel free to correct me):

On boot:

The phy state machine will start in PHY_READY after phy_connect, which
will result in NOPs, so no call to bcm_enet_adjust_phy_link() after
_probe() and before _open(). The state machine starts doing real work
only after calling phy_start(), which happens only after the dma
register accesses in open(). So no race here.

In case of an down/up cycle:

_close() will call phy_stop() first, which will either block until the
current state machine run is complete if there is one, or will block
the next run until it set the state to PHY_HALTED.

When in PHY_HALTED, bcm_enet_adjust_phy_link() will be called once,
but with phy_dev->link = 0, and only phy_dev->link = 1 can result in
register writes in bcm_enet_adjust_phy_link():

	if (phydev->link && phydev->duplex != priv->old_duplex) {
		bcm_enet_set_duplex(priv,
				    (phydev->duplex == DUPLEX_FULL) ? 1 : 0);
		...
	}

	if (phydev->link && phydev->pause != priv->old_pause) {
		...
		bcm_enet_set_flow(priv, rx_pause_en, tx_pause_en);
		...
	}

So no problem there either.

There is a theoretical race in writing to priv->old_link in _open()
and the bcm_enet_adjust_phy_link() call right after phy_stop(), but
since both write the same value to priv->old_link I see no problem
here.

Regards
Jonas
--
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