Re: [PATCH net-next 08/30] net: dsa: mt7530: change p{5,6}_interface to p{5,6}_configured

[Russell King (Oracle)]

On Sun, Jun 04, 2023 at 01:46:46PM +0300, Arınç ÜNAL wrote:
> On 3.06.2023 15:26, Russell King (Oracle) wrote:
> > Given that, you should have no need to make explicit calls to your
> > mac_config, pcs_link_up and mac_link_up functions. If you need to
> > make these calls, it suggests that phylink is not being used for the
> > CPU port.
> 
> Your own commit does this so I don't know what to tell you.
> 
> https://github.com/torvalds/linux/commit/cbd1f243bc41056c76fcfc5f3380cfac1f00d37b

I would like to make a comment here to explain why in that commit I
added a call into mt7531_cpu_port_config() to mt7531_cpu_port_config().

When I'm making changes to drivers, then I follow a golden rule: do
not change the behaviour unless it is an intentional change.

This is exactly what is going on in this commit.
mt7531_cpu_port_config() called mt753x_phylink_mac_link_up(), which
then, as the very first thing, called mt753x_mac_pcs_link_up().
mt753x_mac_pcs_link_up() called priv->info->mac_pcs_link_up() if
it is defined.

Since converting to phylink_pcs involves the removal of the direct
call from mt753x_phylink_mac_link_up() to the
priv->info->mac_pcs_link_up() method, in order to preserve the
behaviour of the driver, it is necessary to ensure that
mt7531_cpu_port_config() still makes that call.

mt753x_phylink_pcs_link_up() is the new function replacing
mt753x_mac_pcs_link_up() which makes that call, so it is entirely
appropriate to add that call into mt7531_cpu_port_config() so that
mt7531_cpu_port_config() behaves _precisely_ the same as it did
before and after this change.

In that sense, as far as mt7531_cpu_port_config() is concerned, this
change is entirely idempotent.

I don't know whether mt7531_cpu_port_config() is necessary to properly
bring up a CPU port, or whether _all_ firmware descriptions for
mt7531 include all the necessary properties so that DSA will always
bring up a phylink instance for the CPU port - that really is not
relevant for the change you point out.

What is relevant is only making the intended change, and the intended
change is to split the PCS-specific code from the MAC-specific code.

This principle of only making one change in a patch, and ensuring that
parts of the code which merely need to be re-organised to achieve that
change are done in an idempotent way is fundamental to good code
maintenance, especially when modifying drivers that one does not have
the hardware to be able to test.

You have provided new information - that basically indicates that
phylink is used for your setup. If we can get to a position where we
can confidently say that phylink will always be used for the CPU port,
the code in mt7531_cpu_port_config() that bypasses phylink, calling
methods in phylink's mac_ops and pcs_ops, should be removed.

I don't remember whether Vladimir's firmware validator will fail for
mt753x if CPU ports are not fully described, but that would be well
worth checking. If it does, then we can be confident that phylink
will always be used, and those bypassing calls should not be necessary.

I hope this explains the situation better.

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