| Message ID | 20231019122850.1199821-6-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: microchip: provide Wake on LAN support | expand |
On 10/19/23 05:28, Oleksij Rempel wrote: > Introduce Wake on Magic Packet (WoL) functionality to the ksz9477 > driver. > > Major changes include: > > 1. Extending the `ksz9477_handle_wake_reason` function to identify Magic > Packet wake events alongside existing wake reasons. > > 2. Updating the `ksz9477_get_wol` and `ksz9477_set_wol` functions to > handle WAKE_MAGIC alongside the existing WAKE_PHY option, and to > program the switch's MAC address register accordingly when Magic > Packet wake-up is enabled. This change will prevent WAKE_MAGIC > activation if the related port has a different MAC address compared > to a MAC address already used by HSR or an already active WAKE_MAGIC > on another port. > > 3. Adding a restriction in `ksz_port_set_mac_address` to prevent MAC > address changes on ports with active Wake on Magic Packet, as the > switch's MAC address register is utilized for this feature. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
On Thu, Oct 19, 2023 at 02:28:46PM +0200, Oleksij Rempel wrote: > Introduce Wake on Magic Packet (WoL) functionality to the ksz9477 > driver. > > Major changes include: > > 1. Extending the `ksz9477_handle_wake_reason` function to identify Magic > Packet wake events alongside existing wake reasons. > > 2. Updating the `ksz9477_get_wol` and `ksz9477_set_wol` functions to > handle WAKE_MAGIC alongside the existing WAKE_PHY option, and to > program the switch's MAC address register accordingly when Magic > Packet wake-up is enabled. This change will prevent WAKE_MAGIC > activation if the related port has a different MAC address compared > to a MAC address already used by HSR or an already active WAKE_MAGIC > on another port. > > 3. Adding a restriction in `ksz_port_set_mac_address` to prevent MAC > address changes on ports with active Wake on Magic Packet, as the > switch's MAC address register is utilized for this feature. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/dsa/microchip/ksz9477.c | 60 ++++++++++++++++++++++++-- > drivers/net/dsa/microchip/ksz_common.c | 15 +++++-- > drivers/net/dsa/microchip/ksz_common.h | 3 ++ > 3 files changed, 71 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c > index b9419d4b5e7b..bcc8863951ca 100644 > --- a/drivers/net/dsa/microchip/ksz9477.c > +++ b/drivers/net/dsa/microchip/ksz9477.c > @@ -81,7 +81,8 @@ static int ksz9477_handle_wake_reason(struct ksz_device *dev, int port) > if (!pme_status) > return 0; > > - dev_dbg(dev->dev, "Wake event on port %d due to: %s %s\n", port, > + dev_dbg(dev->dev, "Wake event on port %d due to: %s %s %s\n", port, > + pme_status & PME_WOL_MAGICPKT ? "\"Magic Packet\"" : "", > pme_status & PME_WOL_LINKUP ? "\"Link Up\"" : "", > pme_status & PME_WOL_ENERGY ? "\"Enery detect\"" : ""); Trivial: if you format the printf string as %s%s%s and the arguments as "\"Magic Packet\" " : "", then the printed line won't have a trailing space at the end. > > @@ -109,10 +110,22 @@ void ksz9477_get_wol(struct ksz_device *dev, int port, > > wol->supported = WAKE_PHY; > > + /* Check if at this moment we would be able to get the MAC address > + * and use it for WAKE_MAGIC support. This result may change dynamically > + * depending on configuration of other ports. > + */ > + ret = ksz_switch_macaddr_get(dev->ds, port, NULL); > + if (!ret) { > + wol->supported |= WAKE_MAGIC; > + ksz_switch_macaddr_put(dev->ds); I don't get it, why do you release the reference on the MAC address as soon as you successfully get it? Without a reference held, the programmed address still lingers on, but the HSR offload code, on a different port with a different MAC address, can change it and break WoL. > + } > + > ret = ksz_pread8(dev, port, REG_PORT_PME_CTRL, &pme_ctrl); > if (ret) > return; > > + if (pme_ctrl & PME_WOL_MAGICPKT) > + wol->wolopts |= WAKE_MAGIC; > if (pme_ctrl & (PME_WOL_LINKUP | PME_WOL_ENERGY)) > wol->wolopts |= WAKE_PHY; > }
On Thu, Oct 19, 2023 at 08:29:53PM +0300, Vladimir Oltean wrote: > On Thu, Oct 19, 2023 at 02:28:46PM +0200, Oleksij Rempel wrote: > > Introduce Wake on Magic Packet (WoL) functionality to the ksz9477 > > driver. > > > > Major changes include: > > > > 1. Extending the `ksz9477_handle_wake_reason` function to identify Magic > > Packet wake events alongside existing wake reasons. > > > > 2. Updating the `ksz9477_get_wol` and `ksz9477_set_wol` functions to > > handle WAKE_MAGIC alongside the existing WAKE_PHY option, and to > > program the switch's MAC address register accordingly when Magic > > Packet wake-up is enabled. This change will prevent WAKE_MAGIC > > activation if the related port has a different MAC address compared > > to a MAC address already used by HSR or an already active WAKE_MAGIC > > on another port. > > > > 3. Adding a restriction in `ksz_port_set_mac_address` to prevent MAC > > address changes on ports with active Wake on Magic Packet, as the > > switch's MAC address register is utilized for this feature. > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > --- > > drivers/net/dsa/microchip/ksz9477.c | 60 ++++++++++++++++++++++++-- > > drivers/net/dsa/microchip/ksz_common.c | 15 +++++-- > > drivers/net/dsa/microchip/ksz_common.h | 3 ++ > > 3 files changed, 71 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c > > index b9419d4b5e7b..bcc8863951ca 100644 > > --- a/drivers/net/dsa/microchip/ksz9477.c > > +++ b/drivers/net/dsa/microchip/ksz9477.c > > @@ -81,7 +81,8 @@ static int ksz9477_handle_wake_reason(struct ksz_device *dev, int port) > > if (!pme_status) > > return 0; > > > > - dev_dbg(dev->dev, "Wake event on port %d due to: %s %s\n", port, > > + dev_dbg(dev->dev, "Wake event on port %d due to: %s %s %s\n", port, > > + pme_status & PME_WOL_MAGICPKT ? "\"Magic Packet\"" : "", > > pme_status & PME_WOL_LINKUP ? "\"Link Up\"" : "", > > pme_status & PME_WOL_ENERGY ? "\"Enery detect\"" : ""); > > Trivial: if you format the printf string as %s%s%s and the arguments as > "\"Magic Packet\" " : "", then the printed line won't have a trailing > space at the end. Sadly, it still will. The best solution is to prepend the space character to each entry in the "list" and remove the space characters after the : in the format string thusly: dev_dbg(dev->dev, "Wake event on port %d due to:%s%s%s\n", port, pme_status & PME_WOL_MAGICPKT ? " \"Magic Packet\"" : "", pme_status & PME_WOL_LINKUP ? " \"Link Up\"" : "", pme_status & PME_WOL_ENERGY ? " \"Enery detect\"" : "");
On Thu, Oct 19, 2023 at 08:29:53PM +0300, Vladimir Oltean wrote: > On Thu, Oct 19, 2023 at 02:28:46PM +0200, Oleksij Rempel wrote: .... > > @@ -109,10 +110,22 @@ void ksz9477_get_wol(struct ksz_device *dev, int port, > > > > wol->supported = WAKE_PHY; > > > > + /* Check if at this moment we would be able to get the MAC address > > + * and use it for WAKE_MAGIC support. This result may change dynamically > > + * depending on configuration of other ports. > > + */ > > + ret = ksz_switch_macaddr_get(dev->ds, port, NULL); > > + if (!ret) { > > + wol->supported |= WAKE_MAGIC; > > + ksz_switch_macaddr_put(dev->ds); > > I don't get it, why do you release the reference on the MAC address as > soon as you successfully get it? Without a reference held, the > programmed address still lingers on, but the HSR offload code, on a > different port with a different MAC address, can change it and break WoL. It is ksz9477_get_wol() function. We do not actually need to program here the MAC address, we only need to test if we would be able to get it. To show the use more or less correct information on WoL capabilities. For example, instead showing the user that Wake on Magic is supported, where we already know that is not the case, we can already show correct information. May be it will be better to have extra option for ksz_switch_macaddr_get() to not allocate and do the refcounting or have a separate function. Regards, Oleksij
On Thu, Oct 19, 2023 at 07:17:32PM +0100, Russell King (Oracle) wrote: > On Thu, Oct 19, 2023 at 08:29:53PM +0300, Vladimir Oltean wrote: > > > - dev_dbg(dev->dev, "Wake event on port %d due to: %s %s\n", port, > > > + dev_dbg(dev->dev, "Wake event on port %d due to: %s %s %s\n", port, > > > + pme_status & PME_WOL_MAGICPKT ? "\"Magic Packet\"" : "", > > > pme_status & PME_WOL_LINKUP ? "\"Link Up\"" : "", > > > pme_status & PME_WOL_ENERGY ? "\"Enery detect\"" : ""); > > > > Trivial: if you format the printf string as %s%s%s and the arguments as > > "\"Magic Packet\" " : "", then the printed line won't have a trailing > > space at the end. > > Sadly, it still will. The best solution is to prepend the space > character to each entry in the "list" and remove the space characters > after the : in the format string thusly: > > dev_dbg(dev->dev, "Wake event on port %d due to:%s%s%s\n", port, > pme_status & PME_WOL_MAGICPKT ? " \"Magic Packet\"" : "", > pme_status & PME_WOL_LINKUP ? " \"Link Up\"" : "", > pme_status & PME_WOL_ENERGY ? " \"Enery detect\"" : ""); Thanks for correcting me.
On Fri, Oct 20, 2023 at 07:08:56AM +0200, Oleksij Rempel wrote: > On Thu, Oct 19, 2023 at 08:29:53PM +0300, Vladimir Oltean wrote: > > I don't get it, why do you release the reference on the MAC address as > > soon as you successfully get it? Without a reference held, the > > programmed address still lingers on, but the HSR offload code, on a > > different port with a different MAC address, can change it and break WoL. > > It is ksz9477_get_wol() function. We do not actually need to program > here the MAC address, we only need to test if we would be able to get > it. To show the use more or less correct information on WoL > capabilities. For example, instead showing the user that Wake on Magic > is supported, where we already know that is not the case, we can already > show correct information. May be it will be better to have > extra option for ksz_switch_macaddr_get() to not allocate and do the > refcounting or have a separate function. Ah, yes, it is from get_wol(). Maybe a ksz_switch_macaddr_tryget(ds, port) which returns bool (true if dev->switch_macaddr is NULL, or if non-NULL and ether_addr_equal(dev->switch_macaddr->addr, port addr))?
On Fri, Oct 20, 2023 at 11:23:50AM +0300, Vladimir Oltean wrote: > On Fri, Oct 20, 2023 at 07:08:56AM +0200, Oleksij Rempel wrote: > > On Thu, Oct 19, 2023 at 08:29:53PM +0300, Vladimir Oltean wrote: > > > I don't get it, why do you release the reference on the MAC address as > > > soon as you successfully get it? Without a reference held, the > > > programmed address still lingers on, but the HSR offload code, on a > > > different port with a different MAC address, can change it and break WoL. > > > > It is ksz9477_get_wol() function. We do not actually need to program > > here the MAC address, we only need to test if we would be able to get > > it. To show the use more or less correct information on WoL > > capabilities. For example, instead showing the user that Wake on Magic > > is supported, where we already know that is not the case, we can already > > show correct information. May be it will be better to have > > extra option for ksz_switch_macaddr_get() to not allocate and do the > > refcounting or have a separate function. > > Ah, yes, it is from get_wol(). Maybe a ksz_switch_macaddr_tryget(ds, port) > which returns bool (true if dev->switch_macaddr is NULL, or if non-NULL > and ether_addr_equal(dev->switch_macaddr->addr, port addr))? Hmm, I've checked other uses of the "tryget" name in the kernel and their semantics all seem to imply that upon success, a reference is acquired. That would not be the case here, so the naming would be confusing. How about a bool ksz_switch_macaddr_check(ds, port)?
On Fri, Oct 20, 2023 at 11:23:50AM +0300, Vladimir Oltean wrote: > On Fri, Oct 20, 2023 at 07:08:56AM +0200, Oleksij Rempel wrote: > > On Thu, Oct 19, 2023 at 08:29:53PM +0300, Vladimir Oltean wrote: > > > I don't get it, why do you release the reference on the MAC address as > > > soon as you successfully get it? Without a reference held, the > > > programmed address still lingers on, but the HSR offload code, on a > > > different port with a different MAC address, can change it and break WoL. > > > > It is ksz9477_get_wol() function. We do not actually need to program > > here the MAC address, we only need to test if we would be able to get > > it. To show the use more or less correct information on WoL > > capabilities. For example, instead showing the user that Wake on Magic > > is supported, where we already know that is not the case, we can already > > show correct information. May be it will be better to have > > extra option for ksz_switch_macaddr_get() to not allocate and do the > > refcounting or have a separate function. > > Ah, yes, it is from get_wol(). Maybe a ksz_switch_macaddr_tryget(ds, port) > which returns bool (true if dev->switch_macaddr is NULL, or if non-NULL > and ether_addr_equal(dev->switch_macaddr->addr, port addr))? Ack, something like this. I'll send new version later.
> > Ah, yes, it is from get_wol(). Maybe a ksz_switch_macaddr_tryget(ds, port) > > which returns bool (true if dev->switch_macaddr is NULL, or if non-NULL > > and ether_addr_equal(dev->switch_macaddr->addr, port addr))? > > Ack, something like this. > I'll send new version later. And maybe add a comment. Seems like everybody got it wrong what is going on here. Maybe i should not of suggested it :-) Andrew
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c index b9419d4b5e7b..bcc8863951ca 100644 --- a/drivers/net/dsa/microchip/ksz9477.c +++ b/drivers/net/dsa/microchip/ksz9477.c @@ -81,7 +81,8 @@ static int ksz9477_handle_wake_reason(struct ksz_device *dev, int port) if (!pme_status) return 0; - dev_dbg(dev->dev, "Wake event on port %d due to: %s %s\n", port, + dev_dbg(dev->dev, "Wake event on port %d due to: %s %s %s\n", port, + pme_status & PME_WOL_MAGICPKT ? "\"Magic Packet\"" : "", pme_status & PME_WOL_LINKUP ? "\"Link Up\"" : "", pme_status & PME_WOL_ENERGY ? "\"Enery detect\"" : ""); @@ -109,10 +110,22 @@ void ksz9477_get_wol(struct ksz_device *dev, int port, wol->supported = WAKE_PHY; + /* Check if at this moment we would be able to get the MAC address + * and use it for WAKE_MAGIC support. This result may change dynamically + * depending on configuration of other ports. + */ + ret = ksz_switch_macaddr_get(dev->ds, port, NULL); + if (!ret) { + wol->supported |= WAKE_MAGIC; + ksz_switch_macaddr_put(dev->ds); + } + ret = ksz_pread8(dev, port, REG_PORT_PME_CTRL, &pme_ctrl); if (ret) return; + if (pme_ctrl & PME_WOL_MAGICPKT) + wol->wolopts |= WAKE_MAGIC; if (pme_ctrl & (PME_WOL_LINKUP | PME_WOL_ENERGY)) wol->wolopts |= WAKE_PHY; } @@ -134,10 +147,12 @@ void ksz9477_get_wol(struct ksz_device *dev, int port, int ksz9477_set_wol(struct ksz_device *dev, int port, struct ethtool_wolinfo *wol) { - u8 pme_ctrl = 0; + u8 pme_ctrl = 0, pme_ctrl_old = 0; + bool magic_switched_off; + bool magic_switched_on; int ret; - if (wol->wolopts & ~WAKE_PHY) + if (wol->wolopts & ~(WAKE_PHY | WAKE_MAGIC)) return -EINVAL; if (!dev->wakeup_source) @@ -147,10 +162,42 @@ int ksz9477_set_wol(struct ksz_device *dev, int port, if (ret) return ret; + if (wol->wolopts & WAKE_MAGIC) + pme_ctrl |= PME_WOL_MAGICPKT; if (wol->wolopts & WAKE_PHY) pme_ctrl |= PME_WOL_LINKUP | PME_WOL_ENERGY; - return ksz_pwrite8(dev, port, REG_PORT_PME_CTRL, pme_ctrl); + ret = ksz_pread8(dev, port, REG_PORT_PME_CTRL, &pme_ctrl_old); + if (ret) + return ret; + + if (pme_ctrl_old == pme_ctrl) + return 0; + + magic_switched_off = (pme_ctrl_old & PME_WOL_MAGICPKT) && + !(pme_ctrl & PME_WOL_MAGICPKT); + magic_switched_on = !(pme_ctrl_old & PME_WOL_MAGICPKT) && + (pme_ctrl & PME_WOL_MAGICPKT); + + /* To keep reference count of MAC address, we should do this + * operation only on change of WOL settings. + */ + if (magic_switched_on) { + ret = ksz_switch_macaddr_get(dev->ds, port, NULL); + if (ret) + return ret; + } else if (magic_switched_off) { + ksz_switch_macaddr_put(dev->ds); + } + + ret = ksz_pwrite8(dev, port, REG_PORT_PME_CTRL, pme_ctrl); + if (ret) { + if (magic_switched_on) + ksz_switch_macaddr_put(dev->ds); + return ret; + } + + return 0; } static int ksz9477_wait_vlan_ctrl_ready(struct ksz_device *dev) @@ -1106,6 +1153,11 @@ void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port) /* clear pending wake flags */ ksz9477_handle_wake_reason(dev, port); + + /* Disable all WoL options by default. Otherwise + * ksz_switch_macaddr_get/put logic will not work properly. + */ + ksz_pwrite8(dev, port, REG_PORT_PME_CTRL, 0); } void ksz9477_config_cpu_port(struct dsa_switch *ds) diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index 3f7c86e545a7..377998966b13 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -3569,6 +3569,7 @@ static int ksz_port_set_mac_address(struct dsa_switch *ds, int port, const unsigned char *addr) { struct dsa_port *dp = dsa_to_port(ds, port); + struct ethtool_wolinfo wol; if (dp->hsr_dev) { dev_err(ds->dev, @@ -3577,6 +3578,14 @@ static int ksz_port_set_mac_address(struct dsa_switch *ds, int port, return -EBUSY; } + ksz_get_wol(ds, dp->index, &wol); + if (wol.wolopts & WAKE_MAGIC) { + dev_err(ds->dev, + "Cannot change MAC address on port %d with active Wake on Magic Packet\n", + port); + return -EBUSY; + } + return 0; } @@ -3587,8 +3596,8 @@ static int ksz_port_set_mac_address(struct dsa_switch *ds, int port, * the same. The user ports' MAC addresses must not change while they have * ownership of the switch MAC address. */ -static int ksz_switch_macaddr_get(struct dsa_switch *ds, int port, - struct netlink_ext_ack *extack) +int ksz_switch_macaddr_get(struct dsa_switch *ds, int port, + struct netlink_ext_ack *extack) { struct net_device *slave = dsa_to_port(ds, port)->slave; const unsigned char *addr = slave->dev_addr; @@ -3628,7 +3637,7 @@ static int ksz_switch_macaddr_get(struct dsa_switch *ds, int port, return 0; } -static void ksz_switch_macaddr_put(struct dsa_switch *ds) +void ksz_switch_macaddr_put(struct dsa_switch *ds) { struct ksz_switch_macaddr *switch_macaddr; struct ksz_device *dev = ds->priv; diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h index a7394175fcf6..8fc3210d7a3d 100644 --- a/drivers/net/dsa/microchip/ksz_common.h +++ b/drivers/net/dsa/microchip/ksz_common.h @@ -396,6 +396,9 @@ void ksz_port_stp_state_set(struct dsa_switch *ds, int port, u8 state); bool ksz_get_gbit(struct ksz_device *dev, int port); phy_interface_t ksz_get_xmii(struct ksz_device *dev, int port, bool gbit); extern const struct ksz_chip_data ksz_switch_chips[]; +int ksz_switch_macaddr_get(struct dsa_switch *ds, int port, + struct netlink_ext_ack *extack); +void ksz_switch_macaddr_put(struct dsa_switch *ds); /* Common register access functions */ static inline struct regmap *ksz_regmap_8(struct ksz_device *dev)
Introduce Wake on Magic Packet (WoL) functionality to the ksz9477 driver. Major changes include: 1. Extending the `ksz9477_handle_wake_reason` function to identify Magic Packet wake events alongside existing wake reasons. 2. Updating the `ksz9477_get_wol` and `ksz9477_set_wol` functions to handle WAKE_MAGIC alongside the existing WAKE_PHY option, and to program the switch's MAC address register accordingly when Magic Packet wake-up is enabled. This change will prevent WAKE_MAGIC activation if the related port has a different MAC address compared to a MAC address already used by HSR or an already active WAKE_MAGIC on another port. 3. Adding a restriction in `ksz_port_set_mac_address` to prevent MAC address changes on ports with active Wake on Magic Packet, as the switch's MAC address register is utilized for this feature. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/dsa/microchip/ksz9477.c | 60 ++++++++++++++++++++++++-- drivers/net/dsa/microchip/ksz_common.c | 15 +++++-- drivers/net/dsa/microchip/ksz_common.h | 3 ++ 3 files changed, 71 insertions(+), 7 deletions(-)