Re: [PATCH net-next 2/2] net: mctp: Add Axiado AX3005 IRoT MCTP transport driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] net: mctp: irot_ndo_stop() cancelling tx_work wedges link after late TX ACK
- [High] net: mctp: unlocked netif_stop_queue() in irot_ndo_open() races with irot_tx_wake()
- [High] net: mctp: irot_rx_work() ACKs IROT_RX_INVALID without updating last_rx_counter
- [Medium] net: mctp: axiado: premature retry exhaustion in irot_discover_buffers()
--

commit c82310d54b9aff23038939986150dc23c6fe72a1
Author: Akhila Kavi <akavi@axiado.com>

net: mctp: Add Axiado AX3005 IRoT MCTP transport driver

This commit adds an MCTP transport driver for the IRoT co-processor on
Axiado AX3005 SoCs, carrying packets through shared-memory windows
negotiated and signaled over a mailbox interface.

> diff --git a/drivers/net/mctp/axiado-mctp-irot.c b/drivers/net/mctp/axiado-mctp-irot.c
> new file mode 100644
> index 0000000000000..d393a8102f2db
> --- /dev/null
> +++ b/drivers/net/mctp/axiado-mctp-irot.c
> @@ -0,0 +1,1089 @@
[ ... ]
> +/* cc_mctp_buffer: the peer's windows. Write-once; zero mtu means no answer. */
> +static void irot_cb_buffer_rsp(struct irot_priv *p, const struct irot_msg *m)
> +{
> +	u32 mtu = le32_to_cpu(m->data.buffers.mtu_limit);
> +
> +	scoped_guard(spinlock_irqsave, &p->rx_lock) {
> +		if (!p->buffers_valid && mtu) {
> +			irot_region_from_span(&p->host_write,
> +					      &m->data.buffers.host_write);
> +			irot_region_from_span(&p->host_read,
> +					      &m->data.buffers.host_read);
> +			p->mtu = mtu;
> +			p->buffers_valid = true;
> +		}
> +	}
> +
> +	complete(&p->buffers_done);
> +}

[Severity: Medium]
Should irot_cb_buffer_rsp() only call complete(&p->buffers_done) when mtu
is non-zero?

When the peer replies with IROT_CC_MCTP_BUFFER containing mtu_limit == 0,
p->buffers_valid remains false, but complete(&p->buffers_done) still wakes
wait_for_completion_timeout() in irot_discover_buffers() immediately:

	for (i = 0; i < IROT_BUFFER_RETRIES && !got; i++) {
		reinit_completion(&p->buffers_done);

		rc = irot_send(p, &req);
		...
		wait_for_completion_timeout(&p->buffers_done,
					    secs_to_jiffies(IROT_BUFFER_TIMEOUT_S));

		scoped_guard(spinlock_irqsave, &p->rx_lock)
			got = p->buffers_valid;
	}

Wouldn't this cause irot_discover_buffers() to spin through all
IROT_BUFFER_RETRIES attempts back-to-back without waiting for the intended
timeout window before failing probe with -ETIMEDOUT?

[ ... ]
> +static void irot_rx_work(struct work_struct *work)
> +{
[ ... ]
> +	if (do_rx) {
> +		res = irot_rx_deliver(p, &e, &dropped);
> +
> +		scoped_guard(spinlock_irqsave, &p->rx_lock) {
> +			p->rx_busy = false;
> +			if (res == IROT_RX_DELIVERED) {
> +				p->last_rx_counter = e.counter;
> +				p->has_last_rx_counter = true;
> +			}
> +		}
> +	}
> +
> +	local_bh_disable();
> +	if (dropped)
> +		dev_dstats_rx_dropped_add(p->ndev, dropped);
> +	if (do_rx && res == IROT_RX_DELIVERED)
> +		dev_dstats_rx_add(p->ndev, e.size);
> +	local_bh_enable();
> +
> +	/* Withhold the ACK and the peer resends */
> +	if (do_rx && res != IROT_RX_RETRY)
> +		irot_send_done(p, e.counter);
> +}

[Severity: High]
When irot_rx_deliver() returns IROT_RX_INVALID (for example, when a packet
arrives while !netif_running(p->ndev)), irot_rx_work() still sends
IROT_CC_MCTP_DONE via irot_send_done(p, e.counter), which releases the
host_read window back to the IRoT.

However, p->rx_busy is cleared before irot_send_done() is called, and
p->last_rx_counter is only updated when res == IROT_RX_DELIVERED.

