| Message ID | 20231118123205.266819-7-arinc.unal@arinc9.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | MT7530 DSA subdriver improvements | expand |
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.
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ç
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); }
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(-)