* ethtool occationally fails to communicate with with ucc_geth
@ 2013-02-06 20:05 Lennart Sorensen
2013-02-06 21:08 ` Ben Hutchings
0 siblings, 1 reply; 3+ messages in thread
From: Lennart Sorensen @ 2013-02-06 20:05 UTC (permalink / raw)
To: Li Yang; +Cc: netdev, linuxppc-dev, Len Sorensen, linux-kernel
We are occationally seeing ethtool fail to communicate with ucc_geth.
I think I have tracked down why it happens, but I don't see a good way
to fix it.
When the phy state changes, adjust_link() checks if the state has changed
and if the link is up. If it is it does:
if (new_state) {
/*
* To change the MAC configuration we need to disable
* the controller. To do so, we have to either grab
* ugeth->lock, which is a bad idea since 'graceful
* stop' commands might take quite a while, or we can
* quiesce driver's activity.
*/
ugeth_quiesce(ugeth);
ugeth_disable(ugeth, COMM_DIR_RX_AND_TX);
out_be32(&ug_regs->maccfg2, tempval);
out_be32(&uf_regs->upsmr, upsmr);
ugeth_enable(ugeth, COMM_DIR_RX_AND_TX);
ugeth_activate(ugeth);
}
The problem I believe is that ugeth_quiesce() does netif_device_detach
which clears __LINK_STATE_PRESENT, and hence makes dev_ethtool fail
due to:
if (!dev || !netif_device_present(dev))
return -ENODEV;
So if ethtool happens to be run between ugeth_quiesce() and
ugeth_activate(), it fails as if the device simply doesn't exist, which
is of course not true, it's just temporarily disabled.
I don't see any obvious way to make the ethtool requests block while the
adjust_link does it's business. It seems that that making the device
disappear is the wrong thing to do though.
I am able to make it happen if I do:
'while ethtool ifname; do :; done' while plugging and unplugging the
cable for a few minutes.
Any suggestions?
--
len Sorensen
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: ethtool occationally fails to communicate with with ucc_geth
2013-02-06 20:05 ethtool occationally fails to communicate with with ucc_geth Lennart Sorensen
@ 2013-02-06 21:08 ` Ben Hutchings
2013-02-06 22:24 ` Lennart Sorensen
0 siblings, 1 reply; 3+ messages in thread
From: Ben Hutchings @ 2013-02-06 21:08 UTC (permalink / raw)
To: Lennart Sorensen; +Cc: Li Yang, netdev, linuxppc-dev, linux-kernel
On Wed, 2013-02-06 at 15:05 -0500, Lennart Sorensen wrote:
> We are occationally seeing ethtool fail to communicate with ucc_geth.
> I think I have tracked down why it happens, but I don't see a good way
> to fix it.
>
> When the phy state changes, adjust_link() checks if the state has changed
> and if the link is up. If it is it does:
>
> if (new_state) {
> /*
> * To change the MAC configuration we need to disable
> * the controller. To do so, we have to either grab
> * ugeth->lock, which is a bad idea since 'graceful
> * stop' commands might take quite a while, or we can
> * quiesce driver's activity.
> */
> ugeth_quiesce(ugeth);
> ugeth_disable(ugeth, COMM_DIR_RX_AND_TX);
>
> out_be32(&ug_regs->maccfg2, tempval);
> out_be32(&uf_regs->upsmr, upsmr);
>
> ugeth_enable(ugeth, COMM_DIR_RX_AND_TX);
> ugeth_activate(ugeth);
> }
>
> The problem I believe is that ugeth_quiesce() does netif_device_detach
> which clears __LINK_STATE_PRESENT, and hence makes dev_ethtool fail
> due to:
>
> if (!dev || !netif_device_present(dev))
> return -ENODEV;
>
> So if ethtool happens to be run between ugeth_quiesce() and
> ugeth_activate(), it fails as if the device simply doesn't exist, which
> is of course not true, it's just temporarily disabled.
[...]
> Any suggestions?
This seems to be a workaround for a bug in phylib: phy_state_machine()
calls netif_carrier_on() before adjust_link(), so the TX scheduler can
start immediately even though the MAC has not been configured.
A better workaround would be to use netif_carrier_{off,on}() in
ugeth_{quiesce,activate}() respectively instead of
netif_device_{detach,attach}(). But I think phylib really ought to be
fixed.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: ethtool occationally fails to communicate with with ucc_geth
2013-02-06 21:08 ` Ben Hutchings
@ 2013-02-06 22:24 ` Lennart Sorensen
0 siblings, 0 replies; 3+ messages in thread
From: Lennart Sorensen @ 2013-02-06 22:24 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Li Yang, netdev, linuxppc-dev, linux-kernel
On Wed, Feb 06, 2013 at 09:08:32PM +0000, Ben Hutchings wrote:
> This seems to be a workaround for a bug in phylib: phy_state_machine()
> calls netif_carrier_on() before adjust_link(), so the TX scheduler can
> start immediately even though the MAC has not been configured.
>
> A better workaround would be to use netif_carrier_{off,on}() in
> ugeth_{quiesce,activate}() respectively instead of
> netif_device_{detach,attach}(). But I think phylib really ought to be
> fixed.
I am willing to try things, but this is certainly in parts of the network
stack I don't normally poke around in and hence don't know how works.
I just managed to track it down this far. :)
I can try the carrier_off/on in place of the detach/attach and see if
it works.
--
Len Sorensen
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-02-06 22:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-06 20:05 ethtool occationally fails to communicate with with ucc_geth Lennart Sorensen
2013-02-06 21:08 ` Ben Hutchings
2013-02-06 22:24 ` Lennart Sorensen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).