If the IRoT resends the same counter before receiving that ACK (or while
irot_send_done() is waiting on tx_chan_lock), irot_cb_mctp() will see
p->rx_busy == false and e.counter != p->last_rx_counter:

	scoped_guard(spinlock_irqsave, &p->rx_lock) {
		if (p->has_last_rx_counter && e.counter == p->last_rx_counter) {
			/* Resend of a delivered packet: our ACK was missed */
			p->dup_ack_counter = e.counter;
			p->dup_ack_pending = true;
			wake = true;
		} else if (p->rx_busy) {
			...
		} else {
			p->rx_entry = e;
			p->rx_busy = true;
			p->rx_new = true;
			wake = true;
		}
	}

Can this cause the resend to be queued as a new packet in p->rx_entry,
allowing irot_rx_work() to read from host_read concurrently with the IRoT
writing the next packet after receiving the first IROT_CC_MCTP_DONE?

[ ... ]
> +static int irot_ndo_open(struct net_device *ndev)
> +{
> +	struct irot_priv *p = netdev_priv(ndev);
> +
> +	/*
> +	 * A busy or stalled window keeps the queue stopped; tx_work wakes it
> +	 * once the ACK arrives, since netif_running() is already true here.
> +	 */
> +	if (irot_tx_ready(p))
> +		netif_start_queue(ndev);
> +	else
> +		netif_stop_queue(ndev);
> +
> +	return 0;
> +}

[Severity: High]
Can irot_ndo_open() race with irot_tx_wake() and leave the TX queue
permanently stopped?

In irot_ndo_open(), irot_tx_ready(p) checks !p->pending_write &&
!p->tx_stalled && !p->tx_skb under p->tx_state_lock, and then releases the
lock before calling netif_stop_queue(ndev).

If a packet was in flight from before the interface was closed and its
IROT_CC_MCTP_DONE arrives right after irot_tx_ready(p) returns false:

CPU0                                    CPU1
irot_ndo_open()
  irot_tx_ready(p) -> false
                                        irot_tx_mark_done()
                                          p->pending_write = false;
                                          queue_work(p->wq, &p->tx_work);
                                        irot_tx_work()
                                          irot_tx_wake()
                                            lock(&p->tx_state_lock)
                                            netif_wake_queue(p->ndev);
                                            unlock(&p->tx_state_lock)
  netif_stop_queue(ndev);

Since p->pending_write is now false and no further ACK will arrive,
wouldn't netif_stop_queue(ndev) leave the queue stopped forever?

> +
> +static int irot_ndo_stop(struct net_device *ndev)
> +{
> +	struct irot_priv *p = netdev_priv(ndev);
> +	struct sk_buff *skb;
> +
> +	netif_stop_queue(ndev);
> +
> +	/*
> +	 * Do not touch tx_timeout_work: a packet in flight still owns the
> +	 * window, and the stall and late-ACK handling must keep running.
> +	 */
> +	cancel_work_sync(&p->tx_work);

[Severity: High]
What happens if a stalled TX transfer receives its late IROT_CC_MCTP_DONE
just before irot_ndo_stop() runs cancel_work_sync(&p->tx_work)?

When a TX transfer times out, irot_tx_stall() sets p->tx_stalled = true and
calls netif_carrier_off(p->ndev). When the late ACK arrives in
irot_rx_callback(), irot_tx_mark_done() clears p->pending_write and queues
p->tx_work:

	scoped_guard(spinlock_irqsave, &p->tx_state_lock) {
		if (!p->pending_write || p->pending_tx_counter != counter)
			return;

		p->pending_write = false;
	}

	cancel_delayed_work(&p->tx_timeout_work);
	queue_work(p->wq, &p->tx_work);

Clearing p->tx_stalled and calling netif_carrier_on(p->ndev) only happens
when p->tx_work runs irot_tx_wake():

	scoped_guard(spinlock_irqsave, &p->tx_state_lock) {
		if (p->pending_write || p->tx_skb)
			return;

		if (p->tx_stalled) {
			p->tx_stalled = false;
			recovered = true;
		}
		...
	}

	if (recovered) {
		dev_dbg(p->dev, "late TX ACK arrived, link up\n");
		netif_carrier_on(p->ndev);
	}

If irot_ndo_stop() cancels p->tx_work after irot_tx_mark_done() has already
set p->pending_write = false, irot_tx_wake() never runs. Does this leave
p->tx_stalled == true and the carrier off permanently, causing subsequent
irot_ndo_open() calls to see irot_tx_ready(p) == false and keep the TX
queue stopped?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101-mctp-transport-irot-ax3005-v1-0-36ceb822f01e@axiado.com?part=2