Re: [PATCH net-next v3] net: wangxun: use cached NCSI and WOL capability flags

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch is submitted as a mechanical cleanup ("use cached
  NCSI and WOL capability flags") but carries a real, user-visible…

Pre-existing issues:
- [Low] Semantic overload of wx->wol_hw_supported: two libwx consumers
  now read the same field with different meanings.

--

net: wangxun: use cached NCSI and WOL capability flags

This replaces the open-coded subsystem device ID tests in wx_disable_rx()
and txgbe_disable_device() with the cached wx->ncsi_enabled and
wx->wol_hw_supported flags, and initializes both flags in
txgbe_init_type_code().  For ngbe this changes behaviour, because
ngbe_init_type_code() also sets ncsi_enabled for OCP cards.

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> index 59cdd7d71e446..6fbfbb77646c0 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]
Should the ngbe behaviour change here be a separate patch with a Fixes:
tag, rather than part of a patch whose subject describes a cleanup?

For txgbe the conversion looks bit-for-bit equivalent: WX_NCSI_SUP ==
WX_NCSI_MASK == 0x8000 and WX_WOL_SUP == WX_WOL_MASK == 0x4000 in
libwx/wx_type.h, and txgbe_init_type_code() runs from txgbe_sw_init()
before the first txgbe_reset_hw() and before any
txgbe_disable_device()/wx_disable_rx() call.

The whole functional delta comes from ngbe, where ncsi_enabled covers an
extra card type:

drivers/net/ethernet/wangxun/ngbe/ngbe_main.c:ngbe_init_type_code() {
	...
	wx->ncsi_enabled = (ncsi_mask == WX_NCSI_SUP ||
			   type_mask == NGBE_SUBID_OCP_CARD) ? 1 : 0;
	...
}

So on an OCP card with the 0x8000 NCSI bit clear, WX_MAC_RX_CFG_RE is no
longer cleared on the paths reaching wx_disable_rx():

  ngbe_close() -> ngbe_down() -> ngbe_disable_device() -> wx_disable_rx()
  ngbe_reset_hw() -> wx_stop_adapter() -> wx_disable_rx()
  wx_configure_rx() -> wx_disable_rx()

That is a user-visible fix for dropped out-of-band management traffic.
Could it be split out and carry a Fixes: tag identifying the introducing
commit, with the stable exclusion expressed separately (for example
Cc: stable+noautosel) instead of by omitting the tag?  The commit message
says:

	No Fixes: tag is added on purpose. The broken behaviour goes back to
	the initial ngbe support in v6.2, and wx_disable_rx() has since been
	moved into libwx and reworked several times, so this does not
	cherry-pick into stable kernels in any useful form.

Without the tag, does the provenance of the fix become hard to recover,
and does bundling it with the refactor make it hard to revert or bisect
the two changes independently?

> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> index eb91c4f28ecd4..0ecbd1c0e5532 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);
>  	}

[Severity: Low]
This is a pre-existing issue and not introduced by this patch, but is
wol_hw_supported now being read with two different meanings in libwx?

In wx_get_wol()/wx_set_wol() (libwx/wx_ethtool.c) it is strictly a
hardware capability gate, with the armed state kept in wx->wol:

	if (!wx->wol_hw_supported)
		return -EOPNOTSUPP;

	wx->wol = 0;
	if (wol->wolopts & WAKE_MAGIC)
		wx->wol = WX_PSR_WKUP_CTL_MAG;

The new tests in wx_disable_rx() and txgbe_disable_device() read the same
capability flag as "the MAC has to stay alive", so after
"ethtool -s ethX wol d" leaves wx->wol == 0, the MAC receiver (and the
txgbe transmitter) is still kept enabled on any WoL-capable board.  This
matches the removed open-coded test exactly, since WX_WOL_SUP ==
WX_WOL_MASK == 0x4000, so nothing changes at runtime today.

Would testing wx->ncsi_enabled || wx->wol, or introducing a flag named
for the actual decision (keep the MAC alive for out-of-band management),
make the shared-code contract explicit?  Otherwise, narrowing
wol_hw_supported later, or wiring the WoL ethtool ops into txgbe, would
silently change whether the MAC stays enabled.

> @@ -381,6 +380,9 @@ static void txgbe_init_type_code(struct wx *wx)
>  		wx->media_type = wx_media_unknown;
>  		break;
>  	}
> +
> +	wx->wol_hw_supported = ((wx->subsystem_device_id & WX_WOL_MASK) == WX_WOL_SUP);
> +	wx->ncsi_enabled = ((wx->subsystem_device_id & WX_NCSI_MASK) == WX_NCSI_SUP);
>  }
>  
>  /**

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/E5215E3514E295BA%2B20260916063312.203457-1-jiawenwu%40trustnetic.com