| Message ID | E1swKNV-0060oN-1b@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 539770616521e5b046ca7612eb79ba11b53edb1d |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] net: dsa: remove obsolete phylink dsa_switch operations | expand |
On Thu, Oct 03, 2024 at 12:52:17PM +0100, Russell King (Oracle) wrote: > No driver now uses the DSA switch phylink members, so we can now remove > the method pointers, but we need to leave empty shim functions to allow > those drivers that do not provide phylink MAC operations structure to > continue functioning. > > Signed-off-by: Russell King (oracle) <rmk+kernel@armlinux.org.uk> > --- > diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c > index 668c729946ea..09d2f5d4b3dd 100644 > --- a/net/dsa/dsa.c > +++ b/net/dsa/dsa.c > diff --git a/net/dsa/port.c b/net/dsa/port.c > index 25258b33e59e..f1e96706a701 100644 > --- a/net/dsa/port.c > +++ b/net/dsa/port.c > @@ -1579,40 +1579,19 @@ static struct phylink_pcs * > dsa_port_phylink_mac_select_pcs(struct phylink_config *config, > phy_interface_t interface) > { > - struct dsa_port *dp = dsa_phylink_to_port(config); > - struct phylink_pcs *pcs = ERR_PTR(-EOPNOTSUPP); > - struct dsa_switch *ds = dp->ds; > - > - if (ds->ops->phylink_mac_select_pcs) > - pcs = ds->ops->phylink_mac_select_pcs(ds, dp->index, interface); > - > - return pcs; > + return ERR_PTR(-EOPNOTSUPP); > } dsa_port_phylink_mac_select_pcs() didn't have to stay, as phylink_mac_select_pcs() is entirely optional in phylink. Otherwise: Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com> # sja1105, felix, dsa_loop
On Thu, Oct 03, 2024 at 05:51:03PM +0300, Vladimir Oltean wrote: > On Thu, Oct 03, 2024 at 12:52:17PM +0100, Russell King (Oracle) wrote: > > No driver now uses the DSA switch phylink members, so we can now remove > > the method pointers, but we need to leave empty shim functions to allow > > those drivers that do not provide phylink MAC operations structure to > > continue functioning. > > > > Signed-off-by: Russell King (oracle) <rmk+kernel@armlinux.org.uk> > > --- > > diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c > > index 668c729946ea..09d2f5d4b3dd 100644 > > --- a/net/dsa/dsa.c > > +++ b/net/dsa/dsa.c > > diff --git a/net/dsa/port.c b/net/dsa/port.c > > index 25258b33e59e..f1e96706a701 100644 > > --- a/net/dsa/port.c > > +++ b/net/dsa/port.c > > @@ -1579,40 +1579,19 @@ static struct phylink_pcs * > > dsa_port_phylink_mac_select_pcs(struct phylink_config *config, > > phy_interface_t interface) > > { > > - struct dsa_port *dp = dsa_phylink_to_port(config); > > - struct phylink_pcs *pcs = ERR_PTR(-EOPNOTSUPP); > > - struct dsa_switch *ds = dp->ds; > > - > > - if (ds->ops->phylink_mac_select_pcs) > > - pcs = ds->ops->phylink_mac_select_pcs(ds, dp->index, interface); > > - > > - return pcs; > > + return ERR_PTR(-EOPNOTSUPP); > > } > > dsa_port_phylink_mac_select_pcs() didn't have to stay, as phylink_mac_select_pcs() > is entirely optional in phylink. Otherwise: Yes, that's correct, but let's keep it to this for the moment. There's more to do with mac_select_pcs(). When it was introdued, we needed a way to distinguish whether the method was actually implemented or whether the old phylink_set_pcs() function was being used. Those days are long gone, so returning ERR_PTR(-EOPNOTSUPP) no longer makes much sense. DSA's core code returns this, as does mv88e6xxx when the chip doesn't have any pcs_ops (I'm not sure now why I did the latter now.) So, I'd like to (a) make mv88e6xxx_mac_select_pcs() return NULL, then kill dsa_port_phylink_mac_select_pcs() and then eliminate: if (mac_ops->mac_select_pcs && mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) != ERR_PTR(-EOPNOTSUPP)) using_mac_select_pcs = true; replacing all other cases of pl->using_mac_select_pcs with a test for pl->mac_ops->mac_select_pcs being non-NULL. However, that's for later - I think for this patch, it makes sense to keep returning the ERR_PTR() value because that's what it was doing prior to this patch - we're then only removing the members on the dsa_switch_ops and their callsite in this patch.
On Thu, Oct 03, 2024 at 04:28:57PM +0100, Russell King (Oracle) wrote: > Yes, that's correct, but let's keep it to this for the moment. > > There's more to do with mac_select_pcs(). When it was introdued, we > needed a way to distinguish whether the method was actually implemented > or whether the old phylink_set_pcs() function was being used. Those days > are long gone, so returning ERR_PTR(-EOPNOTSUPP) no longer makes much > sense. > > DSA's core code returns this, as does mv88e6xxx when the chip doesn't > have any pcs_ops (I'm not sure now why I did the latter now.) > > So, I'd like to (a) make mv88e6xxx_mac_select_pcs() return NULL, then > kill dsa_port_phylink_mac_select_pcs() and then eliminate: > > if (mac_ops->mac_select_pcs && > mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) != > ERR_PTR(-EOPNOTSUPP)) > using_mac_select_pcs = true; > > replacing all other cases of pl->using_mac_select_pcs with a test > for pl->mac_ops->mac_select_pcs being non-NULL. > > However, that's for later - I think for this patch, it makes sense > to keep returning the ERR_PTR() value because that's what it was > doing prior to this patch - we're then only removing the members > on the dsa_switch_ops and their callsite in this patch. I was about to write the same thing earlier, except I stopped when I realized I also had no idea why mv88e6xxx_mac_select_pcs() returns ERR_PTR(-EOPNOTSUPP). The removal of using_mac_select_pcs is "further work", and deleting dsa_port_phylink_mac_select_pcs() altogether, although it can also be done now, can be grouped with that.
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Thu, 03 Oct 2024 12:52:17 +0100 you wrote: > No driver now uses the DSA switch phylink members, so we can now remove > the method pointers, but we need to leave empty shim functions to allow > those drivers that do not provide phylink MAC operations structure to > continue functioning. > > Signed-off-by: Russell King (oracle) <rmk+kernel@armlinux.org.uk> > > [...] Here is the summary with links: - [net-next,v2] net: dsa: remove obsolete phylink dsa_switch operations https://git.kernel.org/netdev/net-next/c/539770616521 You are awesome, thank you!
diff --git a/include/net/dsa.h b/include/net/dsa.h index d7a6c2930277..72ae65e7246a 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -885,21 +885,6 @@ struct dsa_switch_ops { */ void (*phylink_get_caps)(struct dsa_switch *ds, int port, struct phylink_config *config); - struct phylink_pcs *(*phylink_mac_select_pcs)(struct dsa_switch *ds, - int port, - phy_interface_t iface); - void (*phylink_mac_config)(struct dsa_switch *ds, int port, - unsigned int mode, - const struct phylink_link_state *state); - void (*phylink_mac_link_down)(struct dsa_switch *ds, int port, - unsigned int mode, - phy_interface_t interface); - void (*phylink_mac_link_up)(struct dsa_switch *ds, int port, - unsigned int mode, - phy_interface_t interface, - struct phy_device *phydev, - int speed, int duplex, - bool tx_pause, bool rx_pause); void (*phylink_fixed_state)(struct dsa_switch *ds, int port, struct phylink_link_state *state); /* diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c index 668c729946ea..09d2f5d4b3dd 100644 --- a/net/dsa/dsa.c +++ b/net/dsa/dsa.c @@ -1505,14 +1505,6 @@ static int dsa_switch_probe(struct dsa_switch *ds) if (!ds->num_ports) return -EINVAL; - if (ds->phylink_mac_ops) { - if (ds->ops->phylink_mac_select_pcs || - ds->ops->phylink_mac_config || - ds->ops->phylink_mac_link_down || - ds->ops->phylink_mac_link_up) - return -EINVAL; - } - if (np) { err = dsa_switch_parse_of(ds, np); if (err) diff --git a/net/dsa/port.c b/net/dsa/port.c index 25258b33e59e..f1e96706a701 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -1579,40 +1579,19 @@ static struct phylink_pcs * dsa_port_phylink_mac_select_pcs(struct phylink_config *config, phy_interface_t interface) { - struct dsa_port *dp = dsa_phylink_to_port(config); - struct phylink_pcs *pcs = ERR_PTR(-EOPNOTSUPP); - struct dsa_switch *ds = dp->ds; - - if (ds->ops->phylink_mac_select_pcs) - pcs = ds->ops->phylink_mac_select_pcs(ds, dp->index, interface); - - return pcs; + return ERR_PTR(-EOPNOTSUPP); } static void dsa_port_phylink_mac_config(struct phylink_config *config, unsigned int mode, const struct phylink_link_state *state) { - struct dsa_port *dp = dsa_phylink_to_port(config); - struct dsa_switch *ds = dp->ds; - - if (!ds->ops->phylink_mac_config) - return; - - ds->ops->phylink_mac_config(ds, dp->index, mode, state); } static void dsa_port_phylink_mac_link_down(struct phylink_config *config, unsigned int mode, phy_interface_t interface) { - struct dsa_port *dp = dsa_phylink_to_port(config); - struct dsa_switch *ds = dp->ds; - - if (!ds->ops->phylink_mac_link_down) - return; - - ds->ops->phylink_mac_link_down(ds, dp->index, mode, interface); } static void dsa_port_phylink_mac_link_up(struct phylink_config *config, @@ -1622,14 +1601,6 @@ static void dsa_port_phylink_mac_link_up(struct phylink_config *config, int speed, int duplex, bool tx_pause, bool rx_pause) { - struct dsa_port *dp = dsa_phylink_to_port(config); - struct dsa_switch *ds = dp->ds; - - if (!ds->ops->phylink_mac_link_up) - return; - - ds->ops->phylink_mac_link_up(ds, dp->index, mode, interface, phydev, - speed, duplex, tx_pause, rx_pause); } static const struct phylink_mac_ops dsa_port_phylink_mac_ops = { @@ -1871,9 +1842,6 @@ static void dsa_shared_port_link_down(struct dsa_port *dp) if (ds->phylink_mac_ops && ds->phylink_mac_ops->mac_link_down) ds->phylink_mac_ops->mac_link_down(&dp->pl_config, MLO_AN_FIXED, PHY_INTERFACE_MODE_NA); - else if (ds->ops->phylink_mac_link_down) - ds->ops->phylink_mac_link_down(ds, dp->index, MLO_AN_FIXED, - PHY_INTERFACE_MODE_NA); } int dsa_shared_port_link_register_of(struct dsa_port *dp)
No driver now uses the DSA switch phylink members, so we can now remove the method pointers, but we need to leave empty shim functions to allow those drivers that do not provide phylink MAC operations structure to continue functioning. Signed-off-by: Russell King (oracle) <rmk+kernel@armlinux.org.uk> --- Changes since v1 (a while ago): - don't remove the shim functions and ops structure which will break some DSA drivers that do not provide a phylink_mac_ops struct as pointed out by Vladimir (thanks.) include/net/dsa.h | 15 --------------- net/dsa/dsa.c | 8 -------- net/dsa/port.c | 34 +--------------------------------- 3 files changed, 1 insertion(+), 56 deletions(-)