Re: [PATCH] can: bcm: Account cyclic transmissions only on success

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In bcm_can_tx() (net/can/bcm.c:378), the patch changes the
  condition to `if (!err && (!cyclic || op->currframe == idx))`.
- [Low] The commit fixes a user-visible BCM bug: cyclic transmissions and
  the ival1 count are used up, and TX_EXPIRED can be reported, without…

--

can: bcm: Account cyclic transmissions only on success

This patch changes bcm_can_tx() so that op->currframe and op->count only
move forward when can_send() returns success. The goal is to stop the BCM
timer from using up the configured ival1 transmissions, and reporting
TX_EXPIRED, when no frame was sent, for example while the interface is down.

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 7bd2aa39777af..cd66b23bf95c2 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -375,7 +375,7 @@ static void bcm_can_tx(struct bcm_op *op, struct canfd_frame *cf)
>  	 * we were sending - a concurrent TX_RESET_MULTI_IDX means this
>  	 * frame's bookkeeping belongs to a sequence that no longer exists
>  	 */
> -	if (!cyclic || op->currframe == idx) {
> +	if (!err && (!cyclic || op->currframe == idx)) {

[Severity: Medium]

What happens here when can_send() keeps failing with a persistent error?

With the new !err gate, op->count is never decremented. The timer logic
depends on op->count reaching zero:

net/can/bcm.c:bcm_tx_timeout_handler() {
    ...
	tx_ival1 = op->kt_ival1 && (op->count > 0);
    ...
		if (!count && (flags & TX_COUNTEVT)) {
			/* create notification to user */
    ...
}

net/can/bcm.c:bcm_tx_set_expiry() {
    ...
	if (op->kt_ival1 && op->count)
		ival = op->kt_ival1;
	else if (op->kt_ival2) {
		ival = op->kt_ival2;
	} else {
		spin_unlock_bh(&op->bcm_tx_lock);
		return false;
	}
    ...
}

While the error lasts, the op stays in the ival1 phase with no limit. It
never switches to ival2 and never sends TX_EXPIRED.

An op with ival2 == 0 used to stop on its own after count attempts. It now
keeps re-arming at ival1 until the error clears or userspace sends
TX_DELETE.

Several can_send() failures on this path look persistent:

- -ENETDOWN while the interface is administratively down. The NETDEV_DOWN
  handling in bcm_notifier() only sets sk->sk_err. Only NETDEV_UNREGISTER
  cancels the tx_op timers.
- -EMSGSIZE when a CAN_FD_FRAME op targets a device with CAN_MTU, or after
  the MTU is lowered. bcm_tx_setup() only checks cf->len.
- -EPERM and -EINVAL.
- -ENOBUFS once the qdisc fills on a stopped or bus-off controller.

This also changes what count means in the UAPI. It used to count ival1
transmission attempts. It now counts frames that can_send() accepted. The
value reported through TX_READ and TX_STATUS changes to match.

Documentation/networking/can.rst describes count as a timer schedule:
"run 'count' times with ival1". It describes TX_EXPIRED as "Notification
when counter finishes sending at initial interval". It does not say that
failed sends are retried at ival1 with no limit.

The commit message only covers the opposite symptom, TX_EXPIRED being
reported too early.

Was this change in meaning intended? If so, could the commit message and
the documentation describe it? Should there be a limit when the error is
permanent, such as -ENETDOWN or -EMSGSIZE?

[Severity: Low]

Should this commit have a Fixes: tag?

The behaviour being changed goes back to the original BCM code. git blame
points at ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol").
The count decrement was later moved into bcm_can_tx() by c2aba69d0c36a4.
The TX_RESET_MULTI_IDX guard was added by 749179c2e25b95.

Could the commit name one of these in a Fixes: tag? If it is not meant for
stable, could it say that this is a behaviour change instead?

>  		op->currframe++;
>  
>  		/* reached last frame? */

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/45A9B6D8611C63C9%2B20261110052315.253367-1-raoxu%40uniontech.com