Re: [PATCH net-next v13 5/5] net: rnpgbe: Add link status handling support

[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 · 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] netif_running() is used as the gate for 'the data path is
  configured, it is safe to enable the GMAC receiver and assert carrier'.
- [High] mucse->serv_task (delayed work) is queued from the mailbox work
  item — mucse_mbx_fw_req_handler() calls…
- [Medium] M_DEFAULT_ST (0xa0000000) is simultaneously the 'driver has
  not seen link / make firmware report again' snapshot and the steady-…
- [Medium] The firmware->PF request counter read by mucse_mbx_get_fwreq()
  is only 16 bits (FIELD_GET(GENMASK_U32(15,0), MUCSE_MBX_FW2PF_CNT))…
- [Medium] rnpgbe_set_link() performs a read-modify-write of GMAC_CONTROL
  plus a write of GMAC_FRAME_FILTER, but the two callers use different…

--

net: rnpgbe: Add link status handling support

This adds firmware-driven link handling to rnpgbe: link state is cached in
struct mucse_hw, firmware link events arrive over the mailbox, and a delayed
work item (serv_task) converts them into netif_carrier_on()/off(). The patch
also adds the GMAC receiver enable in rnpgbe_set_link() and makes ndo_open
fail when the firmware port-up or link-report commands do not complete.

A few questions about the new paths below.

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> index 4e2835b71f32..b56263de4a36 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c

[ ... ]

> @@ -224,3 +249,30 @@ void rnpgbe_set_rx_mode(struct net_device *netdev)
>  	mucse_hw_wr32(hw, RNPGBE_RX_MCAST_CTRL, mcast_ctrl);
>  	mucse_hw_wr32(hw, RNPGBE_RX_FILTER_CTRL, filter_ctrl);
>  }
> +
> +/**
> + * rnpgbe_set_link - Set the hardware link state
> + * @hw: hw information structure
> + * @linkup: link on or not
> + *
> + * rnpgbe_set_link setup link status
> + *
> + **/
> +void rnpgbe_set_link(struct mucse_hw *hw, bool linkup)
> +{
> +	u32 value = mucse_hw_rd32(hw, GMAC_CONTROL);
> +
> +	/* The chip-level filter is programmed by ndo_set_rx_mode(). Keep the
> +	 * GMAC in receive-all mode so it does not discard frames accepted by
> +	 * that filter.
> +	 */
> +	if (linkup) {
> +		mucse_hw_wr32(hw, GMAC_FRAME_FILTER, GMAC_RX_ALL);
> +		value |= GMAC_CONTROL_RE;
> +		mucse_hw_wr32(hw, GMAC_CONTROL, value);
> +	} else {
> +		value &= ~GMAC_CONTROL_RE;
> +		mucse_hw_wr32(hw, GMAC_CONTROL, value);
> +		mucse_hw_wr32(hw, GMAC_FRAME_FILTER, 0);
> +	}
> +}

[Severity: Medium]
Should the read-modify-write of GMAC_CONTROL here be serialized?

The two callers disagree on the lockset. mucse_mbx_fw_req_handler() calls
rnpgbe_set_link() with mucse->link_lock held:

	spin_lock_irqsave(&mucse->link_lock, flags);
	...
	rnpgbe_set_link(hw, mucse_link_is_up(hw, req));

while rnpgbe_down() calls it after link_lock has already been dropped:

	spin_unlock_irqrestore(&mucse->link_lock, flags);
	...
	rnpgbe_set_link(hw, false);

So the intersection of locks held for GMAC_CONTROL and GMAC_FRAME_FILTER is
empty. Can the read of GMAC_CONTROL in one path interleave with the write
from the other, and can the mailbox path re-set GMAC_CONTROL_RE plus
GMAC_FRAME_FILTER = GMAC_RX_ALL after rnpgbe_down() has cleared them?

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> index feb84704d602..5659e91f3497 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> @@ -21,6 +21,17 @@
>  #define RNPGBE_DMA_TX_STATUS           GENMASK_U32(23, 22)
>  #define RNPGBE_DMA_RX_STATUS           GENMASK_U32(21, 20)
>  #define RX_AXI_RW_EN                   0x03
> +/* RNPGBE_LINK_ST is a driver-owned link-state snapshot consumed by firmware.
> + * Set the driver-state marker; all other driver status fields start clear.
> + */
> +#define M_DEFAULT_ST                   0xa0000000
> +/* Firmware never reports 0xf in the speed field. */
> +#define M_INVALID_ST                   (M_DEFAULT_ST | GENMASK_U32(11, 8))
> +/* Driver-reported fields: 25:24 pause, 11:8 speed, 6 LLDP, 4 duplex,
> + * and 0 link up/down. M_DEFAULT_ST resets these fields so firmware reports
> + * the current hardware state again.
> + */
> +#define RNPGBE_LINK_ST                 0x000c
>  #define RNPGBE_DMA_AXI_EN              0x0010
>  #define RNPGBE_TX_MIN_PKT_LEN          33

