Re: [PATCH net-next v3 3/6] net: lan966x: add port module support

[Russell King (Oracle)]

On Wed, Nov 24, 2021 at 03:58:00PM +0100, Horatiu Vultur wrote:
> > 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.

> > 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!