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

[Yibo Dong]

On Thu, Oct 01, 2026 at 03:39:50AM +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> 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
> 
> [ ... ]
> 
> > 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;
> > +}
I think no need to handle this. rnpgbe_down only warns is because driver
should release memory for tx/rx even with a mailbox error.
After a system crash, the PCIe reset performed during system
initialization causes firmware to clear the driver-loaded, port-up and
link-event-enable states.
During a normal driver probe, mailbox synchronization is performed before
RESET_HW and PHY_LINK_SET. If an unexpected pending event prevents
mailbox synchronization, probe fails rather than continuing with an
ambiguous mailbox state. The driver does not need to distinguish whether
that failure was caused by firmware or by stale mailbox state.
Therefore, after successful synchronization and reset, no asynchronous
event can be pending when mucse_mbx_set_link() is called.
so I plan to keep the current implementation.
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/0A4D45AD9F6A0F14%2B20260928033701.1033196-1-dong100%40mucse.com
>