Received-SPF: pass (google.com: domain of linux-kernel+bounces-202255-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <e9985211-81a5-469f-a4bd-97385c02638b@intel.com>
Date: Wed, 5 Jun 2024 13:20:42 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net V3 2/3] net: lan743x: Support WOL at both the PHY and
 MAC appropriately
To: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>, <netdev@vger.kernel.org>
CC: <davem@davemloft.net>, <kuba@kernel.org>, <linux-kernel@vger.kernel.org>,
	<bryan.whitehead@microchip.com>, <andrew@lunn.ch>, <linux@armlinux.org.uk>,
	<sbauer@blackbox.su>, <hmehrtens@maxlinear.com>, <lxu@maxlinear.com>,
	<hkallweit1@gmail.com>, <edumazet@google.com>, <pabeni@redhat.com>,
	<UNGLinuxDriver@microchip.com>
References: <20240605101611.18791-1-Raju.Lakkaraju@microchip.com>
 <20240605101611.18791-3-Raju.Lakkaraju@microchip.com>
Content-Language: en-US
From: Wojciech Drewek <wojciech.drewek@intel.com>
In-Reply-To: <20240605101611.18791-3-Raju.Lakkaraju@microchip.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Z0tnVWNBZjVneWx5MTJHOEVLeVZxdktNaWVKbTZRZDl0VTZJaXF1OFllN0JG?=
 =?utf-8?B?VndzR0RFVytqTXdKRTJqbWZIWWI4a1B0UjRZcTZPS2VHYjVnMXdVMW1KU2Iw?=
 =?utf-8?B?cVZWcEpkZThaMGRvckVTMW9FS2I5RHhma050aE44ZnRNNzBwVFRubVViWFNY?=
 =?utf-8?B?aG81VG4xcHpsR0NCdm9ZV0MxZWlXdkJERTlxd2VJVVJjM1l0Tk1BWHBMS2xF?=
 =?utf-8?B?V3lhajZxSUpvcVRIWFpvL1hibVR5UG11VXJoeU4zaHJmcEJSeUlXMVVncXlW?=
 =?utf-8?B?T3YrMnJuUWtLUVNUTzZCNlZFUlBxZmllbWY1VndwRjZySHdZenN3SmdrL2Ru?=
 =?utf-8?B?c2xMbXJuY1ljWlE3bm90K3FIa25qa1J0YUVUWFF1UjVOSDB3YzJNWGxRNERW?=
 =?utf-8?B?WVk1STVaWHQxK1VuTDUraTFXUGNpUXJmVkw3OXhPck8rY2ZXRzFJbk56UHhq?=
 =?utf-8?B?YkQwYUpiR2JKaXRWcHJtSzlSRUtXcXB5Z3Q1T0piZFlJNXk2UGNxK1lqdGFY?=
 =?utf-8?B?ajQ3NDg3MFJocXFpSGpxamlPbTcrSnI0aElpY3gwdHB4U3hrWlR2UjNnMFJU?=
 =?utf-8?B?bHl3NW40b0dwbmJqU2R6QkpZRHFyTnVxT2FxclBlRFdHRGdiNnY1NzJsdGI5?=
 =?utf-8?B?TWFTSkdCdnpUVGhkRU1NMmw0THRvRWhzZFlQQjVCYno1ajJvZHZVd1cxKzBY?=
 =?utf-8?B?MFFtUUk2U3AzODVkSmpEZTVFcituL2JHaUg4TEFycGk2NmxNb1VHeXhmODhY?=
 =?utf-8?B?MVRwdDZWZUJROTc2RTZDUFkrZzkyOGEydllUa0RNT0lNT3lTMTNVK1U1Qkkx?=
 =?utf-8?B?a3liREpHTXdQV25wYVBHTk4wNURjc3VmWnhHUkxRZGF1TFFaRnU2VFdhZDln?=
 =?utf-8?B?ZFpEM2lseTIvdlpYM2daTkFKelNTL21YN1V4aVFtQTRHQlB1bnJ0YlZvM3hQ?=
 =?utf-8?B?ZlBFaFJLOW1QTVlIL2pFcW9sbGIvcDB4eno0b0ZCWWc2dWFjRis3WWNVc3NY?=
 =?utf-8?B?TDBzRnFuOFBUZHpndndFSVB3OHZNWXJqckpCS2F0d2RqQ2orUWR1NlB4bFhI?=
 =?utf-8?B?R0xlVmZpVHVGZVdjQU5PTCtEdjRzUS92UFExaWh0T0ZOZTNSTTNuOUVUTys3?=
 =?utf-8?B?L2V6eHV3VzhzcVNtRmVPcE16cmRONStOWDBHSXFxTlFRVXJ5YjJjV3FGSE0r?=
 =?utf-8?B?VlZrSUZVMUZaQ2tNZk1aWjYvb21jL1hBN0hUNUwxSWhkY1VyUVY1TjZkbHZ4?=
 =?utf-8?B?cW1lN0VhZ3RnaDJJT1V0WWU2SHlhYjRzZkhhUHFPRE1qcmJuRE1BUGt1MlZY?=
 =?utf-8?B?VHlmYXpHOTNQT1ZMYm1sNmhWdnVITGZFdzJQQ2tNOFJCa3lUWUluM0NTMUFP?=
 =?utf-8?B?QXhSTmJoZnhVd1FZTEFpeHJ0N0ZhSnJwd09Rd3lWUWVvc3E3YWtZL2doZkJo?=
 =?utf-8?B?TWFwSVQ1dFpLeW1yYk51cldwQVVsOEYxbzJEcTRabGgzNFlFRDM3eFgrN0JU?=
 =?utf-8?B?RVN4cnFWakVBNkZ1NDlXNjlHRDVESC81U2tlYUFXbDJZU0xITEFGaWY1NmVv?=
 =?utf-8?B?bGNIVTFsYXRHOUlGRVBtQmREOWZVZmpwWUcxRUJRQ0Jtd000R0xiMUpPSnhN?=
 =?utf-8?B?Q25MZjdIWGEvK2E0WjlqUngrSUJXcHNhUnB0NTkyaWJBTHp0cFVPai9Ma3g1?=
 =?utf-8?B?Q2p5TGRYSEZKaS9ST0pLc0RYdWZWZnhVaE5sbTBRZ2NLVW1nZ2hsS09xS2ha?=
 =?utf-8?B?dWNxNk1xeXFpcmRnbXllNDVZeG1WYUN5bS9wUlVJZjM1R1hJcUlFeHZ5ckFX?=
 =?utf-8?B?cmsyVGVZWm1LRWk5UEE5SUJVVmR2M2xKRTF1Z3hLLzhZcmZadjliMjVCMGxl?=
 =?utf-8?B?aEYvSndkUXVjRkx5VEpFeHA2TzRMbGNqRXZkNzhER2lqYTlZWDFEdENEcGx0?=
 =?utf-8?B?WldMZWlaZ0E1MTNTaVZzcDZwbXZLcStyQXZKM1pmZGt6djVZS3N6TkhJWFMz?=
 =?utf-8?B?TVQzd1NQZDUyK2VnamJDMVY0Zms0ZlkvRkxxbGtNK1RmWXJiSGxDK3RkazA2?=
 =?utf-8?B?Mys4Wi9MdGJmYkVoMzZMNXVxblZrQS9zdTBRUFZDQlZqa3JMWmlsaVArdk1Q?=
 =?utf-8?B?MUpuNFZtSXFYSktNaEFtL2hZMDdWMVdIM0lPQ0x0MFNIUmdaTHFTZk10blo4?=
 =?utf-8?B?Qnc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 7466ab43-6eba-44b3-bb75-08dc85518e0a
X-MS-Exchange-CrossTenant-AuthSource: MW4PR11MB5776.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Jun 2024 11:20:50.3798
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: GZtcePIcBMyi8lbylXqbxSdIIsaNh7gsGqy4XUrY/Bs24TZWPkatx8F1vjGa4GtcD3nwSqENB5DxFflRK9MMT1TDnYwcvO758vmH1KJtCWU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB7523
X-OriginatorOrg: 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;