Re: [PATCH net v2] net: usb: ipheth: stop RX URB on ndo_stop

[Jakub Kicinski <kuba@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.
---
net: usb: ipheth: stop RX URB on ndo_stop

ipheth_close() previously stopped the netdev queue and disabled the carrier
work but left the RX URB submitted, so an in-flight RX could still deliver
packets and re-arm itself after an administrative link down.  This commit
adds a usb_kill_urb() on the RX URB in ndo_stop, deliberately leaving an
in-flight TX URB to complete on its own.

> diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
> index 2b490114d2327..6b90fe1e0e382 100644
> --- a/drivers/net/usb/ipheth.c
> +++ b/drivers/net/usb/ipheth.c
> @@ -505,6 +505,7 @@ static int ipheth_close(struct net_device *net)
>  	 * it, so that such a schedule_delayed_work() is a no-op.
>  	 */
>  	disable_delayed_work_sync(&dev->carrier_work);
> +	usb_kill_urb(dev->rx_urb);
>  	return 0;
>  }

[Severity: Low]
Can this make a plain "ip link set ethN down" print a KERN_ERR line while
RX traffic is flowing?

usb_kill_urb() does atomic_inc(&urb->reject) before it unlinks and waits,
so a giveback that is already in progress can run ipheth_rcvbulk_callback()
with status 0 and fall through to the resubmit:

drivers/net/usb/ipheth.c:ipheth_rcvbulk_callback() {
	...
rx_submit:
	dev->confirmed_pairing = true;
	ipheth_rx_submit(dev, GFP_ATOMIC);
}

That resubmit then hits the reject check in usb_hcd_submit_urb():

drivers/usb/core/hcd.c:usb_hcd_submit_urb() {
	...
	/* Check that the URB isn't being killed */
	if (unlikely(atomic_read(&urb->reject))) {
		rc = -EPERM;
		goto done;
	}

and ipheth_rx_submit() logs that unconditionally:

drivers/net/usb/ipheth.c:ipheth_rx_submit() {
	...
	retval = usb_submit_urb(dev->rx_urb, mem_flags);
	if (retval)
		dev_err(&dev->intf->dev, "%s: usb_submit_urb: %d\n",
			__func__, retval);

Before this change urb->reject was never raised on the close path, so this
-EPERM branch was not reachable from ndo_stop.

The two sites also seem to disagree about what -EPERM means: ipheth_close()
treats the blocked resubmit as the intended outcome, while
ipheth_rx_submit() reports it as a device error.  Would it be worth teaching
ipheth_rx_submit() (or the rx_submit path in ipheth_rcvbulk_callback()) to
skip the dev_err() for -EPERM?

Related to that, ipheth_rcvbulk_callback() discards the return value of
ipheth_rx_submit(), so the now-expected -EPERM and a genuine failure such
as -ENOMEM, which leaves RX dead until the next ipheth_open(), are treated
identically.  Should the callback distinguish them?

Should the commit message also mention the new log output on the close path?
-- 
pw-bot: cr