Re: [PATCH v6 09/13] can: peak_usb: register the netdevs only once all channels are set up

[Stéphane Grosjean]

On 24/10/2026 05:58, netdev-bot+sashiko@xxxxxxxxxx wrote:
[...]
> [Severity: Low]
> Should the helpers on this same path be converted as well?
> pcan_usb_wait_rsp() and pcan_usb_send_cmd() still log with
> netdev_err(dev->netdev, ...):
[...]
> These messages
> therefore print with an "(unnamed net_device) (uninitialized)" prefix.
[...]
> The new lbl_disconnect path in peak_usb_probe() has a similar issue.
> peak_usb_disconnect() copies netdev->name and logs "%s removed" for
> channels that were created but never registered, so it prints a literal
> "can%d removed".

Right. The "can%d removed" message is a regression of this patch: v7
only reports the registered netdevs as removed.

The netdev_err() of the command helpers only print on error, and only
the prefix is wrong. Converting all of them in this patch would make a
Fixes: patch grow for a cosmetic issue, so I'll do it in a separate
patch for linux-can-next.

[...]
> [Severity: High]
> This isn't a bug introduced by this patch, but can the PCAN-USB restart
> timer fire after free_candev() in this loop?
[...]
> When the timer expires, pcan_usb_restart()->peak_usb_restart_complete()
> writes dev->can.state and calls netif_wake_queue() on the freed netdev.

Right, thanks. This is a real bug, present since the PCAN-USB support was
added. Tristan Madani has posted a fix for it, independently [1], so I
won't fix it in this series, and both apply in any order.

The Sashiko review of his patch [2] found that its other part, the init
failure path, isn't closed by shutting the timer down in
pcan_usb_init(): the unanchored restart URB may still be in flight. That
path is closed by this patch, since no netdev can be opened, nor
restarted, before all the channels are set up.

[...]
> [Severity: Medium]
> This is a pre-existing issue, but take a multi-channel adapter (PCAN-USB
> Pro, or a two-channel PCAN-USB FD). Channel 0 is registered and opened by
> userspace, and then register_candev() fails for channel 1. What state is
> the hardware left in?
>
> The commit message says:
>
>   Since no netdev can be opened during probe anymore, there is no RX to
>   stop when probing a channel fails.
>
> That doesn't quite hold inside this loop.
[...]
> Does this leave the still-attached adapter bus-on on the opened channel?
> Calibration messages would stay enabled, and the firmware would still
> believe a driver is loaded.
>
> The same ordering existed before this patch.

Right, the sentence was too broad. v7 says instead:

  Since no netdev is registered while the channels are being created,
  there is no RX to stop anymore when creating a channel fails. If
  registering a netdev fails, peak_usb_disconnect() still stops RX on
  the channels already registered, which may have been opened in the
  meantime.

For the bus-on state, this is the same pre-existing issue as the one
reported on patch 6: I'll deal with it in a separate series.

[1] https://lore.kernel.org/r/20261025210256.629830-1-tristan@xxxxxxxxxxxxxxxxxxx
[2] https://lore.kernel.org/r/179304858361.3134.1029344134259623994@xxxxxxxxxx

Thanks!

Stéphane