[RESEND PATCH net-next] net: wangxun: refactor NCSI and WOL capability checks

[RESEND PATCH net-next] net: wangxun: refactor NCSI and WOL capability checks

[Jiawen Wu]

There is a clean refactoring that replaces inline bitwise checks with
precomputed boolean fields. The hardware capability of NCSI and WOL is
obtained from the sub-system ID and do not change.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ethernet/wangxun/libwx/wx_hw.c      | 3 +--
 drivers/net/ethernet/wangxun/txgbe/txgbe_main.c | 8 ++++++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
index 59cdd7d71e44..6fbfbb77646c 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
@@ -1313,8 +1313,7 @@ void wx_disable_rx(struct wx *wx)
 		rxctrl &= ~WX_RDB_PB_CTL_RXEN;
 		wr32(wx, WX_RDB_PB_CTL, rxctrl);
 
-		if (!(((wx->subsystem_device_id & WX_NCSI_MASK) == WX_NCSI_SUP) ||
-		      ((wx->subsystem_device_id & WX_WOL_MASK) == WX_WOL_SUP))) {
+		if (!(wx->ncsi_enabled || wx->wol_hw_supported)) {
 			/* disable mac receiver */
 			wr32m(wx, WX_MAC_RX_CFG,
 			      WX_MAC_RX_CFG_RE, 0);
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
index eb91c4f28ecd..8c508aa3ce62 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
@@ -267,8 +267,7 @@ static void txgbe_disable_device(struct wx *wx)
 		wx_set_all_vfs(wx);
 	}
 
-	if (!(((wx->subsystem_device_id & WX_NCSI_MASK) == WX_NCSI_SUP) ||
-	      ((wx->subsystem_device_id & WX_WOL_MASK) == WX_WOL_SUP))) {
+	if (!(wx->ncsi_enabled || wx->wol_hw_supported)) {
 		/* disable mac transmiter */
 		wr32m(wx, WX_MAC_TX_CFG, WX_MAC_TX_CFG_TE, 0);
 	}
@@ -336,6 +335,8 @@ static void txgbe_down_suspend(struct wx *wx)
  **/
 static void txgbe_init_type_code(struct wx *wx)
 {
+	u16 ncsi_mask = wx->subsystem_device_id & WX_NCSI_MASK;
+	u16 wol_mask = wx->subsystem_device_id & WX_WOL_MASK;
 	u8 device_type = wx->subsystem_device_id & 0xF0;
 
 	switch (wx->device_id) {
@@ -381,6 +382,9 @@ static void txgbe_init_type_code(struct wx *wx)
 		wx->media_type = wx_media_unknown;
 		break;
 	}
+
+	wx->wol_hw_supported = ((wol_mask) == WX_WOL_SUP);
+	wx->ncsi_enabled = ((ncsi_mask) == WX_NCSI_SUP);
 }
 
 /**
-- 
2.51.0

Re: [RESEND PATCH net-next] net: wangxun: refactor NCSI and WOL capability checks

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] Non-equivalent refactor: the shared libwx helper
  wx_disable_rx() (drivers/net/ethernet/wangxun/libwx/wx_hw.c:1316) now…
- [Low] Style/readability nit in the new code in txgbe_init_type_code()
  (drivers/net/ethernet/wangxun/txgbe/txgbe_main.c:386-387): the…

--

net: wangxun: refactor NCSI and WOL capability checks

This replaces the inline subsystem-ID bit tests for NCSI and WOL support in
wx_disable_rx() and txgbe_disable_device() with the precomputed booleans
wx->ncsi_enabled and wx->wol_hw_supported, and adds the assignment of those
two fields to txgbe_init_type_code().

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> index 122c4952d203f..f0046dcb54962 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> @@ -1313,8 +1313,7 @@ void wx_disable_rx(struct wx *wx)
>  		rxctrl &= ~WX_RDB_PB_CTL_RXEN;
>  		wr32(wx, WX_RDB_PB_CTL, rxctrl);
>  
> -		if (!(((wx->subsystem_device_id & WX_NCSI_MASK) == WX_NCSI_SUP) ||
> -		      ((wx->subsystem_device_id & WX_WOL_MASK) == WX_WOL_SUP))) {
> +		if (!(wx->ncsi_enabled || wx->wol_hw_supported)) {
>  			/* disable mac receiver */
>  			wr32m(wx, WX_MAC_RX_CFG,
>  			      WX_MAC_RX_CFG_RE, 0);

[Severity: Medium]
wx_disable_rx() is shared libwx code, so ngbe reaches this too.  Is the new
condition still equivalent for ngbe?

ngbe_init_type_code() sets ncsi_enabled with an extra OCP term:

drivers/net/ethernet/wangxun/ngbe/ngbe_main.c:
	type_mask = (u16)(wx->subsystem_device_id & NGBE_OEM_MASK);
	ncsi_mask = wx->subsystem_device_id & WX_NCSI_MASK;
	...
	wx->ncsi_enabled = (ncsi_mask == WX_NCSI_SUP ||
			   type_mask == NGBE_SUBID_OCP_CARD) ? 1 : 0;

NGBE_OEM_MASK is 0x00FF and NGBE_SUBID_OCP_CARD is 0x0040, while
WX_NCSI_MASK is 0x8000 and WX_WOL_MASK is 0x4000, so the OCP term is
independent of the two bits the old expression tested.

For an ngbe adapter with (subsystem_device_id & 0x00FF) == 0x0040 and bits
15 and 14 clear, the pre-patch expression evaluated false and the driver
cleared WX_MAC_RX_CFG_RE; after this patch ncsi_enabled is true and the
write is skipped, so WX_RDB_PB_CTL_RXEN is cleared but the MAC receiver
stays enabled.  That affects the ngbe down/reset paths that call
wx_disable_rx(): ngbe_disable_device()->wx_disable_rx(),
ngbe_reset_hw()->wx_stop_adapter()->wx_disable_rx(), and
wx_configure_rx()->wx_disable_rx().  wx_enable_rx() still sets
WX_MAC_RX_CFG_RE, so on those SKUs the enable would no longer have a
matching disable.

Also, at the baseline wx->ncsi_enabled had no readers anywhere in the
wangxun tree, so this patch gives it its first reader inside code ngbe uses,
without the patch touching ngbe.

Is the OCP term intended to apply to wx_disable_rx() as well?  If not, would
a separate strict capability field (for example ncsi_hw_supported) used by
wx_disable_rx() keep the behaviour unchanged?

The commit message says:

    There is a clean refactoring that replaces inline bitwise checks with
    precomputed boolean fields. The hardware capability of NCSI and WOL is
    obtained from the sub-system ID and do not change.

Given the ngbe OCP term, is "do not change" accurate for all wangxun
devices, and could the message spell out the ngbe behaviour change?

> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> index eb91c4f28ecd4..8c508aa3ce62e 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c

[ ... ]

> @@ -336,6 +335,8 @@ static void txgbe_down_suspend(struct wx *wx)
>   **/
>  static void txgbe_init_type_code(struct wx *wx)
>  {
> +	u16 ncsi_mask = wx->subsystem_device_id & WX_NCSI_MASK;
> +	u16 wol_mask = wx->subsystem_device_id & WX_WOL_MASK;
>  	u8 device_type = wx->subsystem_device_id & 0xF0;
>  
>  	switch (wx->device_id) {

[ ... ]

> @@ -381,6 +382,9 @@ static void txgbe_init_type_code(struct wx *wx)
>  		wx->media_type = wx_media_unknown;
>  		break;
>  	}
> +
> +	wx->wol_hw_supported = ((wol_mask) == WX_WOL_SUP);
> +	wx->ncsi_enabled = ((ncsi_mask) == WX_NCSI_SUP);
>  }

[Severity: Low]
This isn't a bug, but the inner parentheses around the plain locals wol_mask
and ncsi_mask in txgbe_init_type_code() are redundant:

	wx->wol_hw_supported = (wol_mask == WX_WOL_SUP);
	wx->ncsi_enabled = (ncsi_mask == WX_NCSI_SUP);

The two single-use locals are also declared at the top of
txgbe_init_type_code() but only consumed after both switch statements.
Would assigning the booleans directly from wx->subsystem_device_id at the
point of use, as ngbe_init_type_code() does inline, drop both locals?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/95D34449BA183C54%2B20260901070238.78509-1-jiawenwu%40trustnetic.com