Re: [PATCH v7 06/13] can: peak_usb: harden channel handling in PCAN-USB FD

[Stéphane Grosjean]

On 01/11/2026 03:15, netdev-bot+sashiko@xxxxxxxxxx wrote:
[...]
>>  adap_dev_free:
>> +	/* the whole interface is released when probing a channel fails:
>> +	 * stop RX on this channel, which may already be open, and on the
>> +	 * channels already created, since their RX URBs may carry records
>> +	 * for this one.
>> +	 */
>> +	peak_usb_stop_rx(dev);
>
> [Severity: Medium]
> Does clearing CONNECTED here suppress the shutdown commands that ndo_stop
> sends for a channel that is already open?
[...]
> Could
> a probe failure now leave the controller bus-on, with calibration records
> still streaming, while the device stays attached?

At this commit, the only way to reach adap_dev_free is a failure of
dev_set_bus(dev, 0) just above: sending bus off to this channel has
just failed, so ndo_stop() would most likely fail to send it, and to
turn off the calibration messages, as well. Moreover, the channel can
only be open here if userspace opened it between register_candev()
and dev_init(), where opening it already dereferences a NULL usb_if
(the pre-existing issue you report below).

As you note, patch 9 removes this call, and no netdev is registered on
this path anymore. So I'll keep it as is for these three commits. v8
will mention it in its changelog.

[...]
> [Severity: High]
> This is a pre-existing issue, but does this CONNECTED test close the
> window where the netdev is visible before its private data is set up?
[...]
> [Severity: High]
> This isn't a bug introduced by this patch, but is it safe to call
> dev_free() before unregister_candev() on this error path?

Both are pre-existing, and fixed by patch 9, as you note.

[...]
> [Severity: High]
> This is a pre-existing issue, but can an asynchronous restart URB outlive
> the peak_usb_device it points to on the probe error path?
[...]
> No patch in the series tracks
> or kills restart URBs.

Right. The restart URB isn't anchored, so peak_usb_unlink_all_urbs()
doesn't kill it, and its completion may run after free_candev(). This
isn't specific to the probe error path: on unbind, the endpoints are
only disabled after ->disconnect() returns. The window is narrow (a
short bulk OUT transfer right after a restart), but real.

This series doesn't touch this code, so I'll fix it in the separate
series announced in v6, by anchoring the restart URB so that it is
killed with the other ones.

Thanks!

Stéphane