Re: [PATCH net-next 7/7] bnge: Process async events while administratively down

[netdev-bot+sashiko@kernel.org]

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

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

- [High] bnge_sp_task(): replacing the early return with `goto async_evt`
  lets the entire link/PHY event block execute while BNGE_STATE_OPEN is…
- [High] Processing PHY events while administratively down inadvertently
  turns the hardware link back on.
- [Medium] The new `if (!netif_running(bd->netdev)) return;` in
  bnge_report_link() (bnge_link.c:405) silently drops a report that its…
- [Medium] bnge_close() ends with the plain, non-atomic store
  `bn->sp_event = 0;` (bnge_netdev.c:3307) while NQ0's NAPI and IRQ are…
- [Medium] A speed/PHY-configuration async event processed while the
  interface is closed silently overwrites the link and pause settings…

Pre-existing issues:
- [Medium] bnge_free_nq0() (bnge_netdev.c:3523-3583) sends HWRM_RING_FREE
  for NQ0 and invalidates ring->fw_ring_id before it disables the NAPI…

--

bnge: Process async events while administratively down

bnge_sp_task() now jumps over the periodic-stats block with a goto instead
of returning early when BNGE_STATE_OPEN is clear, so async link/PHY events
are handled while the interface is down.  bnge_report_link() gains an early
return when the netdev is not running.

