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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 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] This patch sets a mailbox protocol: a pending firmware event
  holds the shared FW<->PF window until the PF acks it.
- [Low] rnpgbe_down() calls rnpgbe_set_link(hw, false) when no link event
  is being handled.

--

net: rnpgbe: Add link status handling support

This commit adds link status handling to the rnpgbe driver. Firmware link
change events arrive over the mailbox, and a service task updates the
carrier state from them. Firmware is also notified of port up/down and
link report enable/disable, and the default link is set up at probe.

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> index 7beec27991d35..6f7bf4ca5b927 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c

[ ... ]

> @@ -1448,22 +1470,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");
> +	}
> +	/* Disable the receiver after RX DMA is quiesced and firmware
> +	 * has handled port-down, but before releasing the RX buffers.
> +	 */
> +	spin_lock_irqsave(&mucse->link_lock, flags);
> +	rnpgbe_set_link(hw, false);
> +	spin_unlock_irqrestore(&mucse->link_lock, flags);

[Severity: Low]
Is the read-modify-write of GMAC_CONTROL in rnpgbe_set_link() safe
against firmware at this call site?

The kernel-doc for rnpgbe_set_link() in rnpgbe_chip.c gives this reason
for why the RMW is safe:

    Firmware updates the speed and duplex fields before posting a link event
    and waits for the PF acknowledgment, so event handling completes this
    read-modify-write before firmware accesses GMAC_CONTROL again.

That reasoning covers the call from mucse_mbx_fw_req_handler(), which runs
before mucse_mbx_event_end() acks the event.

In rnpgbe_down(), however, LINK_REPORT_EN=0 and SET_PHY_UP(0) have
already been acked, or they failed and only produced a warning. So no
LINK_CHANGE_EVT is outstanding here, and firmware is not waiting for the
PF. link_lock is a driver-only spinlock, so it cannot order the host
against firmware.

Taking the PHY down is the kind of link change on which firmware writes
speed/duplex into GMAC_CONTROL. Could an interleaving like this happen?

driver                            firmware
value = rd32(GMAC_CONTROL) (RE=1)
                                  reads GMAC_CONTROL
wr32(GMAC_CONTROL, RE=0)
                                  writes speed/duplex back (RE=1)

That would leave the receiver enabled after ifdown. In the opposite
order, the driver's stale write-back would drop firmware's speed/duplex
update.

rnpgbe_stop_all_rx_rings() has already stopped RX DMA at this point, and
the next ifup sets RE again through the event handler. So the effect seems
limited to wrong GMAC state while the interface is down. The driver code
alone doesn't show whether firmware writes GMAC_CONTROL after acking
port-down with reporting disabled.

Should the kernel-doc be updated for this call site, or does some other
handshake cover it?

>  	rnpgbe_clean_all_rx_rings(mucse);
>  }

[ ... ]

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
> index e28f3193aa854..683e0ea741136 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c

[ ... ]

> +int mucse_mbx_set_link(struct mucse_hw *hw, u32 advertising, bool autoneg,
> +		       u32 speed, u32 duplex, u32 mdix_ctrl)
> +{

[ ... ]

> +	int len, err;
> +
> +	len = le16_to_cpu(req.r.datalen);
> +	mutex_lock(&hw->mbx.lock);
> +	err = mucse_write_and_wait_ack_mbx(hw, req.dwords, len);

[Severity: Medium]
Should this use mucse_write_mbx_coalesce_event(), as mucse_mbx_phyup()
and mucse_mbx_link_report() do?

The kernel-doc for mucse_mbx_event_begin() in this patch describes the
mailbox protocol:

    Firmware releases the hardware mailbox lock before raising the event
    interrupt and does not access the shared window again until the PF
    acknowledges the event.

The patch also adds mucse_ack_pending_event(), so that a PF command acks
any pending event before it writes the shared window.

PHY_LINK_SET does not do this. It goes through
mucse_write_and_wait_ack_mbx()->mucse_write_mbx_pf(), which writes
MUCSE_MBX_FWPF_SHM directly and never checks for a pending event.

Firmware link reporting can still be enabled at probe. rnpgbe_down() only
warns "Fw will still report link event" when LINK_REPORT_EN=0 fails. A
crashed or kdump'd previous kernel never runs close() at all.

Suppose firmware posts a LINK_CHANGE_EVT after mucse_mbx_reset() has
taken its fw_req snapshot, for example because RESET_HW bounced the link.
Would PHY_LINK_SET then overwrite the unacked event?

Under the protocol above, firmware would not process the command, and
mucse_poll_for_ack() would time out. Probe now treats that as fatal:

rnpgbe_add_adapter()
    err = rnpgbe_setup_default_link(hw);
    if (err) {
        dev_err(&pdev->dev, "Setup link failed %d\n", err);
        goto err_powerdown;
    }

If firmware does ack the command, the stale FW2PF count would seem to make
the following GET_MAC_ADDRESS in mucse_fw_send_cmd_wait_resp() read the
window straight away. It would then recover only through the opcode retry
loop.

The existing mucse_mbx_powerup() uses the same non-coalescing path. It
runs on remove/shutdown after close, where a failed LINK_REPORT_EN=0 could
leave an event pending.

The driver code doesn't show whether firmware keeps link_report_en set
across RESET_HW or a driver reload.

> +	mutex_unlock(&hw->mbx.lock);
> +
> +	return err;
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/0A4D45AD9F6A0F14%2B20260928033701.1033196-1-dong100%40mucse.com