Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

From: Dan Murphy
Date: Tue Feb 18 2020 - 12:43:27 EST


On 2/18/20 11:33 AM, Russell King - ARM Linux admin wrote:
On Tue, Feb 18, 2020 at 11:12:37AM -0600, Dan Murphy wrote:
Well now the read_status is becoming a lot more complex. It would be better
to remove the ack_interrupt call back and just have read_status call the
interrupt function regardless of interrupts or not. Because all the
interrupt function would be doing is clearing all the interrupts in the ISR
register on a link up/down event. And as you pointed out we can check and
set a flag that indicates if a downshift has happened on link up status and
clear it on link down. We would need to set the downshift interrupt mask to
always report that bit. As opposed to not setting any interrupts if
interrupts are not enabled. If the user wants to track WoL interrupt or any
other feature interrupt we would have to add that flag to the read_status as
well seems like it could get a bit out of control.
To be honest, I don't like phylib's interrupt implementation; it
imposes a fixed way of dealing with interrupts on phylib drivers,
and it sounds like its ideas don't match what modern PHYs want.

For example, the Marvell 88x3310 can produce interrupts for GPIOs
that are on-board, or temperature alarms, or other stuff... but
phylib's idea is that a PHY interrupt is for signalling that the
link changed somehow, and imposes a did_interrupt(), handle
interrupt, clear_interrupt() structure whether or not it's
suitable for the PHY. If you don't provide the functions phylib
wants, then phydev->irq gets killed. Plus, phylib claims the
interrupt for you at certain points depending on whether the
PHY is bound to a network interface or not.

So, my suggestion to move ack_interrupt to did_interrupt will
result in phylib forcing phydev->irq to PHY_POLL, killing any
support for interrupts what so ever...

Does it really make sense to do all of this for a downshift message in the kernel log?

Again this is a lot of error prone complex changes and tracking just to
populate a message in the kernel log. There is no guarantee that the LP did
not force the downshift or advertise that it supports 1Gbps. So what
condition is really being reported here? There seems like there are so many
different scenarios why the PHY could not negotiate to its advertised 1Gbps.
Note that when a PHY wants to downshift, it does that by changing its
advertisement that is sent to the other PHY.

So, if both ends support 1G, 100M and 10M, and the remote end decides
to downshift to 100M, the remote end basically stops advertising 1G
and restarts negotiation afresh with the new advertisement.

In that case, if you look at ethtool's output, it will indicate that
the link partner is not advertising 1G, and the link is operating at

If 100M doesn't work (and there are cases where connections become
quite flakey) then 100M may also be omitted as well, negotiation
restarted, causing a downshift to 10M.

That's basically how downshift works - a PHY will attempt to
establish link a number of times before deciding to restart
negotiation with some of the advertisement omitted, and the
reduced negotiation advertisement appears in the remote PHY's
link partner registers.
This is the way I understand it too.
As I mentioned, the PHY on either end of the link can be the one
which decides to downshift, and the partner PHY has no idea that
a downshift has happened.

Exactly so we can only report that if the PHY on our end caused the downshift. If this PHY does not cause the downshift then the message will not be presented even though a downshift occurred. So what is the value in presenting this message?