* [PATCH net-next 0/4] net: dsa: simplify port enabling
@ 2017-09-22 16:17 Vivien Didelot
2017-09-22 16:17 ` [PATCH net-next 1/4] net: dsa: move up phy enabling in core Vivien Didelot
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Vivien Didelot @ 2017-09-22 16:17 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli,
Andrew Lunn, Vivien Didelot
This patchset removes the unnecessary PHY device argument in port
enable/disable switch operations, makes slave open and close symmetrical
and finally provides helpers for enabling or disabling a DSA port.
Vivien Didelot (4):
net: dsa: move up phy enabling in core
net: dsa: remove phy arg from port enable/disable
net: dsa: make slave close symmetrical to open
net: dsa: add port enable and disable helpers
drivers/net/dsa/b53/b53_common.c | 6 +++---
drivers/net/dsa/b53/b53_priv.h | 4 ++--
drivers/net/dsa/bcm_sf2.c | 32 ++++++++----------------------
drivers/net/dsa/lan9303-core.c | 6 ++----
drivers/net/dsa/microchip/ksz_common.c | 6 ++----
drivers/net/dsa/mt7530.c | 8 +++-----
drivers/net/dsa/mv88e6xxx/chip.c | 6 ++----
drivers/net/dsa/qca8k.c | 6 ++----
include/net/dsa.h | 6 ++----
net/dsa/dsa_priv.h | 3 ++-
net/dsa/port.c | 31 ++++++++++++++++++++++++++++-
net/dsa/slave.c | 36 ++++++++++++++++++----------------
12 files changed, 77 insertions(+), 73 deletions(-)
--
2.14.1
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH net-next 1/4] net: dsa: move up phy enabling in core 2017-09-22 16:17 [PATCH net-next 0/4] net: dsa: simplify port enabling Vivien Didelot @ 2017-09-22 16:17 ` Vivien Didelot 2017-09-22 16:32 ` Andrew Lunn 2017-09-22 16:52 ` Florian Fainelli 2017-09-22 16:17 ` [PATCH net-next 2/4] net: dsa: remove phy arg from port enable/disable Vivien Didelot ` (2 subsequent siblings) 3 siblings, 2 replies; 16+ messages in thread From: Vivien Didelot @ 2017-09-22 16:17 UTC (permalink / raw) To: netdev Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli, Andrew Lunn, Vivien Didelot bcm_sf2 is currently the only driver using the phy argument passed to .port_enable. It resets the state machine if the phy has been hard reset. This check is generic and can be moved to DSA core. Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> --- drivers/net/dsa/bcm_sf2.c | 16 +--------------- net/dsa/slave.c | 15 +++++++++++++-- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c index 898d5642b516..ad96b9725a2c 100644 --- a/drivers/net/dsa/bcm_sf2.c +++ b/drivers/net/dsa/bcm_sf2.c @@ -184,22 +184,8 @@ static int bcm_sf2_port_setup(struct dsa_switch *ds, int port, core_writel(priv, reg, CORE_PORT_TC2_QOS_MAP_PORT(port)); /* Re-enable the GPHY and re-apply workarounds */ - if (priv->int_phy_mask & 1 << port && priv->hw_params.num_gphy == 1) { + if (priv->int_phy_mask & 1 << port && priv->hw_params.num_gphy == 1) bcm_sf2_gphy_enable_set(ds, true); - if (phy) { - /* if phy_stop() has been called before, phy - * will be in halted state, and phy_start() - * will call resume. - * - * the resume path does not configure back - * autoneg settings, and since we hard reset - * the phy manually here, we need to reset the - * state machine also. - */ - phy->state = PHY_READY; - phy_init_hw(phy); - } - } /* Enable MoCA port interrupts to get notified */ if (port == priv->moca_port) diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 02ace7d462c4..606812160fd5 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -72,6 +72,7 @@ static int dsa_slave_get_iflink(const struct net_device *dev) static int dsa_slave_open(struct net_device *dev) { struct dsa_slave_priv *p = netdev_priv(dev); + struct phy_device *phy = p->phy; struct dsa_port *dp = p->dp; struct dsa_switch *ds = dp->ds; struct net_device *master = dsa_master_netdev(p); @@ -106,8 +107,18 @@ static int dsa_slave_open(struct net_device *dev) dsa_port_set_state_now(p->dp, stp_state); - if (p->phy) - phy_start(p->phy); + if (phy) { + /* If phy_stop() has been called before, phy will be in + * halted state, and phy_start() will call resume. + * + * The resume path does not configure back autoneg + * settings, and since the internal phy may have been + * hard reset, we need to reset the state machine also. + */ + phy->state = PHY_READY; + phy_init_hw(phy); + phy_start(phy); + } return 0; -- 2.14.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH net-next 1/4] net: dsa: move up phy enabling in core 2017-09-22 16:17 ` [PATCH net-next 1/4] net: dsa: move up phy enabling in core Vivien Didelot @ 2017-09-22 16:32 ` Andrew Lunn 2017-09-22 16:58 ` Florian Fainelli 2017-09-22 16:52 ` Florian Fainelli 1 sibling, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2017-09-22 16:32 UTC (permalink / raw) To: Vivien Didelot Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli On Fri, Sep 22, 2017 at 12:17:50PM -0400, Vivien Didelot wrote: > bcm_sf2 is currently the only driver using the phy argument passed to > .port_enable. It resets the state machine if the phy has been hard > reset. This check is generic and can be moved to DSA core. > > dsa_port_set_state_now(p->dp, stp_state); > > - if (p->phy) > - phy_start(p->phy); > + if (phy) { > + /* If phy_stop() has been called before, phy will be in > + * halted state, and phy_start() will call resume. > + * > + * The resume path does not configure back autoneg > + * settings, and since the internal phy may have been > + * hard reset, we need to reset the state machine also. > + */ > + phy->state = PHY_READY; > + phy_init_hw(phy); > + phy_start(phy); > + } Hi Vivien If this is generic, why is it needed at all here? Shouldn't this actually by in phylib? Florian ? Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next 1/4] net: dsa: move up phy enabling in core 2017-09-22 16:32 ` Andrew Lunn @ 2017-09-22 16:58 ` Florian Fainelli 0 siblings, 0 replies; 16+ messages in thread From: Florian Fainelli @ 2017-09-22 16:58 UTC (permalink / raw) To: Andrew Lunn, Vivien Didelot; +Cc: netdev, linux-kernel, kernel, David S. Miller On 09/22/2017 09:32 AM, Andrew Lunn wrote: > On Fri, Sep 22, 2017 at 12:17:50PM -0400, Vivien Didelot wrote: >> bcm_sf2 is currently the only driver using the phy argument passed to >> .port_enable. It resets the state machine if the phy has been hard >> reset. This check is generic and can be moved to DSA core. >> >> dsa_port_set_state_now(p->dp, stp_state); >> >> - if (p->phy) >> - phy_start(p->phy); >> + if (phy) { >> + /* If phy_stop() has been called before, phy will be in >> + * halted state, and phy_start() will call resume. >> + * >> + * The resume path does not configure back autoneg >> + * settings, and since the internal phy may have been >> + * hard reset, we need to reset the state machine also. >> + */ >> + phy->state = PHY_READY; >> + phy_init_hw(phy); >> + phy_start(phy); >> + } > > Hi Vivien > > If this is generic, why is it needed at all here? Shouldn't this > actually by in phylib? This does not belong in the core logic within net/dsa/slave.c. The reason why this is necessary here is because we are doing a HW-based reset of the PHY, as the comment explains this is specific to how the HW works. There may be a cleaner solution to this problem, but in any case, I don't think other drivers should inherit that logic. -- Florian ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next 1/4] net: dsa: move up phy enabling in core 2017-09-22 16:17 ` [PATCH net-next 1/4] net: dsa: move up phy enabling in core Vivien Didelot 2017-09-22 16:32 ` Andrew Lunn @ 2017-09-22 16:52 ` Florian Fainelli 1 sibling, 0 replies; 16+ messages in thread From: Florian Fainelli @ 2017-09-22 16:52 UTC (permalink / raw) To: Vivien Didelot, netdev; +Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn On 09/22/2017 09:17 AM, Vivien Didelot wrote: > bcm_sf2 is currently the only driver using the phy argument passed to > .port_enable. It resets the state machine if the phy has been hard > reset. This check is generic and can be moved to DSA core. This is completely specific to bcm_sf2 because it does call bcm_sf2_gphy_enable_set() which performs a HW reset of the PHY, you can't move this to the generic portion of net/dsa/slave.c. NACK. > > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> > --- > drivers/net/dsa/bcm_sf2.c | 16 +--------------- > net/dsa/slave.c | 15 +++++++++++++-- > 2 files changed, 14 insertions(+), 17 deletions(-) > > diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c > index 898d5642b516..ad96b9725a2c 100644 > --- a/drivers/net/dsa/bcm_sf2.c > +++ b/drivers/net/dsa/bcm_sf2.c > @@ -184,22 +184,8 @@ static int bcm_sf2_port_setup(struct dsa_switch *ds, int port, > core_writel(priv, reg, CORE_PORT_TC2_QOS_MAP_PORT(port)); > > /* Re-enable the GPHY and re-apply workarounds */ > - if (priv->int_phy_mask & 1 << port && priv->hw_params.num_gphy == 1) { > + if (priv->int_phy_mask & 1 << port && priv->hw_params.num_gphy == 1) > bcm_sf2_gphy_enable_set(ds, true); > - if (phy) { > - /* if phy_stop() has been called before, phy > - * will be in halted state, and phy_start() > - * will call resume. > - * > - * the resume path does not configure back > - * autoneg settings, and since we hard reset > - * the phy manually here, we need to reset the > - * state machine also. > - */ > - phy->state = PHY_READY; > - phy_init_hw(phy); > - } > - } > > /* Enable MoCA port interrupts to get notified */ > if (port == priv->moca_port) > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 02ace7d462c4..606812160fd5 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -72,6 +72,7 @@ static int dsa_slave_get_iflink(const struct net_device *dev) > static int dsa_slave_open(struct net_device *dev) > { > struct dsa_slave_priv *p = netdev_priv(dev); > + struct phy_device *phy = p->phy; > struct dsa_port *dp = p->dp; > struct dsa_switch *ds = dp->ds; > struct net_device *master = dsa_master_netdev(p); > @@ -106,8 +107,18 @@ static int dsa_slave_open(struct net_device *dev) > > dsa_port_set_state_now(p->dp, stp_state); > > - if (p->phy) > - phy_start(p->phy); > + if (phy) { > + /* If phy_stop() has been called before, phy will be in > + * halted state, and phy_start() will call resume. > + * > + * The resume path does not configure back autoneg > + * settings, and since the internal phy may have been > + * hard reset, we need to reset the state machine also. > + */ > + phy->state = PHY_READY; > + phy_init_hw(phy); > + phy_start(phy); > + } > > return 0; > > -- Florian ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH net-next 2/4] net: dsa: remove phy arg from port enable/disable 2017-09-22 16:17 [PATCH net-next 0/4] net: dsa: simplify port enabling Vivien Didelot 2017-09-22 16:17 ` [PATCH net-next 1/4] net: dsa: move up phy enabling in core Vivien Didelot @ 2017-09-22 16:17 ` Vivien Didelot 2017-09-22 17:14 ` Florian Fainelli 2017-09-22 16:17 ` [PATCH net-next 3/4] net: dsa: make slave close symmetrical to open Vivien Didelot 2017-09-22 16:17 ` [PATCH net-next 4/4] net: dsa: add port enable and disable helpers Vivien Didelot 3 siblings, 1 reply; 16+ messages in thread From: Vivien Didelot @ 2017-09-22 16:17 UTC (permalink / raw) To: netdev Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli, Andrew Lunn, Vivien Didelot The .port_enable and .port_disable functions are meant to deal with the switch ports only, and no driver is using the phy argument anyway. Remove it. Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> --- drivers/net/dsa/b53/b53_common.c | 6 +++--- drivers/net/dsa/b53/b53_priv.h | 4 ++-- drivers/net/dsa/bcm_sf2.c | 16 +++++++--------- drivers/net/dsa/lan9303-core.c | 6 ++---- drivers/net/dsa/microchip/ksz_common.c | 6 ++---- drivers/net/dsa/mt7530.c | 8 +++----- drivers/net/dsa/mv88e6xxx/chip.c | 6 ++---- drivers/net/dsa/qca8k.c | 6 ++---- include/net/dsa.h | 6 ++---- net/dsa/slave.c | 4 ++-- 10 files changed, 27 insertions(+), 41 deletions(-) diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c index d4ce092def83..e46eb29d29f0 100644 --- a/drivers/net/dsa/b53/b53_common.c +++ b/drivers/net/dsa/b53/b53_common.c @@ -502,7 +502,7 @@ void b53_imp_vlan_setup(struct dsa_switch *ds, int cpu_port) } EXPORT_SYMBOL(b53_imp_vlan_setup); -int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy) +int b53_enable_port(struct dsa_switch *ds, int port) { struct b53_device *dev = ds->priv; unsigned int cpu_port = dev->cpu_port; @@ -531,7 +531,7 @@ int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy) } EXPORT_SYMBOL(b53_enable_port); -void b53_disable_port(struct dsa_switch *ds, int port, struct phy_device *phy) +void b53_disable_port(struct dsa_switch *ds, int port) { struct b53_device *dev = ds->priv; u8 reg; @@ -874,7 +874,7 @@ static int b53_setup(struct dsa_switch *ds) if (dsa_is_cpu_port(ds, port)) b53_enable_cpu_port(dev, port); else if (!(BIT(port) & ds->enabled_port_mask)) - b53_disable_port(ds, port, NULL); + b53_disable_port(ds, port); } return ret; diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h index 603c66d240d8..688d02ee6155 100644 --- a/drivers/net/dsa/b53/b53_priv.h +++ b/drivers/net/dsa/b53/b53_priv.h @@ -311,8 +311,8 @@ int b53_mirror_add(struct dsa_switch *ds, int port, struct dsa_mall_mirror_tc_entry *mirror, bool ingress); void b53_mirror_del(struct dsa_switch *ds, int port, struct dsa_mall_mirror_tc_entry *mirror); -int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy); -void b53_disable_port(struct dsa_switch *ds, int port, struct phy_device *phy); +int b53_enable_port(struct dsa_switch *ds, int port); +void b53_disable_port(struct dsa_switch *ds, int port); void b53_brcm_hdr_setup(struct dsa_switch *ds, int port); void b53_eee_enable_set(struct dsa_switch *ds, int port, bool enable); int b53_eee_init(struct dsa_switch *ds, int port, struct phy_device *phy); diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c index ad96b9725a2c..77e0c43f973b 100644 --- a/drivers/net/dsa/bcm_sf2.c +++ b/drivers/net/dsa/bcm_sf2.c @@ -159,8 +159,7 @@ static inline void bcm_sf2_port_intr_disable(struct bcm_sf2_priv *priv, intrl2_1_writel(priv, P_IRQ_MASK(off), INTRL2_CPU_CLEAR); } -static int bcm_sf2_port_setup(struct dsa_switch *ds, int port, - struct phy_device *phy) +static int bcm_sf2_port_setup(struct dsa_switch *ds, int port) { struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds); unsigned int i; @@ -191,11 +190,10 @@ static int bcm_sf2_port_setup(struct dsa_switch *ds, int port, if (port == priv->moca_port) bcm_sf2_port_intr_enable(priv, port); - return b53_enable_port(ds, port, phy); + return b53_enable_port(ds, port); } -static void bcm_sf2_port_disable(struct dsa_switch *ds, int port, - struct phy_device *phy) +static void bcm_sf2_port_disable(struct dsa_switch *ds, int port) { struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds); u32 off, reg; @@ -214,7 +212,7 @@ static void bcm_sf2_port_disable(struct dsa_switch *ds, int port, else off = CORE_G_PCTL_PORT(port); - b53_disable_port(ds, port, phy); + b53_disable_port(ds, port); /* Power down the port memory */ reg = core_readl(priv, CORE_MEM_PSM_VDD_CTRL); @@ -613,7 +611,7 @@ static int bcm_sf2_sw_suspend(struct dsa_switch *ds) for (port = 0; port < DSA_MAX_PORTS; port++) { if ((1 << port) & ds->enabled_port_mask || dsa_is_cpu_port(ds, port)) - bcm_sf2_port_disable(ds, port, NULL); + bcm_sf2_port_disable(ds, port); } return 0; @@ -636,7 +634,7 @@ static int bcm_sf2_sw_resume(struct dsa_switch *ds) for (port = 0; port < DSA_MAX_PORTS; port++) { if ((1 << port) & ds->enabled_port_mask) - bcm_sf2_port_setup(ds, port, NULL); + bcm_sf2_port_setup(ds, port); else if (dsa_is_cpu_port(ds, port)) bcm_sf2_imp_setup(ds, port); } @@ -745,7 +743,7 @@ static int bcm_sf2_sw_setup(struct dsa_switch *ds) if (dsa_is_cpu_port(ds, port)) bcm_sf2_imp_setup(ds, port); else if (!((1 << port) & ds->enabled_port_mask)) - bcm_sf2_port_disable(ds, port, NULL); + bcm_sf2_port_disable(ds, port); } bcm_sf2_sw_configure_vlan(ds); diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c index 07355db2ad81..0c33b02562dc 100644 --- a/drivers/net/dsa/lan9303-core.c +++ b/drivers/net/dsa/lan9303-core.c @@ -799,8 +799,7 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port, } } -static int lan9303_port_enable(struct dsa_switch *ds, int port, - struct phy_device *phy) +static int lan9303_port_enable(struct dsa_switch *ds, int port) { struct lan9303 *chip = ds->priv; @@ -817,8 +816,7 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port, return -ENODEV; } -static void lan9303_port_disable(struct dsa_switch *ds, int port, - struct phy_device *phy) +static void lan9303_port_disable(struct dsa_switch *ds, int port) { struct lan9303 *chip = ds->priv; diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index 56cd6d365352..4095c50ae111 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -418,8 +418,7 @@ static int ksz_phy_write16(struct dsa_switch *ds, int addr, int reg, u16 val) return 0; } -static int ksz_enable_port(struct dsa_switch *ds, int port, - struct phy_device *phy) +static int ksz_enable_port(struct dsa_switch *ds, int port) { struct ksz_device *dev = ds->priv; @@ -429,8 +428,7 @@ static int ksz_enable_port(struct dsa_switch *ds, int port, return 0; } -static void ksz_disable_port(struct dsa_switch *ds, int port, - struct phy_device *phy) +static void ksz_disable_port(struct dsa_switch *ds, int port) { struct ksz_device *dev = ds->priv; diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index faa3b88d2206..0a7f6209767f 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -693,8 +693,7 @@ mt7530_cpu_port_enable(struct mt7530_priv *priv, } static int -mt7530_port_enable(struct dsa_switch *ds, int port, - struct phy_device *phy) +mt7530_port_enable(struct dsa_switch *ds, int port) { struct mt7530_priv *priv = ds->priv; @@ -719,8 +718,7 @@ mt7530_port_enable(struct dsa_switch *ds, int port, } static void -mt7530_port_disable(struct dsa_switch *ds, int port, - struct phy_device *phy) +mt7530_port_disable(struct dsa_switch *ds, int port) { struct mt7530_priv *priv = ds->priv; @@ -1006,7 +1004,7 @@ mt7530_setup(struct dsa_switch *ds) if (dsa_is_cpu_port(ds, i)) mt7530_cpu_port_enable(priv, i); else - mt7530_port_disable(ds, i, NULL); + mt7530_port_disable(ds, i); } /* Flush the FDB table */ diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index c6678aa9b4ef..e47898fb7dbc 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -1862,8 +1862,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port) return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_DEFAULT_VLAN, 0); } -static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port, - struct phy_device *phydev) +static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port) { struct mv88e6xxx_chip *chip = ds->priv; int err; @@ -1875,8 +1874,7 @@ static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port, return err; } -static void mv88e6xxx_port_disable(struct dsa_switch *ds, int port, - struct phy_device *phydev) +static void mv88e6xxx_port_disable(struct dsa_switch *ds, int port) { struct mv88e6xxx_chip *chip = ds->priv; diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c index 82f09711ac1a..622ee9b8e72b 100644 --- a/drivers/net/dsa/qca8k.c +++ b/drivers/net/dsa/qca8k.c @@ -743,8 +743,7 @@ qca8k_port_bridge_leave(struct dsa_switch *ds, int port, struct net_device *br) } static int -qca8k_port_enable(struct dsa_switch *ds, int port, - struct phy_device *phy) +qca8k_port_enable(struct dsa_switch *ds, int port) { struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv; @@ -755,8 +754,7 @@ qca8k_port_enable(struct dsa_switch *ds, int port, } static void -qca8k_port_disable(struct dsa_switch *ds, int port, - struct phy_device *phy) +qca8k_port_disable(struct dsa_switch *ds, int port) { struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv; diff --git a/include/net/dsa.h b/include/net/dsa.h index 8dee216a5a9b..65b031a69c19 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -337,10 +337,8 @@ struct dsa_switch_ops { /* * Port enable/disable */ - int (*port_enable)(struct dsa_switch *ds, int port, - struct phy_device *phy); - void (*port_disable)(struct dsa_switch *ds, int port, - struct phy_device *phy); + int (*port_enable)(struct dsa_switch *ds, int port); + void (*port_disable)(struct dsa_switch *ds, int port); /* * Port's MAC EEE settings diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 606812160fd5..6290741e496a 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -100,7 +100,7 @@ static int dsa_slave_open(struct net_device *dev) } if (ds->ops->port_enable) { - err = ds->ops->port_enable(ds, p->dp->index, p->phy); + err = ds->ops->port_enable(ds, p->dp->index); if (err) goto clear_promisc; } @@ -155,7 +155,7 @@ static int dsa_slave_close(struct net_device *dev) dev_uc_del(master, dev->dev_addr); if (ds->ops->port_disable) - ds->ops->port_disable(ds, p->dp->index, p->phy); + ds->ops->port_disable(ds, p->dp->index); dsa_port_set_state_now(p->dp, BR_STATE_DISABLED); -- 2.14.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH net-next 2/4] net: dsa: remove phy arg from port enable/disable 2017-09-22 16:17 ` [PATCH net-next 2/4] net: dsa: remove phy arg from port enable/disable Vivien Didelot @ 2017-09-22 17:14 ` Florian Fainelli 2017-09-22 18:12 ` Vivien Didelot 0 siblings, 1 reply; 16+ messages in thread From: Florian Fainelli @ 2017-09-22 17:14 UTC (permalink / raw) To: Vivien Didelot, netdev; +Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn On 09/22/2017 09:17 AM, Vivien Didelot wrote: > The .port_enable and .port_disable functions are meant to deal with the > switch ports only, and no driver is using the phy argument anyway. > Remove it. I don't think this makes sense, there are perfectly legit reasons why a switch driver may have something to do with the PHY device attached to its per-port network interface, we should definitively keep that around, unless you think we should be accessing the PHY within the switch drivers by doing: struct phy_device *phydev = ds->ports[port].netdev->phydev? > > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> > --- > drivers/net/dsa/b53/b53_common.c | 6 +++--- > drivers/net/dsa/b53/b53_priv.h | 4 ++-- > drivers/net/dsa/bcm_sf2.c | 16 +++++++--------- > drivers/net/dsa/lan9303-core.c | 6 ++---- > drivers/net/dsa/microchip/ksz_common.c | 6 ++---- > drivers/net/dsa/mt7530.c | 8 +++----- > drivers/net/dsa/mv88e6xxx/chip.c | 6 ++---- > drivers/net/dsa/qca8k.c | 6 ++---- > include/net/dsa.h | 6 ++---- > net/dsa/slave.c | 4 ++-- > 10 files changed, 27 insertions(+), 41 deletions(-) > > diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c > index d4ce092def83..e46eb29d29f0 100644 > --- a/drivers/net/dsa/b53/b53_common.c > +++ b/drivers/net/dsa/b53/b53_common.c > @@ -502,7 +502,7 @@ void b53_imp_vlan_setup(struct dsa_switch *ds, int cpu_port) > } > EXPORT_SYMBOL(b53_imp_vlan_setup); > > -int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy) > +int b53_enable_port(struct dsa_switch *ds, int port) > { > struct b53_device *dev = ds->priv; > unsigned int cpu_port = dev->cpu_port; > @@ -531,7 +531,7 @@ int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy) > } > EXPORT_SYMBOL(b53_enable_port); > > -void b53_disable_port(struct dsa_switch *ds, int port, struct phy_device *phy) > +void b53_disable_port(struct dsa_switch *ds, int port) > { > struct b53_device *dev = ds->priv; > u8 reg; > @@ -874,7 +874,7 @@ static int b53_setup(struct dsa_switch *ds) > if (dsa_is_cpu_port(ds, port)) > b53_enable_cpu_port(dev, port); > else if (!(BIT(port) & ds->enabled_port_mask)) > - b53_disable_port(ds, port, NULL); > + b53_disable_port(ds, port); > } > > return ret; > diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h > index 603c66d240d8..688d02ee6155 100644 > --- a/drivers/net/dsa/b53/b53_priv.h > +++ b/drivers/net/dsa/b53/b53_priv.h > @@ -311,8 +311,8 @@ int b53_mirror_add(struct dsa_switch *ds, int port, > struct dsa_mall_mirror_tc_entry *mirror, bool ingress); > void b53_mirror_del(struct dsa_switch *ds, int port, > struct dsa_mall_mirror_tc_entry *mirror); > -int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy); > -void b53_disable_port(struct dsa_switch *ds, int port, struct phy_device *phy); > +int b53_enable_port(struct dsa_switch *ds, int port); > +void b53_disable_port(struct dsa_switch *ds, int port); > void b53_brcm_hdr_setup(struct dsa_switch *ds, int port); > void b53_eee_enable_set(struct dsa_switch *ds, int port, bool enable); > int b53_eee_init(struct dsa_switch *ds, int port, struct phy_device *phy); > diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c > index ad96b9725a2c..77e0c43f973b 100644 > --- a/drivers/net/dsa/bcm_sf2.c > +++ b/drivers/net/dsa/bcm_sf2.c > @@ -159,8 +159,7 @@ static inline void bcm_sf2_port_intr_disable(struct bcm_sf2_priv *priv, > intrl2_1_writel(priv, P_IRQ_MASK(off), INTRL2_CPU_CLEAR); > } > > -static int bcm_sf2_port_setup(struct dsa_switch *ds, int port, > - struct phy_device *phy) > +static int bcm_sf2_port_setup(struct dsa_switch *ds, int port) > { > struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds); > unsigned int i; > @@ -191,11 +190,10 @@ static int bcm_sf2_port_setup(struct dsa_switch *ds, int port, > if (port == priv->moca_port) > bcm_sf2_port_intr_enable(priv, port); > > - return b53_enable_port(ds, port, phy); > + return b53_enable_port(ds, port); > } > > -static void bcm_sf2_port_disable(struct dsa_switch *ds, int port, > - struct phy_device *phy) > +static void bcm_sf2_port_disable(struct dsa_switch *ds, int port) > { > struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds); > u32 off, reg; > @@ -214,7 +212,7 @@ static void bcm_sf2_port_disable(struct dsa_switch *ds, int port, > else > off = CORE_G_PCTL_PORT(port); > > - b53_disable_port(ds, port, phy); > + b53_disable_port(ds, port); > > /* Power down the port memory */ > reg = core_readl(priv, CORE_MEM_PSM_VDD_CTRL); > @@ -613,7 +611,7 @@ static int bcm_sf2_sw_suspend(struct dsa_switch *ds) > for (port = 0; port < DSA_MAX_PORTS; port++) { > if ((1 << port) & ds->enabled_port_mask || > dsa_is_cpu_port(ds, port)) > - bcm_sf2_port_disable(ds, port, NULL); > + bcm_sf2_port_disable(ds, port); > } > > return 0; > @@ -636,7 +634,7 @@ static int bcm_sf2_sw_resume(struct dsa_switch *ds) > > for (port = 0; port < DSA_MAX_PORTS; port++) { > if ((1 << port) & ds->enabled_port_mask) > - bcm_sf2_port_setup(ds, port, NULL); > + bcm_sf2_port_setup(ds, port); > else if (dsa_is_cpu_port(ds, port)) > bcm_sf2_imp_setup(ds, port); > } > @@ -745,7 +743,7 @@ static int bcm_sf2_sw_setup(struct dsa_switch *ds) > if (dsa_is_cpu_port(ds, port)) > bcm_sf2_imp_setup(ds, port); > else if (!((1 << port) & ds->enabled_port_mask)) > - bcm_sf2_port_disable(ds, port, NULL); > + bcm_sf2_port_disable(ds, port); > } > > bcm_sf2_sw_configure_vlan(ds); > diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c > index 07355db2ad81..0c33b02562dc 100644 > --- a/drivers/net/dsa/lan9303-core.c > +++ b/drivers/net/dsa/lan9303-core.c > @@ -799,8 +799,7 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port, > } > } > > -static int lan9303_port_enable(struct dsa_switch *ds, int port, > - struct phy_device *phy) > +static int lan9303_port_enable(struct dsa_switch *ds, int port) > { > struct lan9303 *chip = ds->priv; > > @@ -817,8 +816,7 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port, > return -ENODEV; > } > > -static void lan9303_port_disable(struct dsa_switch *ds, int port, > - struct phy_device *phy) > +static void lan9303_port_disable(struct dsa_switch *ds, int port) > { > struct lan9303 *chip = ds->priv; > > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c > index 56cd6d365352..4095c50ae111 100644 > --- a/drivers/net/dsa/microchip/ksz_common.c > +++ b/drivers/net/dsa/microchip/ksz_common.c > @@ -418,8 +418,7 @@ static int ksz_phy_write16(struct dsa_switch *ds, int addr, int reg, u16 val) > return 0; > } > > -static int ksz_enable_port(struct dsa_switch *ds, int port, > - struct phy_device *phy) > +static int ksz_enable_port(struct dsa_switch *ds, int port) > { > struct ksz_device *dev = ds->priv; > > @@ -429,8 +428,7 @@ static int ksz_enable_port(struct dsa_switch *ds, int port, > return 0; > } > > -static void ksz_disable_port(struct dsa_switch *ds, int port, > - struct phy_device *phy) > +static void ksz_disable_port(struct dsa_switch *ds, int port) > { > struct ksz_device *dev = ds->priv; > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > index faa3b88d2206..0a7f6209767f 100644 > --- a/drivers/net/dsa/mt7530.c > +++ b/drivers/net/dsa/mt7530.c > @@ -693,8 +693,7 @@ mt7530_cpu_port_enable(struct mt7530_priv *priv, > } > > static int > -mt7530_port_enable(struct dsa_switch *ds, int port, > - struct phy_device *phy) > +mt7530_port_enable(struct dsa_switch *ds, int port) > { > struct mt7530_priv *priv = ds->priv; > > @@ -719,8 +718,7 @@ mt7530_port_enable(struct dsa_switch *ds, int port, > } > > static void > -mt7530_port_disable(struct dsa_switch *ds, int port, > - struct phy_device *phy) > +mt7530_port_disable(struct dsa_switch *ds, int port) > { > struct mt7530_priv *priv = ds->priv; > > @@ -1006,7 +1004,7 @@ mt7530_setup(struct dsa_switch *ds) > if (dsa_is_cpu_port(ds, i)) > mt7530_cpu_port_enable(priv, i); > else > - mt7530_port_disable(ds, i, NULL); > + mt7530_port_disable(ds, i); > } > > /* Flush the FDB table */ > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > index c6678aa9b4ef..e47898fb7dbc 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.c > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > @@ -1862,8 +1862,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port) > return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_DEFAULT_VLAN, 0); > } > > -static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port, > - struct phy_device *phydev) > +static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port) > { > struct mv88e6xxx_chip *chip = ds->priv; > int err; > @@ -1875,8 +1874,7 @@ static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port, > return err; > } > > -static void mv88e6xxx_port_disable(struct dsa_switch *ds, int port, > - struct phy_device *phydev) > +static void mv88e6xxx_port_disable(struct dsa_switch *ds, int port) > { > struct mv88e6xxx_chip *chip = ds->priv; > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c > index 82f09711ac1a..622ee9b8e72b 100644 > --- a/drivers/net/dsa/qca8k.c > +++ b/drivers/net/dsa/qca8k.c > @@ -743,8 +743,7 @@ qca8k_port_bridge_leave(struct dsa_switch *ds, int port, struct net_device *br) > } > > static int > -qca8k_port_enable(struct dsa_switch *ds, int port, > - struct phy_device *phy) > +qca8k_port_enable(struct dsa_switch *ds, int port) > { > struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv; > > @@ -755,8 +754,7 @@ qca8k_port_enable(struct dsa_switch *ds, int port, > } > > static void > -qca8k_port_disable(struct dsa_switch *ds, int port, > - struct phy_device *phy) > +qca8k_port_disable(struct dsa_switch *ds, int port) > { > struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv; > > diff --git a/include/net/dsa.h b/include/net/dsa.h > index 8dee216a5a9b..65b031a69c19 100644 > --- a/include/net/dsa.h > +++ b/include/net/dsa.h > @@ -337,10 +337,8 @@ struct dsa_switch_ops { > /* > * Port enable/disable > */ > - int (*port_enable)(struct dsa_switch *ds, int port, > - struct phy_device *phy); > - void (*port_disable)(struct dsa_switch *ds, int port, > - struct phy_device *phy); > + int (*port_enable)(struct dsa_switch *ds, int port); > + void (*port_disable)(struct dsa_switch *ds, int port); > > /* > * Port's MAC EEE settings > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 606812160fd5..6290741e496a 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -100,7 +100,7 @@ static int dsa_slave_open(struct net_device *dev) > } > > if (ds->ops->port_enable) { > - err = ds->ops->port_enable(ds, p->dp->index, p->phy); > + err = ds->ops->port_enable(ds, p->dp->index); > if (err) > goto clear_promisc; > } > @@ -155,7 +155,7 @@ static int dsa_slave_close(struct net_device *dev) > dev_uc_del(master, dev->dev_addr); > > if (ds->ops->port_disable) > - ds->ops->port_disable(ds, p->dp->index, p->phy); > + ds->ops->port_disable(ds, p->dp->index); > > dsa_port_set_state_now(p->dp, BR_STATE_DISABLED); > > -- Florian ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next 2/4] net: dsa: remove phy arg from port enable/disable 2017-09-22 17:14 ` Florian Fainelli @ 2017-09-22 18:12 ` Vivien Didelot 2017-09-22 18:23 ` Florian Fainelli 0 siblings, 1 reply; 16+ messages in thread From: Vivien Didelot @ 2017-09-22 18:12 UTC (permalink / raw) To: Florian Fainelli, netdev Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn Hi Florian, Florian Fainelli <f.fainelli@gmail.com> writes: > On 09/22/2017 09:17 AM, Vivien Didelot wrote: >> The .port_enable and .port_disable functions are meant to deal with the >> switch ports only, and no driver is using the phy argument anyway. >> Remove it. > > I don't think this makes sense, there are perfectly legit reasons why a > switch driver may have something to do with the PHY device attached to > its per-port network interface, we should definitively keep that around, > unless you think we should be accessing the PHY within the switch > drivers by doing: > > struct phy_device *phydev = ds->ports[port].netdev->phydev? bcm_sf2 is the only user for this phy argument right now. The reason I'm doing this is because I prefer to discourage switch drivers to dig into the phy device themselves while as you said there must be a cleaner solution. This must be handled somehow elsewhere in the stack. In the meantime, moving the PHY device up to the dsa_port structure is a good solution, in order not to expose it in switch ops, but still make it available to more complex drivers. Do you know if netdev->phydev is usable? Why do DSA has its own copy in dsa_slave_priv then? I'll respin, thanks. Vivien ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next 2/4] net: dsa: remove phy arg from port enable/disable 2017-09-22 18:12 ` Vivien Didelot @ 2017-09-22 18:23 ` Florian Fainelli 2017-09-22 19:11 ` Andrew Lunn 0 siblings, 1 reply; 16+ messages in thread From: Florian Fainelli @ 2017-09-22 18:23 UTC (permalink / raw) To: Vivien Didelot, netdev; +Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn On 09/22/2017 11:12 AM, Vivien Didelot wrote: > Hi Florian, > > Florian Fainelli <f.fainelli@gmail.com> writes: > >> On 09/22/2017 09:17 AM, Vivien Didelot wrote: >>> The .port_enable and .port_disable functions are meant to deal with the >>> switch ports only, and no driver is using the phy argument anyway. >>> Remove it. >> >> I don't think this makes sense, there are perfectly legit reasons why a >> switch driver may have something to do with the PHY device attached to >> its per-port network interface, we should definitively keep that around, >> unless you think we should be accessing the PHY within the switch >> drivers by doing: >> >> struct phy_device *phydev = ds->ports[port].netdev->phydev? > > bcm_sf2 is the only user for this phy argument right now. The reason I'm > doing this is because I prefer to discourage switch drivers to dig into > the phy device themselves while as you said there must be a cleaner > solution. This must be handled somehow elsewhere in the stack. The current approach of passing the phy_device reference as an argument is certainly a cleaner way then. The port_enable caller can provide the correct phy_device and that lifts the switch driver from having to dig it itself from its per-port netdev. > > In the meantime, moving the PHY device up to the dsa_port structure is a > good solution, in order not to expose it in switch ops, but still make > it available to more complex drivers. > > Do you know if netdev->phydev is usable? Why do DSA has its own copy in > dsa_slave_priv then? Historical reasons mostly. Considering the complexity of dsa_slave_phy_setup(), I would certainly be extremely careful in changing any of this, the potential for breakage is pretty big. At first glance, I would say that this is a safe conversion to do, and I can test this on the HW I have here anyway. -- Florian ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next 2/4] net: dsa: remove phy arg from port enable/disable 2017-09-22 18:23 ` Florian Fainelli @ 2017-09-22 19:11 ` Andrew Lunn 0 siblings, 0 replies; 16+ messages in thread From: Andrew Lunn @ 2017-09-22 19:11 UTC (permalink / raw) To: Florian Fainelli Cc: Vivien Didelot, netdev, linux-kernel, kernel, David S. Miller > Historical reasons mostly. Considering the complexity of > dsa_slave_phy_setup(), I would certainly be extremely careful in > changing any of this, the potential for breakage is pretty big. Yes, i took a look at this, wondering how to convert to phylink. I went away and got a stiff drink :-) Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH net-next 3/4] net: dsa: make slave close symmetrical to open 2017-09-22 16:17 [PATCH net-next 0/4] net: dsa: simplify port enabling Vivien Didelot 2017-09-22 16:17 ` [PATCH net-next 1/4] net: dsa: move up phy enabling in core Vivien Didelot 2017-09-22 16:17 ` [PATCH net-next 2/4] net: dsa: remove phy arg from port enable/disable Vivien Didelot @ 2017-09-22 16:17 ` Vivien Didelot 2017-09-22 16:34 ` Andrew Lunn 2017-09-22 17:15 ` Florian Fainelli 2017-09-22 16:17 ` [PATCH net-next 4/4] net: dsa: add port enable and disable helpers Vivien Didelot 3 siblings, 2 replies; 16+ messages in thread From: Vivien Didelot @ 2017-09-22 16:17 UTC (permalink / raw) To: netdev Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli, Andrew Lunn, Vivien Didelot The DSA slave open function configures the unicast MAC addresses on the master device, enable the switch port, change its STP state, then start the PHY device. Make the close function symmetric, by first stopping the PHY device, then changing the STP state, disabling the switch port and restore the master device. Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> --- net/dsa/slave.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 6290741e496a..235a5c95dfcc 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -144,6 +144,11 @@ static int dsa_slave_close(struct net_device *dev) if (p->phy) phy_stop(p->phy); + dsa_port_set_state_now(p->dp, BR_STATE_DISABLED); + + if (ds->ops->port_disable) + ds->ops->port_disable(ds, p->dp->index); + dev_mc_unsync(master, dev); dev_uc_unsync(master, dev); if (dev->flags & IFF_ALLMULTI) @@ -154,11 +159,6 @@ static int dsa_slave_close(struct net_device *dev) if (!ether_addr_equal(dev->dev_addr, master->dev_addr)) dev_uc_del(master, dev->dev_addr); - if (ds->ops->port_disable) - ds->ops->port_disable(ds, p->dp->index); - - dsa_port_set_state_now(p->dp, BR_STATE_DISABLED); - return 0; } -- 2.14.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH net-next 3/4] net: dsa: make slave close symmetrical to open 2017-09-22 16:17 ` [PATCH net-next 3/4] net: dsa: make slave close symmetrical to open Vivien Didelot @ 2017-09-22 16:34 ` Andrew Lunn 2017-09-22 17:15 ` Florian Fainelli 1 sibling, 0 replies; 16+ messages in thread From: Andrew Lunn @ 2017-09-22 16:34 UTC (permalink / raw) To: Vivien Didelot Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli On Fri, Sep 22, 2017 at 12:17:52PM -0400, Vivien Didelot wrote: > The DSA slave open function configures the unicast MAC addresses on the > master device, enable the switch port, change its STP state, then start > the PHY device. > > Make the close function symmetric, by first stopping the PHY device, > then changing the STP state, disabling the switch port and restore the > master device. > > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next 3/4] net: dsa: make slave close symmetrical to open 2017-09-22 16:17 ` [PATCH net-next 3/4] net: dsa: make slave close symmetrical to open Vivien Didelot 2017-09-22 16:34 ` Andrew Lunn @ 2017-09-22 17:15 ` Florian Fainelli 1 sibling, 0 replies; 16+ messages in thread From: Florian Fainelli @ 2017-09-22 17:15 UTC (permalink / raw) To: Vivien Didelot, netdev; +Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn On 09/22/2017 09:17 AM, Vivien Didelot wrote: > The DSA slave open function configures the unicast MAC addresses on the > master device, enable the switch port, change its STP state, then start > the PHY device. > > Make the close function symmetric, by first stopping the PHY device, > then changing the STP state, disabling the switch port and restore the > master device. > > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH net-next 4/4] net: dsa: add port enable and disable helpers 2017-09-22 16:17 [PATCH net-next 0/4] net: dsa: simplify port enabling Vivien Didelot ` (2 preceding siblings ...) 2017-09-22 16:17 ` [PATCH net-next 3/4] net: dsa: make slave close symmetrical to open Vivien Didelot @ 2017-09-22 16:17 ` Vivien Didelot 2017-09-22 16:35 ` Andrew Lunn 2017-09-22 17:16 ` Florian Fainelli 3 siblings, 2 replies; 16+ messages in thread From: Vivien Didelot @ 2017-09-22 16:17 UTC (permalink / raw) To: netdev Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli, Andrew Lunn, Vivien Didelot Provide dsa_port_enable and dsa_port_disable helpers to respectively enable and disable a switch port. This makes the dsa_port_set_state_now helper static. Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> --- net/dsa/dsa_priv.h | 3 ++- net/dsa/port.c | 31 ++++++++++++++++++++++++++++++- net/dsa/slave.c | 19 +++++-------------- 3 files changed, 37 insertions(+), 16 deletions(-) diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index 9803952a5b40..6bfff19d1615 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -117,7 +117,8 @@ void dsa_master_ethtool_restore(struct net_device *dev); /* port.c */ int dsa_port_set_state(struct dsa_port *dp, u8 state, struct switchdev_trans *trans); -void dsa_port_set_state_now(struct dsa_port *dp, u8 state); +int dsa_port_enable(struct dsa_port *dp); +void dsa_port_disable(struct dsa_port *dp); int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br); void dsa_port_bridge_leave(struct dsa_port *dp, struct net_device *br); int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering, diff --git a/net/dsa/port.c b/net/dsa/port.c index 76d43a82d397..50749339e252 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -56,7 +56,7 @@ int dsa_port_set_state(struct dsa_port *dp, u8 state, return 0; } -void dsa_port_set_state_now(struct dsa_port *dp, u8 state) +static void dsa_port_set_state_now(struct dsa_port *dp, u8 state) { int err; @@ -65,6 +65,35 @@ void dsa_port_set_state_now(struct dsa_port *dp, u8 state) pr_err("DSA: failed to set STP state %u (%d)\n", state, err); } +int dsa_port_enable(struct dsa_port *dp) +{ + u8 stp_state = dp->bridge_dev ? BR_STATE_BLOCKING : BR_STATE_FORWARDING; + struct dsa_switch *ds = dp->ds; + int port = dp->index; + int err; + + if (ds->ops->port_enable) { + err = ds->ops->port_enable(ds, port); + if (err) + return err; + } + + dsa_port_set_state_now(dp, stp_state); + + return 0; +} + +void dsa_port_disable(struct dsa_port *dp) +{ + struct dsa_switch *ds = dp->ds; + int port = dp->index; + + dsa_port_set_state_now(dp, BR_STATE_DISABLED); + + if (ds->ops->port_disable) + ds->ops->port_disable(ds, port); +} + int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br) { struct dsa_notifier_bridge_info info = { diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 235a5c95dfcc..e40623939323 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -74,9 +74,7 @@ static int dsa_slave_open(struct net_device *dev) struct dsa_slave_priv *p = netdev_priv(dev); struct phy_device *phy = p->phy; struct dsa_port *dp = p->dp; - struct dsa_switch *ds = dp->ds; struct net_device *master = dsa_master_netdev(p); - u8 stp_state = dp->bridge_dev ? BR_STATE_BLOCKING : BR_STATE_FORWARDING; int err; if (!(master->flags & IFF_UP)) @@ -99,13 +97,9 @@ static int dsa_slave_open(struct net_device *dev) goto clear_allmulti; } - if (ds->ops->port_enable) { - err = ds->ops->port_enable(ds, p->dp->index); - if (err) - goto clear_promisc; - } - - dsa_port_set_state_now(p->dp, stp_state); + err = dsa_port_enable(dp); + if (err) + goto clear_promisc; if (phy) { /* If phy_stop() has been called before, phy will be in @@ -139,15 +133,12 @@ static int dsa_slave_close(struct net_device *dev) { struct dsa_slave_priv *p = netdev_priv(dev); struct net_device *master = dsa_master_netdev(p); - struct dsa_switch *ds = p->dp->ds; + struct dsa_port *dp = p->dp; if (p->phy) phy_stop(p->phy); - dsa_port_set_state_now(p->dp, BR_STATE_DISABLED); - - if (ds->ops->port_disable) - ds->ops->port_disable(ds, p->dp->index); + dsa_port_disable(dp); dev_mc_unsync(master, dev); dev_uc_unsync(master, dev); -- 2.14.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH net-next 4/4] net: dsa: add port enable and disable helpers 2017-09-22 16:17 ` [PATCH net-next 4/4] net: dsa: add port enable and disable helpers Vivien Didelot @ 2017-09-22 16:35 ` Andrew Lunn 2017-09-22 17:16 ` Florian Fainelli 1 sibling, 0 replies; 16+ messages in thread From: Andrew Lunn @ 2017-09-22 16:35 UTC (permalink / raw) To: Vivien Didelot Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli On Fri, Sep 22, 2017 at 12:17:53PM -0400, Vivien Didelot wrote: > Provide dsa_port_enable and dsa_port_disable helpers to respectively > enable and disable a switch port. This makes the dsa_port_set_state_now > helper static. > > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next 4/4] net: dsa: add port enable and disable helpers 2017-09-22 16:17 ` [PATCH net-next 4/4] net: dsa: add port enable and disable helpers Vivien Didelot 2017-09-22 16:35 ` Andrew Lunn @ 2017-09-22 17:16 ` Florian Fainelli 1 sibling, 0 replies; 16+ messages in thread From: Florian Fainelli @ 2017-09-22 17:16 UTC (permalink / raw) To: Vivien Didelot, netdev; +Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn On 09/22/2017 09:17 AM, Vivien Didelot wrote: > Provide dsa_port_enable and dsa_port_disable helpers to respectively > enable and disable a switch port. This makes the dsa_port_set_state_now > helper static. > > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2017-09-22 19:11 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-09-22 16:17 [PATCH net-next 0/4] net: dsa: simplify port enabling Vivien Didelot 2017-09-22 16:17 ` [PATCH net-next 1/4] net: dsa: move up phy enabling in core Vivien Didelot 2017-09-22 16:32 ` Andrew Lunn 2017-09-22 16:58 ` Florian Fainelli 2017-09-22 16:52 ` Florian Fainelli 2017-09-22 16:17 ` [PATCH net-next 2/4] net: dsa: remove phy arg from port enable/disable Vivien Didelot 2017-09-22 17:14 ` Florian Fainelli 2017-09-22 18:12 ` Vivien Didelot 2017-09-22 18:23 ` Florian Fainelli 2017-09-22 19:11 ` Andrew Lunn 2017-09-22 16:17 ` [PATCH net-next 3/4] net: dsa: make slave close symmetrical to open Vivien Didelot 2017-09-22 16:34 ` Andrew Lunn 2017-09-22 17:15 ` Florian Fainelli 2017-09-22 16:17 ` [PATCH net-next 4/4] net: dsa: add port enable and disable helpers Vivien Didelot 2017-09-22 16:35 ` Andrew Lunn 2017-09-22 17:16 ` Florian Fainelli
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).