On 09/06/2017 07:55 AM, Mason wrote: > On 31/08/2017 21:18, Florian Fainelli wrote: > >> On 08/31/2017 12:09 PM, Mason wrote: >> >>> 1) nb8800_link_reconfigure() calls phy_print_status() >>> which prints the "Link down" and "Link up" messages >>> to the console. With the patch reverted, nothing is >>> printed when the link goes down, and the result is >>> random when the link comes up. Sometimes, we get >>> down + up, sometimes just up. >> >> Nothing printed when you bring down the network interface as a result of >> not signaling the link down, there is a small nuance here. > > Let me first focus on the "Link down" message. > > Do you agree that such a message should be printed when the > link goes down, not when the link comes up?
The question is not so much about printing the message rather than a) having the adjust_link callback be called and b) having this particular callback correctly determine if a "change" has occurred, but let's focus on the notification too. Printing the message is a consequence of these two conditions and that's what matters. There is not unfortunately a hard and fast answer it's clearly a philosophical problem here. The link is not physically down, the cable is still plugged so generating a link down even is not necessarily correct. It would be convenient for network manager programs, just like it is for network drivers to treat it as such because that allows them to act like if the cable was unplugged, which may be a good way to perform a number of actions including but not limited to: entering a low power state, re-initialization parts of the Ethernet MAC that need it (DMA, etc., etc.). That does not appear to be an absolute requirement for most, if not all drivers since it changed after 3.4 and no one did bat an eye about it. Upon bringing the interface back up again, same thing, if the cable was not disconnected should we just generate a link UP event, and if we do that, are we going to confuse any network manager application? Generating a link transition DOWN -> UP is certainly helpful for any network application in that they do not need to keep any state just like it clearly indicates a change was detected. > > Perhaps the issue is that the 2 following cases need to be > handled differently: > A) operator sets link down on the command-line This is already handled differently since when you administratively bring down an interface you call ndo_stop() which will be doing a phy_stop() + phy_disconnect() which result in stopping the PHY state machine and disconnecting from the PHY. > B) asynchronous event makes link go down (peer is dead, cable is cut, etc) > > In B) the PHY state machine keeps on running, and eventually > calls adjust_link() Correct. > > In A) the driver calls phy_stop() and phy_disconnect() and > therefore adjust_link() will not be called? That is the current behavior (after the revert) and we can always change it if deemed necessary, problem is, this broke for two people (one still being discussed as of right now), so at this point I am very wary of making any changes without more testing. I really need to get one of these PHY interrupts wired to one of my boards or create a software model of such a configuration before accepting new changes in that area. Thank you -- Florian
