Re: [PATCH net v3 3/3] net: txgbe: keep MAC enabled for AML devices that support NCSI

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [High] This patch has no effect on any txgbe device.

--

net: txgbe: keep MAC enabled for AML devices that support NCSI

This patch makes txgbe_mac_link_down_aml() leave MAC Tx and Rx enabled
when wx->ncsi_enabled is set. In that case it also stops txgbe_down()
from disabling the TX laser on AML 10G/25G devices. The goal is to keep
BMC pass-through working after the host interface goes down.

> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
> index 26d0cfc58ee2c..9f1ce3e2e3a4d 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
> @@ -458,8 +458,10 @@ static void txgbe_mac_link_down_aml(struct phylink_config *config,
>  {
>  	struct wx *wx = phylink_to_wx(config);
>  
> -	wr32m(wx, TXGBE_AML_MAC_TX_CFG, TXGBE_AML_MAC_TX_CFG_TE, 0);
> -	wr32m(wx, WX_MAC_RX_CFG, WX_MAC_RX_CFG_RE, 0);
> +	if (!wx->ncsi_enabled) {
> +		wr32m(wx, TXGBE_AML_MAC_TX_CFG, TXGBE_AML_MAC_TX_CFG_TE, 0);
> +		wr32m(wx, WX_MAC_RX_CFG, WX_MAC_RX_CFG_RE, 0);
> +	}

[Severity: High]
Is wx->ncsi_enabled ever set for txgbe devices?

Under drivers/net/ethernet/wangxun/, the only assignment seems to be in
ngbe:

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;
    ...
}

txgbe_sw_init(), txgbe_init_type_code() and wx_sw_init() don't appear to
set it. struct wx lives in the netdev private area from
devm_alloc_etherdev_mqs(), which is zeroed, so the field would stay
false.

If that's right, on AML NCSI boards this path still clears
TXGBE_AML_MAC_TX_CFG_TE and WX_MAC_RX_CFG_RE:

txgbe_close()
  txgbe_down()
    phylink_stop()
      txgbe_mac_link_down_aml()

The new check in txgbe_down() would also still assert TX_DISABLE:

	case wx_mac_aml:
		phylink_stop(wx->phylink);
		if (!wx->ncsi_enabled) {
			/* Disable TX laser */
			wr32m(wx, WX_GPIO_DR, TXGBE_GPIOBIT_1, TXGBE_GPIOBIT_1);
		}

Wouldn't BMC pass-through still break on "ip link set dev X down", just
as it did before this patch?

An earlier commit in this series, "net: libwx: keep NGBE OCP cards MAC
enabled", seems to say the same thing: "'wx->ncsi_enabled' ... is not
assigned in TXGBE, so we use an explicit subsystem device ID mask in libwx
now".

Earlier on the same down path, txgbe_disable_device() checks the
subsystem ID directly:

	if (!(((wx->subsystem_device_id & WX_NCSI_MASK) == WX_NCSI_SUP) ||
	      ((wx->subsystem_device_id & WX_WOL_MASK) == WX_WOL_SUP))) {
		/* disable mac transmiter */
		wr32m(wx, WX_MAC_TX_CFG, WX_MAC_TX_CFG_TE, 0);
	}

So txgbe_disable_device() leaves MAC Tx on for NCSI SKUs, and then
txgbe_mac_link_down_aml() turns it off. That check also covers
WX_WOL_SUP. A test on ncsi_enabled alone would skip WX_WOL_SUP even if
the field were set.

Should txgbe_sw_init() set wx->ncsi_enabled from
subsystem_device_id & WX_NCSI_MASK, the way ngbe does? Or should these
checks use the subsystem ID test directly?

The SP-side patch in this series, "net: txgbe: keep MAC enabled for SP
devices that support NCSI", appears to rely on the same field in
txgbe_phy.c.

[ ... ]

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