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@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> ---
>   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