| Message ID | 20220105031515.29276-6-luizluca@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: realtek: MDIO interface and RTL8367S | expand |
Luiz Angelo Daros de Luca <luizluca@gmail.com> writes: > The ds->ops->phy_read will only be used if the ds->slave_mii_bus > was not initialized. Calling realtek_smi_setup_mdio will create a > ds->slave_mii_bus, making ds->ops->phy_read dormant. > > Using ds->ops->phy_read will allow switches connected through non-SMI > interfaces (like mdio) to let ds allocate slave_mii_bus and reuse the > same code. > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk> > --- > drivers/net/dsa/realtek/realtek-smi.c | 6 ++++-- > drivers/net/dsa/realtek/realtek.h | 3 --- > drivers/net/dsa/realtek/rtl8365mb.c | 10 ++++++---- > drivers/net/dsa/realtek/rtl8366rb.c | 10 ++++++---- > 4 files changed, 16 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c > index 5514fe81d64f..1f024e2520a6 100644 > --- a/drivers/net/dsa/realtek/realtek-smi.c > +++ b/drivers/net/dsa/realtek/realtek-smi.c > @@ -329,16 +329,18 @@ static const struct regmap_config realtek_smi_mdio_regmap_config = { > static int realtek_smi_mdio_read(struct mii_bus *bus, int addr, int regnum) > { > struct realtek_priv *priv = bus->priv; > + struct dsa_switch *ds = priv->ds; > > - return priv->ops->phy_read(priv, addr, regnum); > + return ds->ops->phy_read(ds, addr, regnum); > } > > static int realtek_smi_mdio_write(struct mii_bus *bus, int addr, int regnum, > u16 val) > { > struct realtek_priv *priv = bus->priv; > + struct dsa_switch *ds = priv->ds; > > - return priv->ops->phy_write(priv, addr, regnum, val); > + return ds->ops->phy_write(ds, addr, regnum, val); > } > > static int realtek_smi_setup_mdio(struct dsa_switch *ds) > diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h > index 58814de563a2..a03de15c4a94 100644 > --- a/drivers/net/dsa/realtek/realtek.h > +++ b/drivers/net/dsa/realtek/realtek.h > @@ -103,9 +103,6 @@ struct realtek_ops { > int (*enable_vlan)(struct realtek_priv *priv, bool enable); > int (*enable_vlan4k)(struct realtek_priv *priv, bool enable); > int (*enable_port)(struct realtek_priv *priv, int port, bool enable); > - int (*phy_read)(struct realtek_priv *priv, int phy, int regnum); > - int (*phy_write)(struct realtek_priv *priv, int phy, int regnum, > - u16 val); > }; > > struct realtek_variant { > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c > index b52bb987027c..11a985900c57 100644 > --- a/drivers/net/dsa/realtek/rtl8365mb.c > +++ b/drivers/net/dsa/realtek/rtl8365mb.c > @@ -674,8 +674,9 @@ static int rtl8365mb_phy_ocp_write(struct realtek_priv *priv, int phy, > return 0; > } > > -static int rtl8365mb_phy_read(struct realtek_priv *priv, int phy, int regnum) > +static int rtl8365mb_phy_read(struct dsa_switch *ds, int phy, int regnum) > { > + struct realtek_priv *priv = ds->priv; > u32 ocp_addr; > u16 val; > int ret; > @@ -702,9 +703,10 @@ static int rtl8365mb_phy_read(struct realtek_priv *priv, int phy, int regnum) > return val; > } > > -static int rtl8365mb_phy_write(struct realtek_priv *priv, int phy, int regnum, > +static int rtl8365mb_phy_write(struct dsa_switch *ds, int phy, int regnum, > u16 val) > { > + struct realtek_priv *priv = (struct realtek_priv *)ds->priv; > u32 ocp_addr; > int ret; > > @@ -1958,6 +1960,8 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops = { > .get_tag_protocol = rtl8365mb_get_tag_protocol, > .setup = rtl8365mb_setup, > .teardown = rtl8365mb_teardown, > + .phy_read = rtl8365mb_phy_read, > + .phy_write = rtl8365mb_phy_write, > .phylink_validate = rtl8365mb_phylink_validate, > .phylink_mac_config = rtl8365mb_phylink_mac_config, > .phylink_mac_link_down = rtl8365mb_phylink_mac_link_down, > @@ -1974,8 +1978,6 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops = { > > static const struct realtek_ops rtl8365mb_ops = { > .detect = rtl8365mb_detect, > - .phy_read = rtl8365mb_phy_read, > - .phy_write = rtl8365mb_phy_write, > }; > > const struct realtek_variant rtl8365mb_variant = { > diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c > index ff607608dead..4576f9b797c5 100644 > --- a/drivers/net/dsa/realtek/rtl8366rb.c > +++ b/drivers/net/dsa/realtek/rtl8366rb.c > @@ -1641,8 +1641,9 @@ static int rtl8366rb_enable_vlan4k(struct realtek_priv *priv, bool enable) > enable ? RTL8366RB_SGCR_EN_VLAN_4KTB : 0); > } > > -static int rtl8366rb_phy_read(struct realtek_priv *priv, int phy, int regnum) > +static int rtl8366rb_phy_read(struct dsa_switch *ds, int phy, int regnum) > { > + struct realtek_priv *priv = ds->priv; > u32 val; > u32 reg; > int ret; > @@ -1675,9 +1676,10 @@ static int rtl8366rb_phy_read(struct realtek_priv *priv, int phy, int regnum) > return val; > } > > -static int rtl8366rb_phy_write(struct realtek_priv *priv, int phy, int regnum, > +static int rtl8366rb_phy_write(struct dsa_switch *ds, int phy, int regnum, > u16 val) > { > + struct realtek_priv *priv = ds->priv; > u32 reg; > int ret; > > @@ -1769,6 +1771,8 @@ static int rtl8366rb_detect(struct realtek_priv *priv) > static const struct dsa_switch_ops rtl8366rb_switch_ops = { > .get_tag_protocol = rtl8366_get_tag_protocol, > .setup = rtl8366rb_setup, > + .phy_read = rtl8366rb_phy_read, > + .phy_write = rtl8366rb_phy_write, > .phylink_mac_link_up = rtl8366rb_mac_link_up, > .phylink_mac_link_down = rtl8366rb_mac_link_down, > .get_strings = rtl8366_get_strings, > @@ -1801,8 +1805,6 @@ static const struct realtek_ops rtl8366rb_ops = { > .is_vlan_valid = rtl8366rb_is_vlan_valid, > .enable_vlan = rtl8366rb_enable_vlan, > .enable_vlan4k = rtl8366rb_enable_vlan4k, > - .phy_read = rtl8366rb_phy_read, > - .phy_write = rtl8366rb_phy_write, > }; > > const struct realtek_variant rtl8366rb_variant = {
On 1/4/2022 7:15 PM, Luiz Angelo Daros de Luca wrote: > The ds->ops->phy_read will only be used if the ds->slave_mii_bus > was not initialized. Calling realtek_smi_setup_mdio will create a > ds->slave_mii_bus, making ds->ops->phy_read dormant. > > Using ds->ops->phy_read will allow switches connected through non-SMI > interfaces (like mdio) to let ds allocate slave_mii_bus and reuse the > same code. > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Humm assigning dsa_switch_ops::phy_read will force DSA into tearing down the MDIO bus in dsa_switch_teardown() instead of letting your driver do it and since realtek-smi-core.c uses devm_mdiobus_unregister(), it is not clear to me what is going to happen but it sounds like a double free might happen? It seems more prudent to me to leave existing code.
> On 1/4/2022 7:15 PM, Luiz Angelo Daros de Luca wrote: > > The ds->ops->phy_read will only be used if the ds->slave_mii_bus > > was not initialized. Calling realtek_smi_setup_mdio will create a > > ds->slave_mii_bus, making ds->ops->phy_read dormant. > > > > Using ds->ops->phy_read will allow switches connected through non-SMI > > interfaces (like mdio) to let ds allocate slave_mii_bus and reuse the > > same code. > > > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > > Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Humm assigning dsa_switch_ops::phy_read will force DSA into tearing down > the MDIO bus in dsa_switch_teardown() instead of letting your driver do > it and since realtek-smi-core.c uses devm_mdiobus_unregister(), it is > not clear to me what is going to happen but it sounds like a double free > might happen? Thanks, Florian. You should be correct. It might call mdiobus_unregister() and mdiobus_free() twice, once inside the dsa code and another one by the devm (if I understood how devm functions work). The issue is that the dsa switch is assuming that if slave_mii is allocated and ds->ops->phy_read is defined, it has allocated the slave_mii by itself and it should clean up the slave_mii during teardown. That assumption came from commit 5135e96a3dd2f4555ae6981c3155a62bcf3227f6 "So I can only guess that no driver that implements ds->ops->phy_read also allocates and registers ds->slave_mii_bus itself.". If that is true, the condition during dsa_switch_setup() is not correct. During dsa_switch_setup(), if it does not fail, I know that ds->slave_mii_bus will be allocated, either by ds->ops->setup() or by itself. dsa_switch_setup() { .... ds->ops->setup() .... if (!ds->slave_mii_bus && ds->ops->phy_read) { ...allocate and register ds->slave_mii_bus... } } During the teardown, ds->slave_mii_bus will always be true (if not cleaning from an error before it was allocated). So, the test is really about having ds->ops->phy_read. dsa_switch_teardown() { ... if (ds->slave_mii_bus && ds->ops->phy_read) { ...unregister and free ds->slave_mii_bus... } ... ds->ops->teardown(); ... } As ds->ops->teardown() is called after slave_mii_bus is gone, there is no opportunity for ds->ops to clean the mii_slave_bus it might have allocated. It does not make sense for me to have those two "if" conditions working together. It should be either: dsa_switch_setup() { .... ds->ops->setup() .... if (ds->ops->phy_read) { if (ds->slave_mii_bus) error("ds->ops->phy_read is set, I should be the one allocating ds->slave_mii_bus!") ...allocate and register ds->slave_mii_bus... } } if "no driver that implements ds->ops->phy_read also allocates and registers ds->slave_mii_bus itself" or: dsa_switch_teardown() { ... if (ds->slave_mii_bus && "slave_mii_bus was allocated by myself") { ...unregister and free ds->slave_mii_bus... } ds->ops->teardown(); ... } if ds->ops->phy_read value should not tell if ds->slave_mii_bus should be cleaned by the DSA switch. I would selfishly hope the correct one was the second option because it would make my code much cleaner. If not, that's a complex issue to solve without lots of duplications: realtek-smi drivers should not have ds->ops->phy_read defined while realtek-mdio requires it. I'll need to duplicate dsa_switch_ops for each subdriver only to unset phy_read and also duplicate realtek_variant for each interface only to reference that different dsa_switch_ops. BTW, the realtek-smi calls of_node_put(priv->slave_mii_bus->dev.of_node) during shutdown while other dsa drivers do not seem to care. Wouldn't devm controls be enough for cleaning that mii_bus? Even if not, wouldn't the ds->ops->teardown be the correct place for that cleanup and not realtek_smi_remove()? > It seems more prudent to me to leave existing code. As I mentioned, It would require a good amount of duplications. But I'll do what needs to be done. Regards, Luiz
> Thanks, Florian. You should be correct. It might call > mdiobus_unregister() and mdiobus_free() twice, once inside the dsa > code and another one by the devm (if I understood how devm functions > work). > > The issue is that the dsa switch is assuming that if slave_mii is > allocated and ds->ops->phy_read is defined, it has allocated the > slave_mii by itself and it should clean up the slave_mii during > teardown. Correct. Either the DSA core takes care of the mdiobus and uses the phy_read and phy_write ops, or the driver internally registers its own mdiobus, and phy_read and phy_write ops are not implemented. The core is not designed to mix those together. > if ds->ops->phy_read value should not tell if ds->slave_mii_bus should > be cleaned by the DSA switch. > > I would selfishly hope the correct one was the second option because > it would make my code much cleaner. If not, that's a complex issue to > solve without lots of duplications: realtek-smi drivers should not > have ds->ops->phy_read defined while realtek-mdio requires it. I'll > need to duplicate dsa_switch_ops for each subdriver only to unset > phy_read and also duplicate realtek_variant for each interface only to > reference that different dsa_switch_ops. One option would be to provide a dummy mdiobus driver for realtek-mdio, which simply passes the access through to the existing MDIO bus. Andrew
--- > > Thanks, Florian. You should be correct. It might call > > mdiobus_unregister() and mdiobus_free() twice, once inside the dsa > > code and another one by the devm (if I understood how devm functions > > work). > > > > The issue is that the dsa switch is assuming that if slave_mii is > > allocated and ds->ops->phy_read is defined, it has allocated the > > slave_mii by itself and it should clean up the slave_mii during > > teardown. > > Correct. Either the DSA core takes care of the mdiobus and uses the > phy_read and phy_write ops, or the driver internally registers its own > mdiobus, and phy_read and phy_write ops are not implemented. The core > is not designed to mix those together. > > > if ds->ops->phy_read value should not tell if ds->slave_mii_bus should > > be cleaned by the DSA switch. > > > > I would selfishly hope the correct one was the second option because > > it would make my code much cleaner. If not, that's a complex issue to > > solve without lots of duplications: realtek-smi drivers should not > > have ds->ops->phy_read defined while realtek-mdio requires it. I'll > > need to duplicate dsa_switch_ops for each subdriver only to unset > > phy_read and also duplicate realtek_variant for each interface only to > > reference that different dsa_switch_ops. > > One option would be to provide a dummy mdiobus driver for > realtek-mdio, which simply passes the access through to the existing > MDIO bus. > > Andrew Ok, thanks for the clarification, Andrew. In the end, I simply duplicated ds_ops and wrote a wrap funcion. Much less painful than I anticipated. Should I submit a patch to make dsa work like this then? dsa_switch_setup() { .... ds->ops->setup() .... if (ds->ops->phy_read) { if (ds->slave_mii_bus) error("ds->ops->phy_read is set, I should be the one allocating ds->slave_mii_bus!") ...allocate and register ds->slave_mii_bus... } } I think it is better to fail when there is an invalid setup than to expect the dsa driver to behave correctly. Regards, Luiz
> Should I submit a patch to make dsa work like this then? > > dsa_switch_setup() { > .... > ds->ops->setup() > .... > if (ds->ops->phy_read) { > if (ds->slave_mii_bus) > error("ds->ops->phy_read is set, I should be the > one allocating ds->slave_mii_bus!") > ...allocate and register ds->slave_mii_bus... > } > } You could add a WARN_ON(ds->ops->phy_read && ds->ops->phy_read); Andrew
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c index 5514fe81d64f..1f024e2520a6 100644 --- a/drivers/net/dsa/realtek/realtek-smi.c +++ b/drivers/net/dsa/realtek/realtek-smi.c @@ -329,16 +329,18 @@ static const struct regmap_config realtek_smi_mdio_regmap_config = { static int realtek_smi_mdio_read(struct mii_bus *bus, int addr, int regnum) { struct realtek_priv *priv = bus->priv; + struct dsa_switch *ds = priv->ds; - return priv->ops->phy_read(priv, addr, regnum); + return ds->ops->phy_read(ds, addr, regnum); } static int realtek_smi_mdio_write(struct mii_bus *bus, int addr, int regnum, u16 val) { struct realtek_priv *priv = bus->priv; + struct dsa_switch *ds = priv->ds; - return priv->ops->phy_write(priv, addr, regnum, val); + return ds->ops->phy_write(ds, addr, regnum, val); } static int realtek_smi_setup_mdio(struct dsa_switch *ds) diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h index 58814de563a2..a03de15c4a94 100644 --- a/drivers/net/dsa/realtek/realtek.h +++ b/drivers/net/dsa/realtek/realtek.h @@ -103,9 +103,6 @@ struct realtek_ops { int (*enable_vlan)(struct realtek_priv *priv, bool enable); int (*enable_vlan4k)(struct realtek_priv *priv, bool enable); int (*enable_port)(struct realtek_priv *priv, int port, bool enable); - int (*phy_read)(struct realtek_priv *priv, int phy, int regnum); - int (*phy_write)(struct realtek_priv *priv, int phy, int regnum, - u16 val); }; struct realtek_variant { diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c index b52bb987027c..11a985900c57 100644 --- a/drivers/net/dsa/realtek/rtl8365mb.c +++ b/drivers/net/dsa/realtek/rtl8365mb.c @@ -674,8 +674,9 @@ static int rtl8365mb_phy_ocp_write(struct realtek_priv *priv, int phy, return 0; } -static int rtl8365mb_phy_read(struct realtek_priv *priv, int phy, int regnum) +static int rtl8365mb_phy_read(struct dsa_switch *ds, int phy, int regnum) { + struct realtek_priv *priv = ds->priv; u32 ocp_addr; u16 val; int ret; @@ -702,9 +703,10 @@ static int rtl8365mb_phy_read(struct realtek_priv *priv, int phy, int regnum) return val; } -static int rtl8365mb_phy_write(struct realtek_priv *priv, int phy, int regnum, +static int rtl8365mb_phy_write(struct dsa_switch *ds, int phy, int regnum, u16 val) { + struct realtek_priv *priv = (struct realtek_priv *)ds->priv; u32 ocp_addr; int ret; @@ -1958,6 +1960,8 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops = { .get_tag_protocol = rtl8365mb_get_tag_protocol, .setup = rtl8365mb_setup, .teardown = rtl8365mb_teardown, + .phy_read = rtl8365mb_phy_read, + .phy_write = rtl8365mb_phy_write, .phylink_validate = rtl8365mb_phylink_validate, .phylink_mac_config = rtl8365mb_phylink_mac_config, .phylink_mac_link_down = rtl8365mb_phylink_mac_link_down, @@ -1974,8 +1978,6 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops = { static const struct realtek_ops rtl8365mb_ops = { .detect = rtl8365mb_detect, - .phy_read = rtl8365mb_phy_read, - .phy_write = rtl8365mb_phy_write, }; const struct realtek_variant rtl8365mb_variant = { diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c index ff607608dead..4576f9b797c5 100644 --- a/drivers/net/dsa/realtek/rtl8366rb.c +++ b/drivers/net/dsa/realtek/rtl8366rb.c @@ -1641,8 +1641,9 @@ static int rtl8366rb_enable_vlan4k(struct realtek_priv *priv, bool enable) enable ? RTL8366RB_SGCR_EN_VLAN_4KTB : 0); } -static int rtl8366rb_phy_read(struct realtek_priv *priv, int phy, int regnum) +static int rtl8366rb_phy_read(struct dsa_switch *ds, int phy, int regnum) { + struct realtek_priv *priv = ds->priv; u32 val; u32 reg; int ret; @@ -1675,9 +1676,10 @@ static int rtl8366rb_phy_read(struct realtek_priv *priv, int phy, int regnum) return val; } -static int rtl8366rb_phy_write(struct realtek_priv *priv, int phy, int regnum, +static int rtl8366rb_phy_write(struct dsa_switch *ds, int phy, int regnum, u16 val) { + struct realtek_priv *priv = ds->priv; u32 reg; int ret; @@ -1769,6 +1771,8 @@ static int rtl8366rb_detect(struct realtek_priv *priv) static const struct dsa_switch_ops rtl8366rb_switch_ops = { .get_tag_protocol = rtl8366_get_tag_protocol, .setup = rtl8366rb_setup, + .phy_read = rtl8366rb_phy_read, + .phy_write = rtl8366rb_phy_write, .phylink_mac_link_up = rtl8366rb_mac_link_up, .phylink_mac_link_down = rtl8366rb_mac_link_down, .get_strings = rtl8366_get_strings, @@ -1801,8 +1805,6 @@ static const struct realtek_ops rtl8366rb_ops = { .is_vlan_valid = rtl8366rb_is_vlan_valid, .enable_vlan = rtl8366rb_enable_vlan, .enable_vlan4k = rtl8366rb_enable_vlan4k, - .phy_read = rtl8366rb_phy_read, - .phy_write = rtl8366rb_phy_write, }; const struct realtek_variant rtl8366rb_variant = {