[ ... ]

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> index 9afd10273a62..2a5349f03a02 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c

[ ... ]

> @@ -1399,7 +1407,19 @@ static void rnpgbe_cancel_rx_retry_timers(struct mucse *mucse)
>  void rnpgbe_down(struct mucse *mucse)
>  {
>  	struct net_device *netdev = mucse->netdev;
> +	struct mucse_hw *hw = &mucse->hw;
> +	unsigned long flags;
> +	int err;
>  
> +	spin_lock_irqsave(&mucse->link_lock, flags);
> +	WRITE_ONCE(hw->link, false);
> +	WRITE_ONCE(hw->speed, 0);
> +	WRITE_ONCE(hw->duplex, 0);
> +	atomic_set(&mucse->link_pending, 0);
> +	spin_unlock_irqrestore(&mucse->link_lock, flags);
> +	cancel_delayed_work_sync(&mucse->serv_task);

[Severity: High]
This is the only cancel_delayed_work_sync(&mucse->serv_task) in the driver.
Is that enough to keep the work item from outliving the struct mucse?

The producer is mucse_mbx_fw_req_handler(), running from mbx_work, gated
only on netif_running():

	atomic_set_release(&mucse->link_pending, 1);
	mod_delayed_work(system_percpu_wq, &mucse->serv_task, 0);

In net/core/dev.c, __dev_open() sets __LINK_STATE_START before calling
ndo_open and clears it only if ndo_open returns an error, so netif_running()
is already true on entry to rnpgbe_open() and stays true across the new
err_down unwind. Can a mailbox event serviced right after the cancel above
re-arm serv_task? Note firmware link reporting is only disabled later in
rnpgbe_down(), and that failure is merely warned about ("Fw will still
report link event").

If serv_task is left queued and the interface was never successfully
opened, removal does not appear to cancel it:

rnpgbe_rm_adapter()
    unregister_netdev()      /* no ndo_stop, device not IFF_UP */
    rnpgbe_free_mbx_irq()    /* cancels mbx_work only */
    rnpgbe_clear_interrupt_scheme()
    free_netdev(netdev)      /* frees the mucse holding serv_task */

and rnpgbe_service_task() then dereferences mucse and mucse->netdev from
that freed allocation. rnpgbe_dev_shutdown() has no serv_task cancellation
either. Should the remove and shutdown paths call
cancel_delayed_work_sync(&mucse->serv_task) before the netdev is freed?

> +
> +	netif_carrier_off(netdev);
>  	rnpgbe_irq_disable(mucse);
>  	rnpgbe_napi_disable_all(mucse);
>  	synchronize_net();
> @@ -1408,22 +1428,77 @@ void rnpgbe_down(struct mucse *mucse)
>  	rnpgbe_stop_all_rx_rings(mucse);
>  	rnpgbe_cancel_rx_retry_timers(mucse);
>  	rnpgbe_clean_all_tx_rings(mucse);
> +
> +	err = rnpgbe_send_notify(hw, false, mucse_fw_link_report_en);
> +	if (err) {
> +		dev_warn(&hw->pdev->dev, "Send link report to hw failed %d\n",
> +			 err);
> +		dev_warn(&hw->pdev->dev, "Fw will still report link event\n");
> +	}
> +
> +	err = rnpgbe_send_notify(hw, false, mucse_fw_portup);
> +	if (err) {
> +		dev_warn(&hw->pdev->dev, "Send port down to hw failed %d\n",
> +			 err);
> +		dev_warn(&hw->pdev->dev, "Port is not truly down\n");
> +	}
> +	rnpgbe_set_link(hw, false);
>  	rnpgbe_clean_all_rx_rings(mucse);
>  }

[ ... ]

> +	err = rnpgbe_send_notify(hw, true, mucse_fw_portup);
> +	if (err) {
> +		dev_err(&hw->pdev->dev,
> +			"Failed to notify firmware that port is up: %d\n", err);
> +		return err;
> +	}
> +	/* Firmware checks RNPGBE_LINK_ST (driver's last-received link state)
> +	 * and only asserts LINK_CHANGE_EVT when it differs from the actual link
> +	 * state AND link_report_en is true.
> +	 */
> +	err = rnpgbe_send_notify(hw, true, mucse_fw_link_report_en);
> +	if (err) {
> +		dev_err(&hw->pdev->dev,
> +			"Failed to enable firmware link reporting: %d\n",
> +			 err);
> +		return err;
> +	}
> +
> +	return 0;
>  }

[ ... ]

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> index 35eb858eb2c1..9ba53bb3629b 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c

[ ... ]

