| Message ID | 20190523011958.14944-8-ioana.ciornei@nxp.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | Decoupling PHYLINK from struct net_device | expand |
On 5/22/2019 6:20 PM, Ioana Ciornei wrote: > In order to have a common handling of PHYLINK for the slave and non-user > ports, the DSA core glue logic (between PHYLINK and the driver) must use > an API that does not rely on a struct net_device. > > These will also be called by the CPU-port-handling code in a further > patch. > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > Suggested-by: Vladimir Oltean <olteanv@gmail.com> > --- [snip] > +void dsa_port_phylink_validate(struct dsa_port *dp, > + unsigned long *supported, > + struct phylink_link_state *state) > +{ > + struct dsa_switch *ds = dp->ds; > + > + if (!ds->ops->phylink_validate) > + return; > + > + ds->ops->phylink_validate(ds, dp->index, supported, state); > +} > +EXPORT_SYMBOL(dsa_port_phylink_validate); Those exports should probably be _GPL to follow the PHYLINK exports but other than that: Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
On Thu, May 23, 2019 at 01:20:41AM +0000, Ioana Ciornei wrote: > diff --git a/net/dsa/port.c b/net/dsa/port.c > index ed8ba9daa3ba..d0f955e8b731 100644 > --- a/net/dsa/port.c > +++ b/net/dsa/port.c > @@ -422,6 +422,102 @@ static struct phy_device *dsa_port_get_phy_device(struct dsa_port *dp) > return phydev; > } > > +void dsa_port_phylink_validate(struct dsa_port *dp, > + unsigned long *supported, > + struct phylink_link_state *state) > +{ > + struct dsa_switch *ds = dp->ds; > + > + if (!ds->ops->phylink_validate) > + return; No, really not acceptable. If there's no phylink_validate function, then simply returning is going to produce what I'd term "unpredictable" results. This callback will be called with various modes in "supported", and if you simply return without any modification, you're basically saying that you support _anything_ that the supported mask throws at you. > + > + ds->ops->phylink_validate(ds, dp->index, supported, state); > +} > +EXPORT_SYMBOL(dsa_port_phylink_validate); > + > +int dsa_port_phylink_mac_link_state(struct dsa_port *dp, > + struct phylink_link_state *state) > +{ > + struct dsa_switch *ds = dp->ds; > + > + /* Only called for SGMII and 802.3z */ > + if (!ds->ops->phylink_mac_link_state) > + return -EOPNOTSUPP; > + > + return ds->ops->phylink_mac_link_state(ds, dp->index, state); > +} > +EXPORT_SYMBOL(dsa_port_phylink_mac_link_state); > + > +void dsa_port_phylink_mac_config(struct dsa_port *dp, > + unsigned int mode, > + const struct phylink_link_state *state) > +{ > + struct dsa_switch *ds = dp->ds; > + > + if (!ds->ops->phylink_mac_config) > + return; If you don't implement this, what's the point? > + > + ds->ops->phylink_mac_config(ds, dp->index, mode, state); > +} > +EXPORT_SYMBOL(dsa_port_phylink_mac_config); > + > +void dsa_port_phylink_mac_an_restart(struct dsa_port *dp) > +{ > + struct dsa_switch *ds = dp->ds; > + > + if (!ds->ops->phylink_mac_an_restart) > + return; > + > + ds->ops->phylink_mac_an_restart(ds, dp->index); > +} > +EXPORT_SYMBOL(dsa_port_phylink_mac_an_restart); > + > +void dsa_port_phylink_mac_link_down(struct dsa_port *dp, > + unsigned int mode, > + phy_interface_t interface, > + struct phy_device *phydev) > +{ > + struct dsa_switch *ds = dp->ds; > + > + if (!ds->ops->phylink_mac_link_down) { > + if (ds->ops->adjust_link && phydev) > + ds->ops->adjust_link(ds, dp->index, phydev); > + return; > + } > + > + ds->ops->phylink_mac_link_down(ds, dp->index, mode, interface); > +} > +EXPORT_SYMBOL(dsa_port_phylink_mac_link_down); > + > +void dsa_port_phylink_mac_link_up(struct dsa_port *dp, > + unsigned int mode, > + phy_interface_t interface, > + struct phy_device *phydev) > +{ > + struct dsa_switch *ds = dp->ds; > + > + if (!ds->ops->phylink_mac_link_up) { > + if (ds->ops->adjust_link && phydev) > + ds->ops->adjust_link(ds, dp->index, phydev); > + return; > + } > + > + ds->ops->phylink_mac_link_up(ds, dp->index, mode, interface, phydev); > +} > +EXPORT_SYMBOL(dsa_port_phylink_mac_link_up); > + > +void dsa_port_phylink_fixed_state(struct dsa_port *dp, > + struct phylink_link_state *state) > +{ > + struct dsa_switch *ds = dp->ds; > + > + /* No need to check that this operation is valid, the callback would > + * not be called if it was not. > + */ > + ds->ops->phylink_fixed_state(ds, dp->index, state); > +} > +EXPORT_SYMBOL(dsa_port_phylink_fixed_state); > + > static int dsa_port_setup_phy_of(struct dsa_port *dp, bool enable) > { > struct dsa_switch *ds = dp->ds; > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 9892ca1f6859..308066da8a0f 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -1169,25 +1169,16 @@ static void dsa_slave_phylink_validate(struct net_device *dev, > struct phylink_link_state *state) > { > struct dsa_port *dp = dsa_slave_to_port(dev); > - struct dsa_switch *ds = dp->ds; > - > - if (!ds->ops->phylink_validate) > - return; Ah, this is where it came from - but still wrong for the reasons I stated above, though it makes it not a bug you're introducing, but one that pre-exists. > > - ds->ops->phylink_validate(ds, dp->index, supported, state); > + dsa_port_phylink_validate(dp, supported, state); > } > > static int dsa_slave_phylink_mac_link_state(struct net_device *dev, > struct phylink_link_state *state) > { > struct dsa_port *dp = dsa_slave_to_port(dev); > - struct dsa_switch *ds = dp->ds; > - > - /* Only called for SGMII and 802.3z */ > - if (!ds->ops->phylink_mac_link_state) > - return -EOPNOTSUPP; > > - return ds->ops->phylink_mac_link_state(ds, dp->index, state); > + return dsa_port_phylink_mac_link_state(dp, state); > } > > static void dsa_slave_phylink_mac_config(struct net_device *dev, > @@ -1195,23 +1186,15 @@ static void dsa_slave_phylink_mac_config(struct net_device *dev, > const struct phylink_link_state *state) > { > struct dsa_port *dp = dsa_slave_to_port(dev); > - struct dsa_switch *ds = dp->ds; > - > - if (!ds->ops->phylink_mac_config) > - return; > > - ds->ops->phylink_mac_config(ds, dp->index, mode, state); > + dsa_port_phylink_mac_config(dp, mode, state); > } > > static void dsa_slave_phylink_mac_an_restart(struct net_device *dev) > { > struct dsa_port *dp = dsa_slave_to_port(dev); > - struct dsa_switch *ds = dp->ds; > - > - if (!ds->ops->phylink_mac_an_restart) > - return; > > - ds->ops->phylink_mac_an_restart(ds, dp->index); > + dsa_port_phylink_mac_an_restart(dp); > } > > static void dsa_slave_phylink_mac_link_down(struct net_device *dev, > @@ -1219,15 +1202,8 @@ static void dsa_slave_phylink_mac_link_down(struct net_device *dev, > phy_interface_t interface) > { > struct dsa_port *dp = dsa_slave_to_port(dev); > - struct dsa_switch *ds = dp->ds; > - > - if (!ds->ops->phylink_mac_link_down) { > - if (ds->ops->adjust_link && dev->phydev) > - ds->ops->adjust_link(ds, dp->index, dev->phydev); > - return; > - } > > - ds->ops->phylink_mac_link_down(ds, dp->index, mode, interface); > + dsa_port_phylink_mac_link_down(dp, mode, interface, dev->phydev); > } > > static void dsa_slave_phylink_mac_link_up(struct net_device *dev, > @@ -1236,15 +1212,8 @@ static void dsa_slave_phylink_mac_link_up(struct net_device *dev, > struct phy_device *phydev) > { > struct dsa_port *dp = dsa_slave_to_port(dev); > - struct dsa_switch *ds = dp->ds; > > - if (!ds->ops->phylink_mac_link_up) { > - if (ds->ops->adjust_link && dev->phydev) > - ds->ops->adjust_link(ds, dp->index, dev->phydev); > - return; > - } > - > - ds->ops->phylink_mac_link_up(ds, dp->index, mode, interface, phydev); > + dsa_port_phylink_mac_link_up(dp, mode, interface, phydev); > } > > static const struct phylink_mac_ops dsa_slave_phylink_mac_ops = { > @@ -1268,12 +1237,8 @@ static void dsa_slave_phylink_fixed_state(struct net_device *dev, > struct phylink_link_state *state) > { > struct dsa_port *dp = dsa_slave_to_port(dev); > - struct dsa_switch *ds = dp->ds; > > - /* No need to check that this operation is valid, the callback would > - * not be called if it was not. > - */ > - ds->ops->phylink_fixed_state(ds, dp->index, state); > + dsa_port_phylink_fixed_state(dp, state); > } > > /* slave device setup *******************************************************/ > -- > 2.21.0 > >
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index 8f1222324646..da70da65bdc5 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -161,6 +161,25 @@ int dsa_port_vlan_del(struct dsa_port *dp, const struct switchdev_obj_port_vlan *vlan); int dsa_port_vid_add(struct dsa_port *dp, u16 vid, u16 flags); int dsa_port_vid_del(struct dsa_port *dp, u16 vid); +void dsa_port_phylink_validate(struct dsa_port *dp, + unsigned long *supported, + struct phylink_link_state *state); +int dsa_port_phylink_mac_link_state(struct dsa_port *dp, + struct phylink_link_state *state); +void dsa_port_phylink_mac_config(struct dsa_port *dp, + unsigned int mode, + const struct phylink_link_state *state); +void dsa_port_phylink_mac_an_restart(struct dsa_port *dp); +void dsa_port_phylink_mac_link_down(struct dsa_port *dp, + unsigned int mode, + phy_interface_t interface, + struct phy_device *phydev); +void dsa_port_phylink_mac_link_up(struct dsa_port *dp, + unsigned int mode, + phy_interface_t interface, + struct phy_device *phydev); +void dsa_port_phylink_fixed_state(struct dsa_port *dp, + struct phylink_link_state *state); int dsa_port_link_register_of(struct dsa_port *dp); void dsa_port_link_unregister_of(struct dsa_port *dp); diff --git a/net/dsa/port.c b/net/dsa/port.c index ed8ba9daa3ba..d0f955e8b731 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -422,6 +422,102 @@ static struct phy_device *dsa_port_get_phy_device(struct dsa_port *dp) return phydev; } +void dsa_port_phylink_validate(struct dsa_port *dp, + unsigned long *supported, + struct phylink_link_state *state) +{ + struct dsa_switch *ds = dp->ds; + + if (!ds->ops->phylink_validate) + return; + + ds->ops->phylink_validate(ds, dp->index, supported, state); +} +EXPORT_SYMBOL(dsa_port_phylink_validate); + +int dsa_port_phylink_mac_link_state(struct dsa_port *dp, + struct phylink_link_state *state) +{ + struct dsa_switch *ds = dp->ds; + + /* Only called for SGMII and 802.3z */ + if (!ds->ops->phylink_mac_link_state) + return -EOPNOTSUPP; + + return ds->ops->phylink_mac_link_state(ds, dp->index, state); +} +EXPORT_SYMBOL(dsa_port_phylink_mac_link_state); + +void dsa_port_phylink_mac_config(struct dsa_port *dp, + unsigned int mode, + const struct phylink_link_state *state) +{ + struct dsa_switch *ds = dp->ds; + + if (!ds->ops->phylink_mac_config) + return; + + ds->ops->phylink_mac_config(ds, dp->index, mode, state); +} +EXPORT_SYMBOL(dsa_port_phylink_mac_config); + +void dsa_port_phylink_mac_an_restart(struct dsa_port *dp) +{ + struct dsa_switch *ds = dp->ds; + + if (!ds->ops->phylink_mac_an_restart) + return; + + ds->ops->phylink_mac_an_restart(ds, dp->index); +} +EXPORT_SYMBOL(dsa_port_phylink_mac_an_restart); + +void dsa_port_phylink_mac_link_down(struct dsa_port *dp, + unsigned int mode, + phy_interface_t interface, + struct phy_device *phydev) +{ + struct dsa_switch *ds = dp->ds; + + if (!ds->ops->phylink_mac_link_down) { + if (ds->ops->adjust_link && phydev) + ds->ops->adjust_link(ds, dp->index, phydev); + return; + } + + ds->ops->phylink_mac_link_down(ds, dp->index, mode, interface); +} +EXPORT_SYMBOL(dsa_port_phylink_mac_link_down); + +void dsa_port_phylink_mac_link_up(struct dsa_port *dp, + unsigned int mode, + phy_interface_t interface, + struct phy_device *phydev) +{ + struct dsa_switch *ds = dp->ds; + + if (!ds->ops->phylink_mac_link_up) { + if (ds->ops->adjust_link && phydev) + ds->ops->adjust_link(ds, dp->index, phydev); + return; + } + + ds->ops->phylink_mac_link_up(ds, dp->index, mode, interface, phydev); +} +EXPORT_SYMBOL(dsa_port_phylink_mac_link_up); + +void dsa_port_phylink_fixed_state(struct dsa_port *dp, + struct phylink_link_state *state) +{ + struct dsa_switch *ds = dp->ds; + + /* No need to check that this operation is valid, the callback would + * not be called if it was not. + */ + ds->ops->phylink_fixed_state(ds, dp->index, state); +} +EXPORT_SYMBOL(dsa_port_phylink_fixed_state); + static int dsa_port_setup_phy_of(struct dsa_port *dp, bool enable) { struct dsa_switch *ds = dp->ds; diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 9892ca1f6859..308066da8a0f 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -1169,25 +1169,16 @@ static void dsa_slave_phylink_validate(struct net_device *dev, struct phylink_link_state *state) { struct dsa_port *dp = dsa_slave_to_port(dev); - struct dsa_switch *ds = dp->ds; - - if (!ds->ops->phylink_validate) - return; - ds->ops->phylink_validate(ds, dp->index, supported, state); + dsa_port_phylink_validate(dp, supported, state); } static int dsa_slave_phylink_mac_link_state(struct net_device *dev, struct phylink_link_state *state) { struct dsa_port *dp = dsa_slave_to_port(dev); - struct dsa_switch *ds = dp->ds; - - /* Only called for SGMII and 802.3z */ - if (!ds->ops->phylink_mac_link_state) - return -EOPNOTSUPP; - return ds->ops->phylink_mac_link_state(ds, dp->index, state); + return dsa_port_phylink_mac_link_state(dp, state); } static void dsa_slave_phylink_mac_config(struct net_device *dev, @@ -1195,23 +1186,15 @@ static void dsa_slave_phylink_mac_config(struct net_device *dev, const struct phylink_link_state *state) { struct dsa_port *dp = dsa_slave_to_port(dev); - struct dsa_switch *ds = dp->ds; - - if (!ds->ops->phylink_mac_config) - return; - ds->ops->phylink_mac_config(ds, dp->index, mode, state); + dsa_port_phylink_mac_config(dp, mode, state); } static void dsa_slave_phylink_mac_an_restart(struct net_device *dev) { struct dsa_port *dp = dsa_slave_to_port(dev); - struct dsa_switch *ds = dp->ds; - - if (!ds->ops->phylink_mac_an_restart) - return; - ds->ops->phylink_mac_an_restart(ds, dp->index); + dsa_port_phylink_mac_an_restart(dp); } static void dsa_slave_phylink_mac_link_down(struct net_device *dev, @@ -1219,15 +1202,8 @@ static void dsa_slave_phylink_mac_link_down(struct net_device *dev, phy_interface_t interface) { struct dsa_port *dp = dsa_slave_to_port(dev); - struct dsa_switch *ds = dp->ds; - - if (!ds->ops->phylink_mac_link_down) { - if (ds->ops->adjust_link && dev->phydev) - ds->ops->adjust_link(ds, dp->index, dev->phydev); - return; - } - ds->ops->phylink_mac_link_down(ds, dp->index, mode, interface); + dsa_port_phylink_mac_link_down(dp, mode, interface, dev->phydev); } static void dsa_slave_phylink_mac_link_up(struct net_device *dev, @@ -1236,15 +1212,8 @@ static void dsa_slave_phylink_mac_link_up(struct net_device *dev, struct phy_device *phydev) { struct dsa_port *dp = dsa_slave_to_port(dev); - struct dsa_switch *ds = dp->ds; - if (!ds->ops->phylink_mac_link_up) { - if (ds->ops->adjust_link && dev->phydev) - ds->ops->adjust_link(ds, dp->index, dev->phydev); - return; - } - - ds->ops->phylink_mac_link_up(ds, dp->index, mode, interface, phydev); + dsa_port_phylink_mac_link_up(dp, mode, interface, phydev); } static const struct phylink_mac_ops dsa_slave_phylink_mac_ops = { @@ -1268,12 +1237,8 @@ static void dsa_slave_phylink_fixed_state(struct net_device *dev, struct phylink_link_state *state) { struct dsa_port *dp = dsa_slave_to_port(dev); - struct dsa_switch *ds = dp->ds; - /* No need to check that this operation is valid, the callback would - * not be called if it was not. - */ - ds->ops->phylink_fixed_state(ds, dp->index, state); + dsa_port_phylink_fixed_state(dp, state); } /* slave device setup *******************************************************/
In order to have a common handling of PHYLINK for the slave and non-user ports, the DSA core glue logic (between PHYLINK and the driver) must use an API that does not rely on a struct net_device. These will also be called by the CPU-port-handling code in a further patch. Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> Suggested-by: Vladimir Oltean <olteanv@gmail.com> --- net/dsa/dsa_priv.h | 19 +++++++++ net/dsa/port.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++ net/dsa/slave.c | 49 ++++------------------- 3 files changed, 122 insertions(+), 42 deletions(-)