On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote: > Hi Russell, > > On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote: > > This should be removed - state->link is not for use in mac_config. > > Even in fixed mode, the link can be brought up/down by means of a > > gpio, and this should be dealt with via the mac_link_* functions. > > > > What do you mean exactly that state->link is not for use, is that true in > general?
Yes. mac_config() should not touch it; it is not always in a defined state. For example, if you set modes via ethtool (the ethtool_ksettings_set API) then state->link will probably contain zero irrespective of the true link state. It exists in this structure because it was convenient to just use one structure to store all the link information in various parts of the code, and when requesting the negotiated in-band MAC configuration. I've come to the conclusion that that decision was a mistake, based on patches such as the above mistakenly thinking that everything in the state structure is fair game. I've since updated the docs to explicitly spell it out, but I'm also looking at the feasibility of changing the mac_config() interface entirely - splitting it into two (mac_config_fixed() and mac_config_inband()) and passing only the appropriate parameters to each. However, having looked at that, I think such a change will make some MAC drivers quite a bit more complicated - having all the config steps in one method appears to make the configuration of MAC drivers easier (eg, mvneta, mvpp2.) > In drivers/net/dsa/sja1105/sja1105_main.c, if I remove the "if > (!state->link)" guard, I see PHYLINK calls with a SPEED_UNKNOWN argument for > ports that are BR_STATE_DISABLED. Is that normal? This looks like another driver which has been converted to phylink without my review; I certainly wasn't aware of it. It gets a few things wrong, such as: 1) not checking state->interface in the validate callback - so it is basically saying that it can support any PHY interface mode that the kernel happens to support. 2) if phylink is configured to use in-band, then state->speed is undefined; this driver will fail. (If folk don't want to support that, we ought to have a way to tell phylink to reject anything that attempts to set it to in-band mode!) 3) it doesn't implement phylink_mac_link_state DSA ops, so it doesn't support SGMII or 802.3z phy interface modes (see 1). -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
