[net-next,v4,5/9] net: dsa: microchip: ksz9477: Add Wake on Magic Packet support
Commit Message
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 | 17 ++++++++++++++---
drivers/net/dsa/microchip/ksz_common.c | 13 +++++++++++--
drivers/net/dsa/microchip/ksz_common.h | 2 ++
3 files changed, 27 insertions(+), 5 deletions(-)
Comments
On 10/16/2023 7:12 AM, 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>
> @@ -155,6 +158,14 @@ int ksz9477_set_wol(struct ksz_device *dev, int port,
> if (ret)
> return ret;
>
> + if (wol->wolopts & WAKE_MAGIC) {
> + ret = ksz_switch_macaddr_get(dev->ds, port, NULL);
> + if (ret)
> + return ret;
> +
> + pme_ctrl |= PME_WOL_MAGICPKT;
> + }
There is no matching ksz_switch_macaddr_put() here when the user turns
WAKE_MAGIC off. Ideally you need to keep track of the WoL state per
port, and when the user disables it, release the MAC address.
> +
> if (wol->wolopts & WAKE_PHY)
> pme_ctrl |= PME_WOL_LINKUP | PME_WOL_ENERGY;
>
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 3f7c86e545a7..4601aaca5179 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);
This is not really an error, as in something went wrong. Its just a
hardware restriction. So dev_warn() would be better, or nothing at all
in the logs.
Andrew
> - wol->supported = WAKE_PHY;
> + wol->supported = WAKE_PHY | WAKE_MAGIC;
It is clearly racy, but maybe try a ksz_switch_macaddr_get() and only
if it is successful add WAKE_MAGIC. Then do a
ksz_switch_macaddr_put(). Given the limitations of the hardware, it
might help the user understand what the hardware can do.
But this is not a must.
Andrew
@@ -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\"" : "");
@@ -111,12 +112,14 @@ void ksz9477_get_wol(struct ksz_device *dev, int port,
if (!(pme_conf & PME_ENABLE))
return;
- wol->supported = WAKE_PHY;
+ wol->supported = WAKE_PHY | WAKE_MAGIC;
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;
}
@@ -141,7 +144,7 @@ int ksz9477_set_wol(struct ksz_device *dev, int port,
u8 pme_conf, pme_ctrl = 0;
int ret;
- if (wol->wolopts & ~WAKE_PHY)
+ if (wol->wolopts & ~(WAKE_PHY | WAKE_MAGIC))
return -EINVAL;
ret = ksz_read8(dev, REG_SW_PME_CTRL, &pme_conf);
@@ -155,6 +158,14 @@ int ksz9477_set_wol(struct ksz_device *dev, int port,
if (ret)
return ret;
+ if (wol->wolopts & WAKE_MAGIC) {
+ ret = ksz_switch_macaddr_get(dev->ds, port, NULL);
+ if (ret)
+ return ret;
+
+ pme_ctrl |= PME_WOL_MAGICPKT;
+ }
+
if (wol->wolopts & WAKE_PHY)
pme_ctrl |= PME_WOL_LINKUP | PME_WOL_ENERGY;
@@ -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;
@@ -396,6 +396,8 @@ 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);
/* Common register access functions */
static inline struct regmap *ksz_regmap_8(struct ksz_device *dev)