Re: [PATCH] can: isotp: fix tx state handling for echo tx processing

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Marc,

On 04.11.22 13:10, Marc Kleine-Budde wrote:
On 01.11.2022 22:29:02, Oliver Hartkopp wrote:
In commit 4b7fe92c0690 ("can: isotp: add local echo tx processing for
consecutive frames") the data flow for consecutive frames (CF) has been
reworked to improve the reliability of long data transfers.

This rework did not touch the transmission and the tx state changes of
single frame (SF) transfers which likely led to the WARN in the
isotp_tx_timer_handler() catching a wrong tx state. This patch makes use
of the improved frame processing for SF frames and sets the ISOTP_SENDING
state in isotp_sendmsg() within the cmpxchg() condition handling.

A review of the state machine and the timer handling additionally revealed
a missing echo timeout handling in the case of the burst mode in
isotp_rcv_echo() and removes a potential timer configuration uncertainty
in isotp_rcv_fc() when the receiver requests consecutive frames.

Fixes: 4b7fe92c0690 ("can: isotp: add local echo tx processing for consecutive frames")
Link: https://lore.kernel.org/linux-can/CAO4mrfe3dG7cMP1V5FLUkw7s+50c9vichigUMQwsxX4M=45QEw@xxxxxxxxxxxxxx/T/#u
Reported-by: Wei Chen <harperchen1110@xxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx # v6.0
Signed-off-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>

[...]

@@ -905,10 +915,11 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
  		so->tx.state = ISOTP_IDLE;
  		wake_up_interruptible(&so->wait);
  		break;
default:
+		pr_notice_once("can-isotp: tx timer state %X\n", so->tx.state);
  		WARN_ON_ONCE(1);

Can you use WARN_ONCE() instead of pr_notice_once() + WARN_ON_ONCE() here?


Yes. That was a good idea! V2 is sent.

It also allowed me to print another relevant variable.

Thanks,
Oliver



[Index of Archives]     [Automotive Discussions]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]     [CAN Bus]

  Powered by Linux