Re: [PATCH net V3 2/3] net: lan743x: Support WOL at both the PHY and MAC appropriately

[Wojciech Drewek <wojciech.drewek@intel.com>]

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@microchip.com>
> ---

Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.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;