Re: [PATCH net-next V1 6/7] net: lan743x: Add support to the phylink framework

[Russell King (Oracle)]

On Tue, Oct 17, 2023 at 03:12:07PM +0530, Raju Lakkaraju wrote:
> +static void lan743x_phylink_mac_config(struct phylink_config *config,
> +				       unsigned int link_an_mode,
> +				       const struct phylink_link_state *state)
> +{
> +	struct net_device *netdev = to_net_dev(config->dev);
> +	struct lan743x_adapter *adapter = netdev_priv(netdev);
> +	bool status;
> +	int ret;
> +
> +	lan743x_mac_cfg_update(adapter, state->link, state->speed,
> +			       state->advertising);

Another case of not reading the phylink documentation... :( I *really*
don't see why we bother to write documentation, from my experience, it
seems to be totally a write-only thing... no one seems to bother
reading it.

/**
 * mac_config() - configure the MAC for the selected mode and state
 * @config: a pointer to a &struct phylink_config.
 * @mode: one of %MLO_AN_FIXED, %MLO_AN_PHY, %MLO_AN_INBAND.
 * @state: a pointer to a &struct phylink_link_state.
 *
 * Note - not all members of @state are valid.  In particular,
 * @state->lp_advertising, @state->link, @state->an_complete are never
 * guaranteed to be correct, and so any mac_config() implementation must
 * never reference these fields.
...
 * The action performed depends on the currently selected mode:
 *
 * %MLO_AN_FIXED, %MLO_AN_PHY:
...
 *   Valid state members: interface, advertising.
 *   Deprecated state members: speed, duplex, pause.
...
 * %MLO_AN_INBAND:
...
 *   Valid state members: interface, an_enabled, pause, advertising.

So "link" and "speed" are not valid. They're certainly not valid now
that the pre-March 2020 legacy support has been removed.

The only time that you get to know what speed the link is operating at
is after the link has been negotiated and is up - and you will be told
via the mac_link_up() function (or in the case of a PCS, via the
pcs_link_up() function.) You can't know before that point what speed
the link will be operating at because its dependent on the results of
link negotiation.

I haven't bothered to look more deeply at this patch, because I regard
any patch that gets this wrong to be utter trash (sorry that's a bit
hard, but I see people not bothering to read documentation all too
often and it pisses me off that I've gone to the bother of writing it
and it just gets ignored.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!