> @@ -86,9 +87,16 @@ static int rnpgbe_open(struct net_device *netdev)
>  	err = rnpgbe_configure(mucse);
>  	if (err)
>  		goto err_free_rx;
> -	rnpgbe_up_complete(mucse);
> +	err = rnpgbe_up_complete(mucse);
> +	if (err)
> +		goto err_down;
>  
>  	return 0;
> +err_down:
> +	rnpgbe_down(mucse);
> +	rnpgbe_free_all_rx_resources(mucse);
> +	rnpgbe_free_all_tx_resources(mucse);
> +	goto err_free_irqs;
>  err_free_rx:
>  	rnpgbe_free_all_rx_resources(mucse);
>  err_free_tx:

[ ... ]

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
> index 2310f01df1f5..74f42be975c2 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
> @@ -197,7 +197,8 @@ static int mucse_read_mbx_pf(struct mucse_hw *hw, __le32 *msg, u16 size)
>   * mucse_check_for_msg_pf - Check to see if the fw has sent mail
>   * @hw: pointer to the HW structure
>   *
> - * Return: 0 if the fw has set the Status bit or else -EIO
> + * Return: 0 if firmware has posted a new message, -ENOMSG if there is no
> + * new message, or -EIO if the mailbox has been reset
>   **/
>  static int mucse_check_for_msg_pf(struct mucse_hw *hw)
>  {
> @@ -205,12 +206,11 @@ static int mucse_check_for_msg_pf(struct mucse_hw *hw)
>  	u16 fw_req;
>  
>  	fw_req = mucse_mbx_get_fwreq(mbx);
> -	/* chip's register is reset to 0 when rc send reset
> -	 * mbx command. Return -EIO if in this state, others
> -	 * fw == hw->mbx.fw_req means no new msg.
> -	 **/
> -	if (fw_req == 0 || fw_req == hw->mbx.fw_req)
> +	/* The request counter is reset to zero by a mailbox reset. */
> +	if (!fw_req)
>  		return -EIO;
> +	if (fw_req == hw->mbx.fw_req)
> +		return -ENOMSG;
>  
>  	return 0;
>  }

[Severity: Medium]
Is zero really reserved for a mailbox reset?

mucse_mbx_get_fwreq() returns a 16-bit field:

	u32 val = mbx_data_rd32(mbx, MUCSE_MBX_FW2PF_CNT);

	return FIELD_GET(GENMASK_U32(15, 0), val);

and the corresponding producer side, mucse_mbx_inc_pf_req(), just does a
plain u16 increment with nothing skipping zero. So zero looks like a normal
counter value after 0xffff.

On the new asynchronous path, what recovers from that? mucse_fw_handle_event()
returns on any error without calling mucse_mbx_event_end():

	err = mucse_mbx_event_begin(hw, msg.dwords, sizeof(msg));
	if (err) {
		mutex_unlock(&hw->mbx.lock);
		if (err != -ENOMSG)
			dev_warn_ratelimited(...);
		return;
	}

so the event is never acknowledged and hw->mbx.fw_req stays stale. Per the
contract documented in mucse_mbx_event_begin(), firmware does not touch the
shared window again until the PF acknowledges, so does the counter ever get
past zero?

The new mucse_ack_pending_event() has the same test:

	fw_req = mucse_mbx_get_fwreq(mbx);
	if (fw_req == hw->mbx.fw_req)
		return 0;
	if (!fw_req)
		return -EIO;

which would make every SET_PHY_UP and LINK_REPORT_EN command fail, and with
the new error returns in rnpgbe_up_complete() that turns into a persistent
failure of ndo_open. RESET_HW is only issued from probe.

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
> index e28f3193aa85..6415b6430efc 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c

[ ... ]

