Re: [PATCH net-next v9 4/4] net: rnpgbe: Add link status handling support

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260814111317.1741087-2-dong100@mucse.com?part=4
---
--- Patch [4]: [PATCH net-next v9 4/4] net: rnpgbe: Add link status handling support ---
commit b6af113d772d4dd7e20d76a3f509b4e2063a20db
Author: Dong Yibo <dong100@mucse.com>

net: rnpgbe: Add link status handling support

This adds firmware-driven link status handling to the rnpgbe driver: link
state fields in struct mucse_hw, LINK_CHANGE_EVT parsing over the mailbox,
and a 500 ms delayed service task that flips the carrier.  It also notifies
firmware of port up/down, makes rnpgbe_up_complete() return an error so
ndo_open can fail, and enables the GMAC receiver in receive-all mode.

A few questions below.

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> index 291e77d573fe6..55b4abed82bd7 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c

[ ... ]

> @@ -149,3 +155,32 @@ int rnpgbe_init_hw(struct mucse_hw *hw, int board_type)
>  
>  	return 0;
>  }
> +
> +/**
> + * rnpgbe_set_link - Set the hardware link state
> + * @hw: hw information structure
> + * @linkup: link on or not
> + *
> + * rnpgbe_set_link setup link status
> + *
> + **/
> +void rnpgbe_set_link(struct mucse_hw *hw, bool linkup)
> +{
> +	u32 value = mucse_hw_rd32(hw, GMAC_CONTROL);
> +
> +	if (linkup)
> +		value |= GMAC_CONTROL_RE;
> +	else
> +		value &= ~GMAC_CONTROL_RE;
> +
> +	mucse_hw_wr32(hw, GMAC_CONTROL, value);

[Severity: Low]
[Finding: main-7-40]
[Sources: opus-5]
The enable path sets GMAC_CONTROL_RE here and only programs
GMAC_FRAME_FILTER afterwards, while the disable path does the two writes in
the opposite order.

Between these two writes the receiver is running with the frame filter
value left behind by the previous link-down path:

	mucse_hw_wr32(hw, GMAC_FRAME_FILTER, 0);

which is exact-address matching, and no GMAC unicast address register is
ever programmed by this driver.

Are frames arriving in that window matched against reset address
registers?  Would writing the filter before enabling the receiver be
more consistent with the disable path?

> +
> +	/* Keep the GMAC in receive-all mode while the link is up. The
> +	 * chip-level filter does the actual address filtering, but there
> +	 * is no ndo_set_rx_mode yet to configure it.
> +	 */
> +	if (linkup)
> +		mucse_hw_wr32(hw, GMAC_FRAME_FILTER, GMAC_RX_ALL);
> +	else
> +		mucse_hw_wr32(hw, GMAC_FRAME_FILTER, 0);
> +}

[Severity: Medium]
[Finding: main-1-0]
[Sources: opus-5, old-prompts, opus-4-7]
GMAC_RX_ALL is defined as BIT(31) | BIT(0), which on this DesignWare-style
GMAC is receive-all plus promiscuous.  Every link-up therefore leaves the
MAC accepting every frame on the wire for the lifetime of the link, and
dev->promiscuity / IFF_PROMISC are never updated, so userspace still sees a
non-promiscuous interface.

The comment and the commit message both say the filtering happens
elsewhere:

    Enable the GMAC receiver in receive-all mode because packet filtering
    is done by the chip-level filter

A grep over drivers/net/ethernet/mucse/ shows the GMAC_CONTROL and
GMAC_FRAME_FILTER writes in rnpgbe_set_link() are the only MAC or filter
register writes in the whole driver, and rnpgbe_netdev_ops has no
ndo_set_rx_mode:

static const struct net_device_ops rnpgbe_netdev_ops = {
	.ndo_open       = rnpgbe_open,
	.ndo_stop       = rnpgbe_close,
	.ndo_start_xmit = rnpgbe_xmit_frame,
	.ndo_get_stats64 = rnpgbe_get_stats64,
};

