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

[netdev-bot+sashiko]

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?

> +
> +	return err;
> +}

[ ... ]

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