On Wed, Nov 24, 2021 at 03:58:00PM +0100, Horatiu Vultur wrote:

quoted

This doesn't look like the correct sequence to me. Shouldn't the net
device be unregistered first, which will take the port down by doing
so and make it unavailable to userspace to further manipulate. Then
we should start tearing other stuff down such as destroying phylink
and disabling interrupts (in the caller of this.)

I can change the order as you suggested.
Regarding the interrupts, shouldn't they be first disable and then do
all the teardown?

Depends if you need them disabled before you do the teardown. However,
what would be the effect of disabling interrupts while the user still
has the ability to interact with the port - that is the main point.

Generally the teardown should be the reverse of setup - where it's now
accepted that all setup should be done prior to user publication. So,
user interfaces should be removed and then teardown should proceed.

quoted

What is the difference between "portmode" and "phy_mode"? Does it matter
if port->config.phy_mode get zeroed when lan966x_port_pcs_set() is
called from lan966x_pcs_config()? It looks to me like the first call
will clear phy_mode, setting it to PHY_INTERFACE_MODE_NA from that point
on.

The purpose was to use portmode to configure the MAC and the phy_mode
to configure the serdes. There are small issues regarding this which
will be fix in the next series also I will add some comments just to
make it clear.

Actually, port->config.phy_mode will not get zeroed. Because right after
the memset it follows: 'config = port->config'.

Ah, missed that, thanks. However, why should portmode and phy_mode be
different?

Actually, like you mentioned it needs to be link partner's advertisement
so that code can be simplified more:

         if (DEV_PCS1G_ANEG_STATUS_ANEG_COMPLETE_GET(val)) {
                 state->an_complete = true;
 
                 bmsr |= state->link ? BMSR_LSTATUS : 0;
                 bmsr |= BMSR_ANEGCOMPLETE;
 
                 lp_adv = DEV_PCS1G_ANEG_STATUS_LP_ADV_GET(val);
                 phylink_mii_c22_pcs_decode_state(state, bmsr, lp_adv);
         }

Because inside phylink_mii_c22_pcs_decode_state, more precisely in
phylink_decode_c37_work, state->advertising will have the local
advertising.

Correct.

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