Which code programs that chip-level filter?  As it stands, foreign unicast
and all multicast frames are DMA'd into the rx rings and discarded in
software, and local taps observe other hosts' traffic while ip link
reports the interface as non-promiscuous.

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> index 1d87edfba3d75..9e640e8d67036 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> @@ -21,8 +21,27 @@
>  #define RNPGBE_DMA_TX_STATUS           GENMASK_U32(23, 22)
>  #define RNPGBE_DMA_RX_STATUS           GENMASK_U32(21, 20)
>  #define RX_AXI_RW_EN                   0x03
> +/* RNPGBE_LINK_ST is a driver-owned link-state snapshot consumed by firmware.
> + * M_DEFAULT_ST replaces the complete snapshot, causing firmware
> + * to report the current link state again when it differs from this default.
> + */
> +#define M_ST_MASK                      (GENMASK_U32(31, 24) | \
> +					GENMASK_U32(11, 8) | BIT(6) | \
> +					BIT(4) | BIT(0))
> +/* Set the driver-state marker; all other driver status fields start clear. */
> +#define M_DEFAULT_ST                   0xa0000000

[Severity: Medium]
[Finding: main-1-1]
[Sources: opus-5, old-prompts]
Who owns the bits of RNPGBE_LINK_ST outside M_ST_MASK, that is bits 23:12,
7, 5 and 3:1?

mucse_update_link_status_reg() treats them as state that must be preserved:

	value = mucse_hw_rd32(hw, RNPGBE_LINK_ST);
	value &= ~M_ST_MASK;
	value |= M_DEFAULT_ST;

while rnpgbe_up_complete() and the invalid-magic path in
mucse_mbx_fw_req_handler() blind-write the whole word:

	mucse_hw_wr32(hw, RNPGBE_LINK_ST, M_DEFAULT_ST);

so exactly the bits the read-modify-write takes care to keep are zeroed.
Note the register sits at 0x000c, between RNPGBE_DMA_STATUS and
RNPGBE_DMA_AXI_EN.

If the whole word is driver-owned, is the masked read-modify-write reading
anything meaningful?  If it is not, do the two full-word writes destroy
state the driver cannot restore?

> +/* Driver-reported fields: 25:24 pause, 11:8 speed, 6 LLDP, 4 duplex,
> + * and 0 link up/down. M_DEFAULT_ST resets these fields so firmware reports
> + * the current hardware state again.
> + */
> +#define RNPGBE_LINK_ST                 0x000c
>  #define RNPGBE_DMA_AXI_EN              0x0010
>  #define RNPGBE_TX_MIN_PKT_LEN          33
>  
> +#define MUCSE_GMAC_OFF(_n)             (0x20000 + (_n))
> +#define GMAC_CONTROL_RE                0x00000004
> +#define GMAC_CONTROL                   MUCSE_GMAC_OFF(0)
> +#define GMAC_RX_ALL                    (BIT(31) | BIT(0))
> +#define GMAC_FRAME_FILTER              MUCSE_GMAC_OFF(0x4)
>  #define RNPGBE_MAX_QUEUES 8
>  #endif /* _RNPGBE_HW_H */
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> index 9199a48e75d42..fa5ea4dd7a1c6 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c

[ ... ]

> @@ -1414,11 +1447,50 @@ void rnpgbe_up_complete(struct mucse *mucse)
>  		rnpgbe_configure_msi(mucse);
>  
>  	rnpgbe_napi_enable_all(mucse);
> +	/* Clear stale link state and tell firmware the driver hasn't
> +	 * seen any link yet. Firmware will re-assert LINK_CHANGE_EVT
> +	 * when link_report_en is sent below since RNPGBE_LINK_ST
> +	 * no longer matches the actual (possibly up) link state.
> +	 */
> +	spin_lock_irqsave(&mucse->link_lock, flags);
> +	WRITE_ONCE(hw->link, false);
> +	WRITE_ONCE(hw->speed, 0);
> +	WRITE_ONCE(hw->duplex, 0);
> +	atomic_set(&mucse->link_pending, 0);
> +	/* echo fw driver now in default state */
> +	mucse_hw_wr32(hw, RNPGBE_LINK_ST, M_DEFAULT_ST);

[ ... ]

> +	queue_delayed_work(system_percpu_wq, &mucse->serv_task,
> +			   msecs_to_jiffies(500));
> +
> +	return 0;
>  }

[ ... ]

> +/**
> + * rnpgbe_service_task - Manages and runs subtasks
> + * @work: pointer to work_struct containing our data
> + **/
> +void rnpgbe_service_task(struct work_struct *work)
> +{
> +	struct mucse *mucse = container_of(work, struct mucse, serv_task.work);
> +
> +	if (test_bit(__MUCSE_DOWN, &mucse->state))
> +		return;
> +
> +	rnpgbe_process_link_subtask(mucse);
> +
> +	if (!test_bit(__MUCSE_DOWN, &mucse->state))
> +		queue_delayed_work(system_percpu_wq, &mucse->serv_task,
> +				   msecs_to_jiffies(500));
> +}

