Re: [PATCH net-next 04/12] net: mctp: usblib: Move TX transfer processing to mctp-usblib

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.com>

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/
---
net: mctp: usblib: Move TX transfer processing to mctp-usblib

This change moves the TX path from mctp-usb into the mctp-usblib helper
by splitting it into push, send, and complete steps so future packed
transfers can carry multiple MCTP packets per USB transfer. The current
implementation keeps a 1:1 mapping between packets and transfers, with no
tx context persisting beyond a single send.

> diff --git a/drivers/net/mctp/mctp-usb.c b/drivers/net/mctp/mctp-usb.c
> index 531b7c994afb..385350792dd4 100644
> --- a/drivers/net/mctp/mctp-usb.c
> +++ b/drivers/net/mctp/mctp-usb.c
> @@ -29,90 +29,82 @@ struct mctp_usb {

[ ... ]

> @@ -220,7 +212,12 @@ static int mctp_usb_stop(struct net_device *dev)
>  	flush_delayed_work(&mctp_usb->rx_retry_work);
>  
>  	usb_kill_urb(mctp_usb->rx_urb);
> +
> +	spin_lock_irqsave(&mctp_usb->tx_lock, flags);
>  	usb_kill_urb(mctp_usb->tx_urb);
> +	spin_unlock_irqrestore(&mctp_usb->tx_lock, flags);

[Critical]
Can usb_kill_urb() safely be called from under spin_lock_irqsave()?

usb_kill_urb() starts with might_sleep() and then blocks on
wait_event(usb_kill_urb_queue, urb->use_count == 0):

drivers/usb/core/urb.c:usb_kill_urb() {
    might_sleep();
    ...
    wait_event(usb_kill_urb_queue, atomic_read(&urb->use_count) == 0);
    ...
}

With CONFIG_DEBUG_ATOMIC_SLEEP, would this trigger a
"sleeping function called from invalid context" splat because tx_lock
is held with IRQs disabled?

There also appears to be a nested-lock concern with the URB completion
handler taking the same tx_lock:

mctp_usb_out_complete() {
    ...
    spin_lock_irqsave(&mctp_usb->tx_lock, flags);
    mctp_usb->tx_urb = NULL;
    spin_unlock_irqrestore(&mctp_usb->tx_lock, flags);
    ...
}

The HCD giveback path needs to run the completion before urb->use_count
drops to zero. If the completion is waiting for tx_lock held by the
caller of usb_kill_urb(), and usb_kill_urb() is itself waiting for
use_count to drop, can this deadlock?

The next patch in the series ("net: mctp: usb: Allow for multiple urb
submissions from a packet tx", 5cf1f88ce49a) appears to remove this
lock-wrapped usb_kill_urb() in favour of usb_kill_anchored_urbs()
called outside the lock — should the fix be folded into this patch
instead?

> +
> +	mctp_usblib_tx_cancel(&mctp_usb->tx, dev);
>  
>  	return 0;
>  }

[ ... ]

> diff --git a/drivers/net/mctp/mctp-usblib.c b/drivers/net/mctp/mctp-usblib.c
> index 9b86eb4310ce..56eca496bbe4 100644
> --- a/drivers/net/mctp/mctp-usblib.c
> +++ b/drivers/net/mctp/mctp-usblib.c
> @@ -162,6 +162,186 @@ void mctp_usblib_rx_cancel(struct mctp_usblib_rx *rx)

[ ... ]

> +/*
> + * Completion for the ->send() op. This will update netdev stats and
> + * free the tx context.
> + *
> + * Likely called from (atomic) URB completion context.
> + */
> +void mctp_usblib_tx_send_complete(struct mctp_usblib_tx_ctx *tx_ctx,
> +				  struct net_device *dev, bool ok)
> +{
> +	mctp_usblib_tx_ctx_free(tx_ctx);
> +}
> +EXPORT_SYMBOL_GPL(mctp_usblib_tx_send_complete);

[High]
Does the body of mctp_usblib_tx_send_complete() match the comment
above it?

The comment says "This will update netdev stats and free the tx
context", and the function takes dev and ok parameters, but the body
only calls mctp_usblib_tx_ctx_free(tx_ctx) and never invokes
mctp_usblib_tx_stats_update().

Prior to this patch, mctp_usb_out_complete() updated counters for both
success and failure:

    case 0:
        dev_dstats_tx_add(netdev, skb->len);
        ...
    case -ENOENT:
    case -ECONNRESET:
    case -ESHUTDOWN:
    case -EPROTO:
        dev_dstats_tx_dropped(netdev);
        ...

After this patch, mctp_usb_out_complete() defers stats to
mctp_usblib_tx_send_complete(), which does not touch them. The only
caller of mctp_usblib_tx_stats_update() is the synchronous push()
failure path. Does this mean tx_packets, tx_bytes and the URB-failure
tx_drops counters are no longer incremented for any successful or
asynchronously-failed transmit?

Was the intent to call mctp_usblib_tx_stats_update(tx_ctx, dev, ok)
here before freeing the context?

> +
> +/* Prepare a skb for push() */
> +static int mctp_usblib_tx_skb_prepare(struct sk_buff *skb)

[ ... ]
-- 
This is an AI-generated review.