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

[Yibo Dong]

On Mon, Oct 05, 2026 at 07:36:08AM +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · 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).
> 
> - [Medium] Only two PF-to-firmware command senders follow the mailbox
>   rule this patch documents.
> 
> --
> 
> net: rnpgbe: Add link status handling support
> 
> This commit adds handling of firmware link events for the rnpgbe driver.
> Link events arrive over the mailbox and a service task updates the
> carrier state. It also sends SET_PHY_UP and LINK_REPORT_EN on open and
> close, and adds a PHY_LINK_SET command at probe that fails probe if the
> firmware does not acknowledge it.
> 
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
> > index 2310f01df1f50..ef00b948ad0f6 100644
> > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
> 
> [ ... ]
> 
> > + * On success the hardware mailbox remains locked. The caller must process
> > + * the event and call mucse_mbx_event_end() to acknowledge it and release
> > + * the hardware mailbox lock.
> > + *
> > + * 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. Therefore, failure to acquire the lock indicates
> > + * a mailbox fault rather than transient contention.
> 
> [ ... ]
> 
> > 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)
> > +{
> 
> [ ... ]
> 
> > +	len = le16_to_cpu(req.r.datalen);
> > +	mutex_lock(&hw->mbx.lock);
> > +	err = mucse_write_and_wait_ack_mbx(hw, req.dwords, len);
> > +	mutex_unlock(&hw->mbx.lock);
> 
> [Severity: Medium]
> Could a pending LINK_CHANGE_EVT stop the firmware from picking up this
> command, or the POWER_UP command?
> 
> The kernel-doc of mucse_mbx_event_begin() says firmware does not touch
> the shared window until the PF acknowledges the event. Only
> mucse_mbx_phyup() and mucse_mbx_link_report() send through
> mucse_write_mbx_coalesce_event(), which calls mucse_ack_pending_event()
> first.
> 
> mucse_mbx_set_link() and mucse_mbx_powerup() go through
> mucse_write_and_wait_ack_mbx()->mucse_write_mbx_pf(). That path writes
> FWPF_SHM and bumps pf_req without checking FW2PF_CNT for an event that
> has not been acknowledged.
> 
> At teardown, rnpgbe_down() only warns if LINK_REPORT_EN(0) fails:
> 
>     err = rnpgbe_send_notify(hw, false, mucse_fw_link_report_en);
>     if (err) {
>         ...
>         dev_warn(&hw->pdev->dev, "Fw will still report link event\n");
>     }
> 
> The following SET_PHY_UP(0) drops the link, so firmware can post
> LINK_CHANGE_EVT. Then rnpgbe_rm_adapter() and rnpgbe_dev_shutdown() do:
> 
>     rnpgbe_free_mbx_irq(mucse);
>     rnpgbe_notify_powerdown(mucse);
> 
> rnpgbe_free_mbx_irq() frees the IRQ and cancels mbx_work. Nothing
> acknowledges the event, and POWER_UP(0) goes out on the
> non-coalescing path.
> 
> At probe, the driver never turns link reporting off. Suppose reporting
> was left enabled by a previous instance, for example after a crash or
> kexec with the interface up, and RESET_HW does not clear it. The new
> PHY_LINK_SET restarts autonegotiation, so an event could be posted
> while set_link or get_macaddr is in flight. Also, mucse_mbx_reset()
> treats an event that is already pending as consumed:
> 
>     val = mbx_data_rd32(mbx, MUCSE_MBX_FW2PF_CNT);
>     hw->mbx.fw_req = FIELD_GET(GENMASK_U32(15, 0), val);
> 
> If the firmware rule holds, the PF would wait out the mailbox ack
> timeout. At remove or shutdown that gives "Send powerdown to hw
> failed". At probe, the error from rnpgbe_setup_default_link() now
> fails the probe.
> 
> Should every PF-to-firmware sender acknowledge a pending event first,
> the way mucse_write_mbx_pf_coalesce_event() does? Alternatively,
> should a pending event be drained before these commands are sent?
> 
Same feedback with v14.
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() (which is called even before
mailbox irq is registered) is called.

> > +
> > +	return err;
> > +}
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/B438B0A83ACCA2DE%2B20261004065604.224873-1-dong100%40mucse.com
>