[Severity: Low]
[Finding: main-1-4]
[Sources: opus-5, old-prompts]
Could the carrier update be done directly from the mailbox work item
instead of being polled?

The link event is already parsed in sleepable process context:

rnpgbe_msix_other()
  queue_work(system_percpu_wq, &mucse->mbx_work)
    rnpgbe_mbx_work()
      mucse_fw_irq_handler()
        mucse_mbx_fw_req_handler()
          atomic_set_release(&mucse->link_pending, 1);

but the handler only sets link_pending, and this delayed work polls the
flag every 500 ms.  That adds up to 500 ms of carrier-update latency per
event, including the first link report since rnpgbe_up_complete() arms the
task with msecs_to_jiffies(500), plus two workqueue wakeups per second for
as long as the interface is up.

Queueing serv_task with zero delay from mucse_mbx_fw_req_handler() would
avoid both.

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
> index 754016f6b4f00..441724f8603eb 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
> @@ -3,6 +3,7 @@
>  
>  #include <linux/if_ether.h>
>  #include <linux/bitfield.h>
> +#include <linux/ethtool.h>
>  
>  #include "rnpgbe.h"
>  #include "rnpgbe_mbx.h"
> @@ -27,10 +28,14 @@ static int mucse_fw_send_cmd_wait_resp(struct mucse_hw *hw,
>  	int retry_cnt = 3;
>  	int err;
>  
> +	BUILD_BUG_ON(sizeof(struct mbx_fw_cmd_reply) != 56);
> +
>  	mutex_lock(&hw->mbx.lock);
> +
>  	err = mucse_write_and_wait_ack_mbx(hw, req->dwords, len);
>  	if (err)
>  		goto out;
> +

[Severity: Low]
[Finding: main-1-3]
[Sources: opus-5, old-prompts]
These changes are not described by the commit message and are unrelated to
link handling: the BUILD_BUG_ON plus blank-line churn inside the
pre-existing mucse_fw_send_cmd_wait_resp(), and a whitespace-only hunk in
rnpgbe_add_adapter().

The new #include <linux/ethtool.h> also has no user in this file: grep
finds no ethtool symbol here, DUPLEX_BIT is driver-local, and the speeds
are plain literals.

The new FLAGS_REPLY and FLAGS_ERR macros and the reply arm of
union mbx_fw_cmd_u have no consumer either; mucse_fw_handle_event()
demultiplexes on the opcode field alone and never checks the
reply/error discriminator.  Could these be dropped or moved to the patch
that uses them?

>  	do {
>  		err = mucse_poll_and_read_mbx(hw, reply->dwords,
>  					      sizeof(reply->r));

[ ... ]

> +static void mucse_update_link_status_reg(struct mucse_hw *hw,
> +					 struct mbx_fw_cmd_req *req)
> +{
> +	u16 status = le16_to_cpu(req->link_stat.st.status);
> +	u16 speed = le16_to_cpu(req->link_stat.st.speed);
> +	u32 value;
> +
> +	value = mucse_hw_rd32(hw, RNPGBE_LINK_ST);
> +	value &= ~M_ST_MASK;
> +	value |= M_DEFAULT_ST;
> +
> +	if (le16_to_cpu(req->link_stat.port_status)) {
> +		value |= BIT(0);
> +		switch (speed) {
> +		case 10:
> +			value |= (mucse_speed_10 << 8);
> +			break;
> +		case 100:
> +			value |= (mucse_speed_100 << 8);
> +			break;
> +		case 1000:
> +			value |= (mucse_speed_1000 << 8);
> +			break;
> +		default:
> +			break;
> +		}

[Severity: Medium]
[Finding: gpt-5-6-sol-1-6]
[Sources: gpt-5-6-sol, opus-5, old-prompts]
What happens for a speed that is not 10, 100 or 1000?

The default arm leaves bits 11:8 at 0, and mucse_speed_10 is also 0:

enum mucse_speed {
	mucse_speed_10 = 0,
	mucse_speed_100 = 1,
	mucse_speed_1000 = 2,
};

so an unsupported speed is encoded as 10 Mbps in the snapshot, while
mucse_mbx_fw_req_handler() caches the raw value:

	WRITE_ONCE(hw->speed, le16_to_cpu(req->link_stat.st.speed));

and rnpgbe_link_is_up() prints it and enables carrier plus the GMAC
receiver.

The commit message says:

    Validate firmware link events before updating the cached link state.

but the only check applied is port_magic against ST_VALID_MAGIC; speed is
not range checked.  Should an out-of-range speed be rejected instead, given
that the snapshot then reports a state the hardware never has and firmware
can keep re-asserting LINK_CHANGE_EVT?

> +
> +		value |= FIELD_PREP(BIT(4),
> +				    !!(req->link_stat.st.flags & DUPLEX_BIT));
> +		value |= FIELD_PREP(GENMASK_U32(25, 24),
> +				    status & GENMASK(1, 0));
> +	} else {
> +		value &= ~BIT(0);
> +	}
> +
> +	if (status & ST_STATUS_LLDP_STATUS_MASK)
> +		value |= BIT(6);
> +	else
> +		value &= ~BIT(6);
> +
> +	mucse_hw_wr32(hw, RNPGBE_LINK_ST, value);
> +}

[ ... ]

> +static void mucse_mbx_fw_req_handler(struct mucse_hw *hw,
> +				     struct mbx_fw_cmd_req *req)
> +{
> +	struct mucse *mucse = container_of(hw, struct mucse, hw);
> +	u32 magic = le32_to_cpu(req->link_stat.port_magic);
> +	unsigned long flags;
> +
> +	if (le16_to_cpu(req->opcode) == LINK_CHANGE_EVT) {
> +		spin_lock_irqsave(&mucse->link_lock, flags);
> +		if (magic != ST_VALID_MAGIC) {
> +			/* Do not let an invalid event change the cached link state.
> +			 * Reset the driver snapshot so firmware reports it again.
> +			 */
> +			mucse_hw_wr32(hw, RNPGBE_LINK_ST, M_DEFAULT_ST);
> +			spin_unlock_irqrestore(&mucse->link_lock, flags);
> +			return;
> +		}

[Severity: Medium]
[Finding: gpt-5-6-sol-3-22]
[Sources: gpt-5-6-sol]
Can this path leave the carrier reported as up after the link is gone?

If an invalid-magic event arrives while the link is going down, the cached
state is kept and the snapshot is reset to M_DEFAULT_ST.  But a legitimate
link-down snapshot with LLDP clear resolves to exactly M_DEFAULT_ST too:
mucse_update_link_status_reg() clears BIT(0), encodes no speed, duplex or
pause, and clears bit 6.

After the reset the snapshot already matches the real down state, so per
the handshake documented in this patch firmware has no mismatch left to
re-report from.  link_pending is never set, rnpgbe_process_link_subtask()
never calls rnpgbe_link_is_down(), and the interface keeps carrier on with
GMAC_CONTROL_RE set until it is cycled by hand.

> +
> +		if (test_bit(__MUCSE_DOWN, &mucse->state)) {
> +			mucse_update_link_status_reg(hw, req);
> +			spin_unlock_irqrestore(&mucse->link_lock, flags);
> +			return;
> +		}
> +
> +		if (le16_to_cpu(req->link_stat.port_status))
> +			WRITE_ONCE(hw->link, true);
> +		else
> +			WRITE_ONCE(hw->link, false);

[Severity: Medium]
[Finding: main-7-37]
[Sources: opus-5, old-prompts]
Should port_status be tested with BIT(hw->port) rather than for plain
non-zero?

Every other port field in this mailbox protocol is a per-port bitmask.  The
requests added by this patch encode:

	.port_mask = cpu_to_le32(BIT(hw->port)),

and the GET_MAC_ADDRESS reply is decoded in mucse_mbx_get_macaddr() as:

	if (le32_to_cpu(reply.r.mac_addr.ports) & BIT(port))

If port_status is likewise a bitmap of ports whose link is up, which its
pairing with changed_lanes suggests, then on multi-port parts such as
RNPGBE_DEVICE_ID_N500_QUAD_PORT a link-up on another port would make this
port declare carrier up, set GMAC_CONTROL_RE plus receive-all filtering,
and advertise link up in the firmware snapshot while its own PHY is down.
The same unmasked test is used in mucse_update_link_status_reg().

> +
> +		WRITE_ONCE(hw->speed, le16_to_cpu(req->link_stat.st.speed));
> +		WRITE_ONCE(hw->duplex, req->link_stat.st.flags & DUPLEX_BIT);
> +		/* update regs to notify link info is received */
> +		mucse_update_link_status_reg(hw, req);
> +		atomic_set_release(&mucse->link_pending, 1);
> +		spin_unlock_irqrestore(&mucse->link_lock, flags);
> +	}
> +}
> +
> +/**
> + * mucse_fw_handle_event - Handle one pending firmware event
> + * @hw: pointer to the hardware structure
> + *
> + * Return: true if an event was handled, false otherwise
> + **/
> +static bool mucse_fw_handle_event(struct mucse_hw *hw)
> +{
> +	union mbx_fw_cmd_u msg = {};
> +	int err;
> +
> +	/* try to check and read fw req */
> +	mutex_lock(&hw->mbx.lock);
> +	err = mucse_check_and_read_mbx(hw, msg.dwords, sizeof(msg));
> +	mutex_unlock(&hw->mbx.lock);
> +	if (err)
> +		return false;
> +
> +	mucse_mbx_fw_req_handler(hw, &msg.req);
> +
> +	return true;
> +}
> +
> +/**
> + * mucse_fw_irq_handler - Drain pending firmware mailbox events
>   * @hw: pointer to the HW structure
> + *
> + * The only asynchronous event currently handled is LINK_CHANGE_EVT.
> + * Firmware rate-limits link-change notifications to a minimum interval
> + * of 500 ms.
>   **/
>  void mucse_fw_irq_handler(struct mucse_hw *hw)
>  {
> +	/* Drain events coalesced while the mailbox work item was pending. */
> +	while (mucse_fw_handle_event(hw))
> +		continue;
>  }

[Severity: Medium]
[Finding: gpt-5-6-sol-6-34]
[Sources: gpt-5-6-sol, opus-5, old-prompts]
Is this loop bounded by anything other than firmware behaviour?

The exit condition is entirely device controlled.  mucse_check_for_msg_pf()
succeeds whenever the firmware-written counter differs from the cached
copy:

	if (fw_req == 0 || fw_req == hw->mbx.fw_req)
		return -EIO;

and mucse_read_mbx_pf() re-caches it from the same register, so as long as
firmware keeps advancing the counter the work item never returns.
mucse_fw_handle_event() also returns true for any successful read,
including messages with an unrecognised opcode.

That blocks the teardown path, which waits for this work:

rnpgbe_free_mbx_irq()
	free_irq(pci_irq_vector(pdev, 0), mucse);
	cancel_work_sync(&mucse->mbx_work);

so a device that keeps posting messages would hang rmmod or PCI remove and
keep one workqueue worker spinning on the mailbox lock.  The reply path
bounds itself with retry_cnt = 3; should this drain get a similar budget?
The 500 ms firmware rate limit is only stated in the comment, not enforced
by the driver.

Simon says: I believe that the issue below is a false-positive as
            the use of __MUCSE_DOWN ensures correct behaviour.

[Severity: Low]
[Finding: gpt-5-6-sol-5-30]
[Sources: gpt-5-6-sol, opus-5]
Now that mucse_fw_irq_handler() is no longer empty, can it run against an
already disabled device on the shutdown path?

rnpgbe_dev_shutdown() quiesces only the netdev and then disables the
device:

	rtnl_lock();
	netif_device_detach(netdev);
	if (netif_running(netdev))
		rnpgbe_close(netdev);
	rtnl_unlock();
	pci_disable_device(pdev);

The mailbox IRQ stays registered and mbx_work is never cancelled there;
rnpgbe_down() only cancels serv_task.  A queued or running mbx_work would
then do mailbox MMIO reads and the RNPGBE_LINK_ST write after
pci_disable_device(), and feed whatever the reads return into
mucse_mbx_fw_req_handler().  rnpgbe_free_mbx_irq() already has the
free_irq() plus cancel_work_sync() ordering the shutdown path is missing.

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h
> index a6bf5de55aa39..0e6972b7c3c02 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h

[ ... ]

>  struct mbx_fw_cmd_reply {
> +#define FLAGS_REPLY       BIT(0)
> +#define FLAGS_ERR         BIT(2)
>  	__le16 flags;
>  	__le16 opcode;
>  	__le16 error_code;
> @@ -94,10 +127,27 @@ union mbx_fw_cmd_reply_u {
>  	__le32 dwords[sizeof(struct mbx_fw_cmd_reply) / sizeof(__le32)];
>  };
>  
> +union mbx_fw_cmd_u {
> +	struct mbx_fw_cmd_req req;
> +	struct mbx_fw_cmd_reply reply;
> +	__le32 dwords[sizeof(struct mbx_fw_cmd_reply) / sizeof(__le32)];
> +};
> +
> +/* Firmware encoding for RNPGBE_LINK_ST[11:8]. Valid link events report
> + * only these three speeds.
> + */
> +enum mucse_speed {
> +	mucse_speed_10 = 0,
> +	mucse_speed_100 = 1,
> +	mucse_speed_1000 = 2,
> +};

[ ... ]