Re: [PATCH netnext 8/8] net: dsa: mt7530: simplify link operations and force link down on all ports

[Arınç ÜNAL]

On 16.02.2024 18:43, Russell King (Oracle) wrote:
> On Thu, Feb 08, 2024 at 08:51:36AM +0300, Arınç ÜNAL via B4 Relay wrote:
> > From: Arınç ÜNAL <arinc.unal@xxxxxxxxxx>
> >
> > Currently, the link operations for switch MACs are scattered across
> > port_enable, port_disable, phylink_mac_config, phylink_mac_link_up, and
> > phylink_mac_link_down.
> >
> > port_enable and port_disable clears the link settings. Move that to
> > mt7530_setup() and mt7531_setup_common() which set up the switches. This
> > way, the link settings are cleared on all ports at setup, and then only
> > once with phylink_mac_link_down() when a link goes down.
> >
> > Enable force mode at setup to apply the force part of the link settings.
> > This ensures that only active ports will have their link up.
>
> I think we may have a different interpretation of what phylink's
> mac_link_down() and mac_link_up() are supposed to be doing here.
> Of course, you have read the documentation of these methods so are
> fully aware of what they're supposed to do. So you are aware that
> when inband mode is being used, forcing the link down may be
> counter-productive depending on how the hardware works.

No, I haven't read the documentation [1] until your response here. My patch
here doesn't touch mt753x_phylink_mac_link_down(), it is already the case
with the driver that falls short of not forcing link down when inband mode
is being used.

That said, what I explain on the patch log does not properly describe the
driver behaviour. This should explain it correctly:

Enable force mode at setup to apply the force part of the link settings.
This ensures that the ports that were never active will have their link
down.

I could, in the future, study this thoroughly to make the driver fully
conform to the documentation.

[1] https://docs.kernel.org/networking/kapi.html#phylink

Arınç