[net-next,v6,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 | 60 ++++++++++++++++++++++++--
drivers/net/dsa/microchip/ksz_common.c | 15 +++++--
drivers/net/dsa/microchip/ksz_common.h | 3 ++
3 files changed, 71 insertions(+), 7 deletions(-)
Comments
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
@@ -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)
@@ -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;
@@ -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)