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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch adds a rule for the shared FW<->PF mailbox window:
  before a PF command overwrites MUCSE_MBX_FWPF_SHM, any unacknowledged…

--

net: rnpgbe: Add link status handling support

This commit adds link status handling to the rnpgbe driver. Firmware link
events arrive over the mailbox and are handled by a service task that sets
the carrier state. Firmware is notified on port up/down, and probe now
configures autonegotiation through a new PHY_LINK_SET command.

> 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

[ ... ]

> @@ -320,6 +418,44 @@ static int mucse_write_mbx_pf(struct mucse_hw *hw, const __le32 *msg, u16 size)
>  	return 0;
>  }
>  
> +/**
> + * mucse_write_mbx_pf_coalesce_event - Write around a level-like event
> + * @hw: pointer to the HW structure
> + * @msg: the message buffer
> + * @size: length of buffer
> + *
> + * Return: 0 on success, negative errno on failure
> + **/
> +static int mucse_write_mbx_pf_coalesce_event(struct mucse_hw *hw,
> +					     const __le32 *msg, u16 size)
> +{
> +	const int size_in_words = size / sizeof(__le32);
> +	struct mucse_mbx_info *mbx = &hw->mbx;
> +	int err;
> +
> +	err = mucse_obtain_mbx_lock_pf(hw);
> +	if (err)
> +		return err;
> +
> +	err = mucse_ack_pending_event(hw);
> +	if (err)
> +		goto release;

[ ... ]

> 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)
> +{
> +	union mbx_fw_cmd_req_u req = {

[ ... ]

> +	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: Low]

This patch adds mucse_write_mbx_pf_coalesce_event(). It acknowledges any
unacknowledged firmware event in MUCSE_MBX_FWPF_SHM before a PF command
overwrites the shared window. Only SET_PHY_UP and LINK_REPORT_EN use it.

Should mucse_mbx_set_link() go through mucse_write_mbx_coalesce_event() as
well? Right now it uses
mucse_write_and_wait_ack_mbx()->mucse_write_mbx_pf(), which writes the
window without checking fw_req. mucse_mbx_powerup() and
mucse_fw_send_cmd_wait_resp() use the same plain path.

One way to reach this is remove or shutdown after an earlier mailbox
failure. rnpgbe_down() treats a failed LINK_REPORT_EN(false) as non-fatal:

    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");
    }

It then sends SET_PHY_UP(false), which can trigger a link-down event.

rnpgbe_rm_adapter() and rnpgbe_dev_shutdown() tear down the event consumer
before the power-down notification:

    rnpgbe_free_mbx_irq(mucse);
    rnpgbe_notify_powerdown(mucse);

The cancel_work_sync() in rnpgbe_free_mbx_irq() can drop a queued
mbx_work. An event can also arrive after free_irq().

In both cases rnpgbe_notify_powerdown()->mucse_mbx_powerup(hw, false)
writes POWER_UP(false) over that event with the plain writer, and the event
is never acknowledged.

What happens depends on how firmware handles a PF request that overwrites
an unacknowledged event. Could the power-down command be misparsed, or time
out waiting for an ack, in that case?

The stale event also looks like it carries over to the next driver
instance. mucse_mbx_reset() syncs hw->mbx.fw_req to the current counter
without incrementing pf_ack. On the next ifup, mucse_ack_pending_event()
then sees fw_req == hw->mbx.fw_req and skips the acknowledgment.

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

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/3B42BE25B3696EAE%2B20261013022920.63529-1-dong100%40mucse.com