Re: [PATCH net-next 06/15] net: dsa: mt7530: do not set priv->p5_interface on mt7530_setup_port5()

["Arınç ÜNAL" <arinc.unal@arinc9.com>]

On 7.12.2023 21:18, Vladimir Oltean wrote:
> On Sat, Nov 18, 2023 at 03:31:56PM +0300, Arınç ÜNAL wrote:
>> Do not set priv->p5_interface on mt7530_setup_port5(). There isn't a case
>> where mt753x_phylink_mac_config() runs after mt7530_setup_port5() which
>> setting priv->p5_interface would prevent mt7530_setup_port5() from running
>> more than once.
>>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
>>   drivers/net/dsa/mt7530.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 069b3dfca6fa..fc87ec817672 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -978,8 +978,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
>>   	dev_dbg(ds->dev, "Setup P5, HWTRAP=0x%x, intf_sel=%s, phy-mode=%s\n",
>>   		val, p5_intf_modes(priv->p5_intf_sel), phy_modes(interface));
>>   
>> -	priv->p5_interface = interface;
>> -
>>   unlock_exit:
>>   	mutex_unlock(&priv->reg_mutex);
>>   }
>> -- 
>> 2.40.1
>>
> 
> Purely as a matter of theory, mt753x_phylink_mac_config() itself could
> be called more than once, at any time there is a phylink_major_config() -
> first during initial one, then upon any change of the phy_interface_t.
> With the port being fixed and internal, maybe that is unlikely.
> 
> Destroying and re-creating the phylink instance could also make the
> driver see more than 1 mt753x_phylink_mac_config() call. During periods
> of -EPROBE_DEFER, maybe this could even happen.
> 
> I think what we need to see is a description of what the priv->p5_interface
> test is really protecting against, because it certainly is uncommon in other
> drivers to have this much logic to avoid mt753x_mac_config() being
> called twice.
> 
> If there's nothing wrong with calling it twice and it's just an optimization,
> I think that's enough of a justification for removing that extra protection.
> At the same time, the optimization (and simplification) that we should
> pursue is that we should remove the overlap between phylink and the
> initial port setup made by the driver.

priv->p5_interface and priv->p6_interface were actually intended to apply
only for MT7531. It prevents the CPU ports to be configured again. CPU
ports are unnecessarily configured before phylink. I intend to address that
with the patch below. I'll submit it after the current patches here go
through. There's a lot to clean up in this driver.

https://github.com/arinc9/linux/commit/80efcb1870530ef5526d7feda625374b8f308369

If you can recall, this is where me and Russell mostly left off on my 30
patch series. I was supposed to run some debug info prints to make sure
that the MT7531 CPU ports are still configured as they were with
priv->info->cpu_port_config().

https://lore.kernel.org/netdev/ZHy2jQLesdYFMQtO@shell.armlinux.org.uk/

For this patch, I can change the patch log to state that priv->p5_interface
and priv->p6_interface are intended for use on the MT7531 switch.

Arınç