Re: [PATCH can-next v6 1/6] can: gs_usb: gs_make_candev(): check can_rx_offload_add_manual() return value

[netdev-bot+sashiko]

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] On the current tree the new error branch in gs_make_candev()
  cannot be reached.

--

can: gs_usb: gs_make_candev(): check can_rx_offload_add_manual() return value

This patch makes gs_make_candev() check the return value of
can_rx_offload_add_manual(). On failure it now exits through the existing
out_free_candev path. The commit message says the change prepares for an
upcoming can_rx_offload series in which can_rx_offload_add_manual() can
fail.

Link: https://patch.msgid.link/20260907104942.17089-4-ciprianmarian.costea@xxxxxxxxxxx

> Fixes: 24bc41b45583 ("can: gs_usb: convert to NAPI/rx-offload to avoid OoO reception")

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 3b9b2f104d868..cac4ce323a1de 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -1487,7 +1487,10 @@ static struct gs_can *gs_make_candev(unsigned int channel,
>  		dev->can.fd.data_bittiming_const = &dev->data_bt_const;
>  	}
>  
> -	can_rx_offload_add_manual(netdev, &dev->offload, GS_NAPI_WEIGHT);
> +	rc = can_rx_offload_add_manual(netdev, &dev->offload, GS_NAPI_WEIGHT);
> +	if (rc)
> +		goto out_free_candev;
> +

[Severity: Low]
This isn't a bug, but can the new error branch be reached on the current
tree?

can_rx_offload_add_manual() in drivers/net/can/dev/rx-offload.c only
fails when mailbox_read is set:

    if (offload->mailbox_read)
        return -EINVAL;

    return can_rx_offload_init_queue(dev, offload, weight);

gs_usb never sets mailbox_read. The private area returned by
alloc_candev() is zeroed, so this check always passes.

can_rx_offload_init_queue() always returns 0. It also sets up
skb_irq_queue as an embedded struct sk_buff_head, so the
"offload->skb_irq_queue == NULL" case only appears after the upcoming
rx-offload rework.

The commit message does say this failure comes "In an upcoming series".
Given that, would it be clearer to present this as preparation for that
series rather than tag it with Fixes: 24bc41b45583? Today the check
cannot trigger, so backporters may read the Fixes: tag as pointing to a
bug users can hit.

>  	SET_NETDEV_DEV(netdev, &intf->dev);
>  
>  	rc = register_candev(dev->netdev);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113-gs_usb-fixes-v6-0-0c039e1bfd74%40pengutronix.de