Messages in this thread | | | Date | Wed, 18 Oct 2023 11:20:50 -0700 | Subject | Re: [PATCH net-next v5 5/9] net: dsa: microchip: ksz9477: Add Wake on Magic Packet support | From | Florian Fainelli <> |
| |
On 10/18/23 04:39, 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>
This looks good to me, just one suggestion below
[snip]
> + if (pme_ctrl_old == pme_ctrl) > + return 0; > + > + /* To keep reference count of MAC address, we should do this > + * operation only on change of WOL settings. > + */ > + if (!(pme_ctrl_old & PME_WOL_MAGICPKT) && > + (pme_ctrl & PME_WOL_MAGICPKT)) {
Maybe use a temporary variable for that condition since you re-use it below in case you failed to perform the write of the pme_ctrl value. It would be more readable IMHO, something like:
bool magicpkt_was_disabled = !(pme_ctrl_old & PME_WOL_MAGICPKT) && (pme_ctrl & PME_WOL_MAGICPKT)); -- Florian
| |