[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e9985211-81a5-469f-a4bd-97385c02638b@intel.com>
Date: Wed, 5 Jun 2024 13:20:42 +0200
From: Wojciech Drewek <wojciech.drewek@...el.com>
To: Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>, <netdev@...r.kernel.org>
CC: <davem@...emloft.net>, <kuba@...nel.org>, <linux-kernel@...r.kernel.org>,
<bryan.whitehead@...rochip.com>, <andrew@...n.ch>, <linux@...linux.org.uk>,
<sbauer@...ckbox.su>, <hmehrtens@...linear.com>, <lxu@...linear.com>,
<hkallweit1@...il.com>, <edumazet@...gle.com>, <pabeni@...hat.com>,
<UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH net V3 2/3] net: lan743x: Support WOL at both the PHY and
MAC appropriately
On 05.06.2024 12:16, Raju Lakkaraju wrote:
> Prevent options not supported by the PHY from being requested to it by the MAC
> Whenever a WOL option is supported by both, the PHY is given priority
> since that usually leads to better power savings
>
> Fixes: e9e13b6adc338 ("lan743x: fix for potential NULL pointer dereference with bare card")
> Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>
> ---
Reviewed-by: Wojciech Drewek <wojciech.drewek@...el.com>
> Change List:
> ------------
> V2 -> V3:
> - Remove the "phy does not support WOL" debug message which is not required
> - Remove WAKE_PHY support option from Ethernet MAC (LAN743x/PCI11x1x) driver
> - Add "phy_wol_supported" and "phy_wolopts" variables to hold PHY's WOL config
> V1 -> V2:
> - Repost - No change
> V0 -> V1:
> - Change the "phy does not support WOL" print from netif_info() to
> netif_dbg()
>
> .../net/ethernet/microchip/lan743x_ethtool.c | 44 +++++++++++++++++--
> drivers/net/ethernet/microchip/lan743x_main.c | 16 +++++--
> drivers/net/ethernet/microchip/lan743x_main.h | 4 ++
> 3 files changed, 56 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> index d0f4ff4ee075..0d1740d64676 100644
> --- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
> +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> @@ -1127,8 +1127,12 @@ static void lan743x_ethtool_get_wol(struct net_device *netdev,
> if (netdev->phydev)
> phy_ethtool_get_wol(netdev->phydev, wol);
>
> - wol->supported |= WAKE_BCAST | WAKE_UCAST | WAKE_MCAST |
> - WAKE_MAGIC | WAKE_PHY | WAKE_ARP;
> + if (wol->supported != adapter->phy_wol_supported)
> + netif_warn(adapter, drv, adapter->netdev,
> + "PHY changed its supported WOL! old=%x, new=%x\n",
> + adapter->phy_wol_supported, wol->supported);
> +
> + wol->supported |= MAC_SUPPORTED_WAKES;
>
> if (adapter->is_pci11x1x)
> wol->supported |= WAKE_MAGICSECURE;
> @@ -1143,7 +1147,39 @@ static int lan743x_ethtool_set_wol(struct net_device *netdev,
> {
> struct lan743x_adapter *adapter = netdev_priv(netdev);
>
> + /* WAKE_MAGICSEGURE is a modifier of and only valid together with
> + * WAKE_MAGIC
> + */
> + if ((wol->wolopts & WAKE_MAGICSECURE) && !(wol->wolopts & WAKE_MAGIC))
> + return -EINVAL;
> +
> + if (netdev->phydev) {
> + struct ethtool_wolinfo phy_wol;
> + int ret;
> +
> + phy_wol.wolopts = wol->wolopts & adapter->phy_wol_supported;
> +
> + /* If WAKE_MAGICSECURE was requested, filter out WAKE_MAGIC
> + * for PHYs that do not support WAKE_MAGICSECURE
> + */
> + if (wol->wolopts & WAKE_MAGICSECURE &&
> + !(adapter->phy_wol_supported & WAKE_MAGICSECURE))
> + phy_wol.wolopts &= ~WAKE_MAGIC;
> +
> + ret = phy_ethtool_set_wol(netdev->phydev, &phy_wol);
> + if (ret && (ret != -EOPNOTSUPP))
> + return ret;
> +
> + if (ret == -EOPNOTSUPP)
> + adapter->phy_wolopts = 0;
> + else
> + adapter->phy_wolopts = phy_wol.wolopts;
> + } else {
> + adapter->phy_wolopts = 0;
> + }
> +
> adapter->wolopts = 0;
> + wol->wolopts &= ~adapter->phy_wolopts;
> if (wol->wolopts & WAKE_UCAST)
> adapter->wolopts |= WAKE_UCAST;
> if (wol->wolopts & WAKE_MCAST)
> @@ -1164,10 +1200,10 @@ static int lan743x_ethtool_set_wol(struct net_device *netdev,
> memset(adapter->sopass, 0, sizeof(u8) * SOPASS_MAX);
> }
>
> + wol->wolopts = adapter->wolopts | adapter->phy_wolopts;
> device_set_wakeup_enable(&adapter->pdev->dev, (bool)wol->wolopts);
>
> - return netdev->phydev ? phy_ethtool_set_wol(netdev->phydev, wol)
> - : -ENETDOWN;
> + return 0;
> }
> #endif /* CONFIG_PM */
>
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> index 6a40b961fafb..b6810840bc61 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -3118,6 +3118,15 @@ static int lan743x_netdev_open(struct net_device *netdev)
> if (ret)
> goto close_tx;
> }
> +
> + if (adapter->netdev->phydev) {
> + struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
> +
> + phy_ethtool_get_wol(netdev->phydev, &wol);
> + adapter->phy_wol_supported = wol.supported;
> + adapter->phy_wolopts = wol.wolopts;
> + }
> +
> return 0;
>
> close_tx:
> @@ -3587,10 +3596,9 @@ static void lan743x_pm_set_wol(struct lan743x_adapter *adapter)
>
> pmtctl |= PMT_CTL_ETH_PHY_D3_COLD_OVR_ | PMT_CTL_ETH_PHY_D3_OVR_;
>
> - if (adapter->wolopts & WAKE_PHY) {
> - pmtctl |= PMT_CTL_ETH_PHY_EDPD_PLL_CTL_;
> + if (adapter->phy_wolopts)
> pmtctl |= PMT_CTL_ETH_PHY_WAKE_EN_;
> - }
> +
> if (adapter->wolopts & WAKE_MAGIC) {
> wucsr |= MAC_WUCSR_MPEN_;
> macrx |= MAC_RX_RXEN_;
> @@ -3686,7 +3694,7 @@ static int lan743x_pm_suspend(struct device *dev)
> lan743x_csr_write(adapter, MAC_WUCSR2, 0);
> lan743x_csr_write(adapter, MAC_WK_SRC, 0xFFFFFFFF);
>
> - if (adapter->wolopts)
> + if (adapter->wolopts || adapter->phy_wolopts)
> lan743x_pm_set_wol(adapter);
>
> if (adapter->is_pci11x1x) {
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
> index fac0f33d10b2..3b2585a384e2 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.h
> +++ b/drivers/net/ethernet/microchip/lan743x_main.h
> @@ -1042,6 +1042,8 @@ enum lan743x_sgmii_lsd {
> LINK_2500_SLAVE
> };
>
> +#define MAC_SUPPORTED_WAKES (WAKE_BCAST | WAKE_UCAST | WAKE_MCAST | \
> + WAKE_MAGIC | WAKE_ARP)
> struct lan743x_adapter {
> struct net_device *netdev;
> struct mii_bus *mdiobus;
> @@ -1049,6 +1051,8 @@ struct lan743x_adapter {
> #ifdef CONFIG_PM
> u32 wolopts;
> u8 sopass[SOPASS_MAX];
> + u32 phy_wolopts;
> + u32 phy_wol_supported;
> #endif
> struct pci_dev *pdev;
> struct lan743x_csr csr;
Powered by blists - more mailing lists