A few questions about the new behaviour below.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_link.c b/drivers/net/ethernet/broadcom/bnge/bnge_link.c
> index 3b0dfcf273762..31e2ede2e537a 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_link.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_link.c
> @@ -402,6 +402,9 @@ static char *bnge_report_fec(struct bnge_link_info *link_info)
>  
>  void bnge_report_link(struct bnge_dev *bd)
>  {
> +	if (!netif_running(bd->netdev))
> +		return;
> +

[Severity: Medium]
Can this early return make a link transition disappear entirely instead of
just deferring the message?

The only caller is edge-triggered.  bnge_update_link() commits the new
state and reports only when it changed:

	if (chng_link_state) {
		if (link_info->phy_link_status == BNGE_LINK_LINK)
			link_info->link_state = BNGE_LINK_STATE_UP;
		else
			link_info->link_state = BNGE_LINK_STATE_DOWN;
		if (link_state != link_info->link_state)
			bnge_report_link(bd);

bnge_hwrm_shutdown_link() relies on that, leaving a sentinel behind on
close:

	if (!rc) {
		/* Device is not obliged to link down in certain scenarios,
		 * even when forced. Setting the state unknown is consistent
		 * with driver startup and will force link state to be
		 * reported during subsequent open based on PORT_PHY_QCFG.
		 */
		bd->link_info.link_state = BNGE_LINK_STATE_UNKNOWN;
	}

With this patch, an async link event handled while down calls
bnge_update_link(bn, true), which replaces BNGE_LINK_STATE_UNKNOWN with
UP/DOWN and consumes the transition, while bnge_report_link() returns
immediately.  At the next open, bnge_open_core() ->
bnge_update_phy_setting() -> bnge_update_link() reads the same value, sees
no change, and never calls bnge_report_link().

Does that permanently lose the "NIC Link is Up, N Mbps ..." / "NIC Link is
Down" message and the FEC line for that transition, so whether link state
gets reported at all depends on whether an async event happened to arrive
while the port was down?  Carrier itself still gets fixed up by
bnge_tx_enable() via "if (BNGE_LINK_IS_UP(bn->bd)) netif_carrier_on()",
which means carrier and the log output are now driven by two different
paths.

>  	if (BNGE_LINK_IS_UP(bd)) {
>  		const char *signal = "";
>  		const char *flow_ctrl;
> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> index d11c404acd4e2..49e27cbc63946 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> @@ -394,10 +394,8 @@ static void bnge_sp_task(struct work_struct *work)
>  	struct bnge_dev *bd = bn->bd;
>  
>  	netdev_lock(bn->netdev);
> -	if (!test_bit(BNGE_STATE_OPEN, &bd->state)) {
> -		netdev_unlock(bn->netdev);
> -		return;
> -	}
> +	if (!test_bit(BNGE_STATE_OPEN, &bd->state))
> +		goto async_evt;

[Severity: High]
Is the block below the async_evt label read-only with respect to the PHY?

For a speed-change event, bnge_sp_task() first calls
bnge_hwrm_phy_qcaps(bd), which refreshes the supported-speed mask:

	if (resp->supported_speeds2_auto_mode)
		link_info->support_auto_speeds2 =
			le16_to_cpu(resp->supported_speeds2_auto_mode);

and then calls bnge_update_link(bn, true), whose tail is:

	support_changed = bnge_support_speed_dropped(bn);
	if (support_changed && (bn->eth_link_info.autoneg & BNGE_AUTONEG_SPEED))
		rc = bnge_hwrm_set_link_setting(bn, true);

bnge_support_speed_dropped() returns true whenever the advertised mask
contains speeds that are no longer in support_auto_speeds2, which is what
happens when a transceiver is swapped for one supporting fewer speeds while
the port is administratively down.

bnge_hwrm_set_link_setting() -> bnge_hwrm_set_link_common() then builds an
HWRM_PORT_PHY_CFG with:

		req->flags |= cpu_to_le32(BNGE_PHY_FLAGS_RESTART_AUTO);
	...
	/* tell FW that the setting takes effect immediately */
	req->flags |= cpu_to_le32(PORT_PHY_CFG_REQ_FLAGS_RESET_PHY);

Does this undo the PORT_PHY_CFG_REQ_FLAGS_FORCE_LINK_DWN that bnge_close()
issued through bnge_hwrm_shutdown_link()?  Nothing re-asserts the force-down
afterwards, and since bnge_report_link() now returns early while not
running, the carrier stays off, so the link partner would see an active link
on a closed interface.

BNGE_PHY_CFG_ABLE(bd) is link_info.phy_enabled, a firmware capability set in
bnge_hwrm_phy_qcaps() and not cleared on close, so it does not block this.
Every other PHY-programming path is gated on netif_running() -
bnge_set_link_ksettings() and bnge_set_pauseparam() both do
"if (netif_running(dev))" before touching hardware.  Would it be better to
narrow the BNGE_STATE_OPEN gate to the read-only link-info refresh rather
than remove it for the whole block?

[Severity: Medium]
Related question about the same fall-through: further down, sp_task ends
with

	if (speed_chng || cfg_chng)
		bnge_init_ethtool_link_settings(bn);

bnge_init_ethtool_link_settings() rewrites elink_info->autoneg,
->advertising, ->req_duplex and ->req_flow_ctrl from the current hardware
auto_mode / duplex_setting / pause settings.

While the interface is down, bn->eth_link_info is the pending userspace
request: bnge_set_link_ksettings() and bnge_set_pauseparam() store into it
and only program hardware "if (netif_running(dev))", and
bnge_update_phy_setting() applies it at the next open.

If an async speed/config event arrives after "ethtool -s" or "ethtool -A"
was issued on a down interface, does this overwrite the queued request with
the hardware-derived values, so the next open sees nothing to change and the
user's setting is silently dropped?

[Severity: Medium]
One more on whether the events the fall-through wants actually survive
close.  bnge_close() ends with a plain store:

	bnge_close_core(bn);
	bnge_hwrm_shutdown_link(bn->bd);
	bnge_hwrm_if_change(bn->bd, false);
	bn->sp_event = 0;

At that point NQ0 is live again: bnge_close_core() -> bnge_free_core() ends
with bnge_resume_nq0(), which re-enables NQ0 NAPI and re-arms the doorbell,
and bnge_free_irq() skips the NQ0 vector.  bnge_hwrm_shutdown_link() forces
the port down, which itself generates a LINK_STATUS_CHANGE async event, and
the NAPI side records it with atomic bitops:

static void bnge_queue_sp_work(struct bnge_net *bn, unsigned int event)
{
	set_bit(event, &bn->sp_event);
	__bnge_queue_sp_work(bn);
}

Two questions here.  Is the plain word store racing with set_bit() from
softirq context on the same word, and can it clobber a bit set
concurrently?

And since bnge is ops-locked and bnge_sp_task() takes netdev_lock, the
queued worker cannot run until bnge_close() returns, i.e. after the wipe.
Does that mean the event the new async_evt path is meant to handle is
discarded before it can be observed, with nothing re-polling while down
because the periodic timer is stopped?  Would clearing only the bits that
are meaningless while down, using atomic bitops, be more consistent with
the goal of this patch?

>  
>  	if (test_and_clear_bit(BNGE_PERIODIC_STATS_SP_EVENT, &bn->sp_event)) {
>  		bnge_hwrm_port_qstats(bd, 0);
> @@ -405,6 +403,7 @@ static void bnge_sp_task(struct work_struct *work)
>  		bnge_accumulate_all_stats(bd);
>  	}
>  
> +async_evt:
>  	if (test_and_clear_bit(BNGE_UPDATE_PHY_SP_EVENT, &bn->sp_event)) {
>  		int rc;
>  

[Severity: High]
The first block that the new label falls into is the PHY retry, and
that one does not just read state - it programs the PHY.
bnge_sp_task() calls bnge_update_phy_setting(bn), whose tail is:

	if (!BNGE_LINK_IS_UP(bd))
		update_link = true;

	if (update_link)
		rc = bnge_hwrm_set_link_setting(bn, update_pause);

So the "link is currently down" case is precisely the case that sends
an HWRM_PORT_PHY_CFG to bring the link back up.  Running that while
BNGE_STATE_OPEN is clear looks like it undoes the
PORT_PHY_CFG_REQ_FLAGS_FORCE_LINK_DWN that bnge_close() issued via
bnge_hwrm_shutdown_link(), and nothing on this path re-asserts it
afterwards.  Combined with the other half of this patch -
bnge_report_link() returning early when !netif_running() - the carrier
stays off while the port is actually being brought up, so the link
partner sees an active link on an administratively down interface and
the driver logs nothing about it.

How does the submitter expect BNGE_UPDATE_PHY_SP_EVENT to be reachable
while down?  bd->link_info.phy_retry is only cleared on success here,
and BNGE_PHY_CFG_ABLE(bd)/link_info.phy_enabled is a firmware
capability that close does not clear, so a retry queued before the
state bit went away (for example a partially failed open, or a reset
that clears BNGE_STATE_OPEN without wiping bn->sp_event) would be
serviced here.  If that is possible at all, this block needs to stay
gated: every other PHY-programming path in the driver checks
netif_running() first (bnge_set_link_ksettings(),
bnge_set_pauseparam()).  Would it be cleaner to keep the
BNGE_STATE_OPEN check for the PHY retry and only let the read-only
link-info refresh run while down?

[Severity: Medium]
This isn't a bug introduced by this patch, but while looking at the NQ0
persistence machinery this patch builds on: in bnge_free_nq0(), the
firmware ring is freed before the poller is disabled.

	bnge_db_nq(bn, &nqr->nq_db, nqr->nq_raw_cons);
	synchronize_irq(irq->vector);

	hwrm_ring_free_send_msg(bn, ring,
				RING_FREE_REQ_RING_TYPE_NQ,
				INVALID_HW_RING_ID);
	ring->fw_ring_id = INVALID_HW_RING_ID;

and only later, inside the irq->requested block:

		free_irq(irq->vector, bnapi);
		irq->requested = 0;

		netdev_lock(bn->netdev);
		napi_disable_locked(&bnapi->napi);

synchronize_irq() waits for an in-flight handler but not for a NAPI
instance that handler already scheduled, so can bnge_napi_poll() still
write BNGE_DB_NQ()/BNGE_DB_NQ_ARM() using the doorbell key of a ring
firmware has already released?  The ring memory itself is freed after
napi_disable_locked() and synchronize_net(), so this looks limited to
doorbell writes against a freed firmware ring id on the removal and probe
error paths.  Would disabling and draining NAPI before sending
HWRM_RING_FREE be the right ordering?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901191728.643212-1-bhargava.marreddy%40broadcom.com