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

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

[raoxu]

From: Xu Rao <raoxu@uniontech.com>

The CAN broadcast manager advances the cyclic frame index and decrements
the initial-interval transmission count even when can_send() fails.
For example, taking the CAN interface down makes can_send() return
-ENETDOWN, but the timer still consumes the configured transmissions and
can report TX_EXPIRED without sending a frame.

Only update cyclic bookkeeping after can_send() succeeds. This matches
the existing successful-send accounting of frames_abs while preserving
the concurrent TX_RESET_MULTI_IDX check.

Signed-off-by: Xu Rao <raoxu@uniontech.com>
---
 net/can/bcm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index 7bd2aa39777a..cd66b23bf95c 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)) {
 		op->currframe++;

 		/* reached last frame? */
--
2.50.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] can: bcm: missing Fixes: trailer for bcm_can_tx() bookkeeping
  fix
--

commit ca441329b91ed7ef08326f38c9caaba62d3a18f6
Author: Xu Rao <raoxu@uniontech.com>

> can: bcm: Account cyclic transmissions only on success
>
> The CAN broadcast manager advances the cyclic frame index and decrements
> the initial-interval transmission count even when can_send() fails.
> For example, taking the CAN interface down makes can_send() return
> -ENETDOWN, but the timer still consumes the configured transmissions and
> can report TX_EXPIRED without sending a frame.
>
> Only update cyclic bookkeeping after can_send() succeeds. This matches
> the existing successful-send accounting of frames_abs while preserving
> the concurrent TX_RESET_MULTI_IDX check.
>
> Signed-off-by: Xu Rao <raoxu@uniontech.com>

[Severity: Low]
Since this fixes a bug in bcm_can_tx() cyclic transmission accounting,
should the commit message include a Fixes: trailer:

  Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/45A9B6D8611C63C9+20261110052315.253367-1-raoxu@uniontech.com?part=1

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

[Oliver Hartkopp]

Hello Xu Rao,

thanks for looking into this, but I don't think this change is the
right approach.

On 10.11.26 06:23, raoxu wrote:
> From: Xu Rao <raoxu@uniontech.com>
> 
> The CAN broadcast manager advances the cyclic frame index and decrements
> the initial-interval transmission count even when can_send() fails.
> For example, taking the CAN interface down makes can_send() return
> -ENETDOWN, but the timer still consumes the configured transmissions and
> can report TX_EXPIRED without sending a frame.

This is intended behaviour. The BCM cyclic transmission is designed as
a time-driven "fire and forget" mechanism: 'count' defines the number
of ival1 time slots before switching to ival2. It does not define the
number of successfully transmitted frames. The timer schedule must
not depend on the state of the CAN interface.

With your patch, if the interface is down (or the tx queue is full
with -ENOBUFS, e.g. no ACK or bus-off), the following happens:

- bcm_tx_set_expiry() still sees 'kt_ival1 && count' and re-arms the
   timer with ival1 forever. The op never switches to ival2 and
   TX_EXPIRED is never sent.

- For setups with ival2 == 0 ("send 'count' frames and stop") the
   hrtimer now never stops and keeps firing at ival1 until the op is
   deleted.

- When the interface comes back up, the remaining ival1 burst is sent
   with a delay that can be arbitrarily long. This defeats the purpose
   of the initial ival1 phase, which is usually a short burst right
   after a content change.

- In the ival2 phase, currframe no longer advances on errors. A
   multiplex sequence then gets stuck repeating the same frame while
   the tx queue is full. Losing one frame and continuing with the next
   mux index in the next time slot is the expected behaviour of a
   time-triggered sender.

> Only update cyclic bookkeeping after can_send() succeeds. This matches
> the existing successful-send accounting of frames_abs while preserving
> the concurrent TX_RESET_MULTI_IDX check.

frames_abs is a statistic that counts what was actually sent.
currframe and count are control data for the transmission schedule.
They intentionally follow different rules.

The cyclic path has no way to report can_send() errors back to the
user. Simply following the timing schedule, the same way a sender
without bus access would, is the consistent behaviour here.

If an application needs "N successfully sent frames", it should
monitor the interface state (e.g. via rtnetlink) in user space and
re-issue TX_SETUP with SETTIMER|STARTTIMER after the interface is up
again.

Therefore I would prefer to keep the current behaviour. So NAK from my 
side for this patch.

If the current semantics are not clear enough from the documentation,
I would be happy to take a patch for
Documentation/networking/can.rst that states explicitly that 'count'
refers to ival1 timer events and not to successful transmissions.

Best regards,
Oliver

> 
> Signed-off-by: Xu Rao <raoxu@uniontech.com>
> ---
>   net/can/bcm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 7bd2aa39777a..cd66b23bf95c 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)) {
>   		op->currframe++;
> 
>   		/* reached last frame? */
> --
> 2.50.1
> 

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