> +static void mucse_update_link_status_reg(struct mucse_hw *hw,
> +					 struct mbx_fw_cmd_req *req)
> +{
> +	u16 status = le16_to_cpu(req->link_stat.st.status);
> +	u16 speed = le16_to_cpu(req->link_stat.st.speed);
> +	u32 value;
> +
> +	value = M_DEFAULT_ST;
> +
> +	if (mucse_link_is_up(hw, req)) {

[ ... ]

> +	} else {
> +		value &= ~BIT(0);
> +	}

[ ... ]

> +static void mucse_mbx_fw_req_handler(struct mucse_hw *hw,
> +				     struct mbx_fw_cmd_req *req)
> +{
> +	struct mucse *mucse = container_of(hw, struct mucse, hw);
> +	u32 magic = le32_to_cpu(req->link_stat.port_magic);
> +	unsigned long flags;
> +
> +	if (le16_to_cpu(req->opcode) == LINK_CHANGE_EVT) {
> +		u16 speed = le16_to_cpu(req->link_stat.st.speed);
> +
> +		spin_lock_irqsave(&mucse->link_lock, flags);
> +		if (magic != ST_VALID_MAGIC) {
> +			/* Do not change the cached state for an invalid event.
> +			 * Use an invalid speed encoding to make firmware report
> +			 * again.
> +			 */
> +			mucse_hw_wr32(hw, RNPGBE_LINK_ST, M_INVALID_ST);
> +			spin_unlock_irqrestore(&mucse->link_lock, flags);
> +			return;
> +		}
> +
> +		if (mucse_link_is_up(hw, req) &&
> +		    !mucse_link_speed_valid(req)) {
> +			/* Do not acknowledge an invalid speed as valid.
> +			 * Keep the snapshot mismatched so firmware retries it.
> +			 * Firmware limits link reports to one per 500 ms.
> +			 */
> +			mucse_hw_wr32(hw, RNPGBE_LINK_ST, M_DEFAULT_ST);
> +			spin_unlock_irqrestore(&mucse->link_lock, flags);
> +			dev_warn_ratelimited(&hw->pdev->dev,
> +					     "unsupported link speed %u Mbps\n",
> +					     speed);
> +			return;
> +		}

[Severity: Medium]
Can M_DEFAULT_ST here be distinguished by firmware from an acknowledged
link-down snapshot?

In mucse_update_link_status_reg() the link-down branch is:

	value = M_DEFAULT_ST;
	...
	} else {
		value &= ~BIT(0);
	}

Bit 0 is already clear in M_DEFAULT_ST, so that mask is a no-op and a
link-down snapshot with LLDP clear is byte-identical to M_DEFAULT_ST. The
speed field also collides, since mucse_speed_10 is 0 in enum mucse_speed.

Given the comment added in rnpgbe_hw.h that M_DEFAULT_ST "resets these
fields so firmware reports the current hardware state again", what happens
on this path if the cable is pulled before a valid retry arrives? The
exported snapshot already reads as link down, which matches the hardware, so
firmware can stop reporting, and rnpgbe_process_link_event() only consumes
pending events:

	if (!atomic_xchg(&mucse->link_pending, 0))
		return false;

so carrier would stay up with no way to refresh it. Under the other reading
of the register, every acknowledged link-down snapshot mismatches reality
and firmware re-reports at its 500 ms rate for as long as the link is down.

Note this path also leaves the previously cached link-up state untouched and
does not set link_pending, while the caller mucse_fw_handle_event() goes on
to acknowledge the event via mucse_mbx_event_end(). Would M_INVALID_ST,
as used by the invalid-magic branch just above, be the right value here?

> +
> +		if (!netif_running(mucse->netdev)) {
> +			rnpgbe_set_link(hw, false);
> +			mucse_update_link_status_reg(hw, req);
> +			spin_unlock_irqrestore(&mucse->link_lock, flags);
> +			return;
> +		}
> +
> +		if (mucse_link_is_up(hw, req))
> +			WRITE_ONCE(hw->link, true);
> +		else
> +			WRITE_ONCE(hw->link, false);
> +
> +		WRITE_ONCE(hw->speed, le16_to_cpu(req->link_stat.st.speed));
> +		WRITE_ONCE(hw->duplex, req->link_stat.st.flags & DUPLEX_BIT);
> +		rnpgbe_set_link(hw, mucse_link_is_up(hw, req));

[Severity: High]
Does netif_running() mean the receive data path is configured here?

__dev_open() in net/core/dev.c sets __LINK_STATE_START before it calls
ndo_open, and clears it only if ndo_open failed, so netif_running() is true
from the first statement of rnpgbe_open() until __dev_open() returns. Can a
LINK_CHANGE_EVT delivered in that window take the link-up branch above and
have rnpgbe_set_link(hw, true) write GMAC_FRAME_FILTER = GMAC_RX_ALL and set
GMAC_CONTROL_RE before rnpgbe_configure_rx() has programmed the ring bases,
RNPGBE_DMA_AXI_EN |= RX_AXI_RW_EN and RNPGBE_RX_START? rnpgbe_service_task()
uses the same gate and would call netif_carrier_on() for an interface whose
open has not completed.

The teardown side looks similar. rnpgbe_down() does not cancel or flush
mbx_work (that only happens in rnpgbe_free_mbx_irq()), so can an event
processed after rnpgbe_down()'s final rnpgbe_set_link(hw, false) re-enable
the receiver in receive-all mode with the rx rings stopped and drained? On
the new err_down path in rnpgbe_open() the rings are freed right after
rnpgbe_down() while netif_running() is still true:

rnpgbe_up_complete()   /* LINK_REPORT_EN written, ack times out */
    return err
rnpgbe_open()
    goto err_down
        rnpgbe_down()
        rnpgbe_free_all_rx_resources()

with firmware link reporting enabled at that point.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/B8AF0FDE24FAFB51%2B20260920092424.345309-1-dong100%40mucse.com