[PATCH net 0/4] support multipacket broadcast message

[PATCH net 0/4] support multipacket broadcast message

[Zhang Changzhong]

Zhang Changzhong (4):
  can: j1939: fix support for multipacket broadcast message
  can: j1939: cancel rxtimer on multipacket broadcast session complete
  can: j1939: abort multipacket broadcast session when timeout occurs
  can: j1939: add rxtimer for multipacket broadcast session

 net/can/j1939/transport.c | 48 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 12 deletions(-)

-- 
2.9.5

[PATCH net 1/4] can: j1939: fix support for multipacket broadcast message

[Zhang Changzhong]

Currently j1939_tp_im_involved_anydir() in j1939_tp_recv() check the
previously set flags J1939_ECU_LOCAL_DST and J1939_ECU_LOCAL_SRC of
incoming skb, thus multipacket broadcast message was aborted by
receive side because it may come from remote ECUs and have no exact
dst address. Similarly, j1939_tp_cmd_recv() and j1939_xtp_rx_dat()
didn't process broadcast message.

So fix it by checking and process broadcast message in j1939_tp_recv(),
j1939_tp_cmd_recv() and j1939_xtp_rx_dat().

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com>
---
 net/can/j1939/transport.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 9f99af5..e5188ac 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1651,8 +1651,12 @@ static void j1939_xtp_rx_rts(struct j1939_priv *priv, struct sk_buff *skb,
 			return;
 		}
 		session = j1939_xtp_rx_rts_session_new(priv, skb);
-		if (!session)
+		if (!session) {
+			if (cmd == J1939_TP_CMD_BAM && j1939_sk_recv_match(priv, skcb))
+				netdev_info(priv->ndev, "%s: failed to create TP BAM session\n",
+					    __func__);
 			return;
+		}
 	} else {
 		if (j1939_xtp_rx_rts_session_active(session, skb)) {
 			j1939_session_put(session);
@@ -1829,6 +1833,13 @@ static void j1939_xtp_rx_dat(struct j1939_priv *priv, struct sk_buff *skb)
 		else
 			j1939_xtp_rx_dat_one(session, skb);
 	}
+
+	if (j1939_cb_is_broadcast(skcb)) {
+		session = j1939_session_get_by_addr(priv, &skcb->addr, false,
+						    false);
+		if (session)
+			j1939_xtp_rx_dat_one(session, skb);
+	}
 }
 
 /* j1939 main intf */
@@ -1920,7 +1931,7 @@ static void j1939_tp_cmd_recv(struct j1939_priv *priv, struct sk_buff *skb)
 		if (j1939_tp_im_transmitter(skcb))
 			j1939_xtp_rx_rts(priv, skb, true);
 
-		if (j1939_tp_im_receiver(skcb))
+		if (j1939_tp_im_receiver(skcb) || j1939_cb_is_broadcast(skcb))
 			j1939_xtp_rx_rts(priv, skb, false);
 
 		break;
@@ -1984,7 +1995,7 @@ int j1939_tp_recv(struct j1939_priv *priv, struct sk_buff *skb)
 {
 	struct j1939_sk_buff_cb *skcb = j1939_skb_to_cb(skb);
 
-	if (!j1939_tp_im_involved_anydir(skcb))
+	if (!j1939_tp_im_involved_anydir(skcb) && !j1939_cb_is_broadcast(skcb))
 		return 0;
 
 	switch (skcb->addr.pgn) {
-- 
2.9.5

[PATCH net 2/4] can: j1939: cancel rxtimer on multipacket broadcast session complete

[Zhang Changzhong]

If j1939_xtp_rx_dat_one() receive last frame of multipacket broadcast
message, j1939_session_timers_cancel() should be called to cancel
rxtimer.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com>
---
 net/can/j1939/transport.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index e5188ac..dd6a120 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1788,6 +1788,7 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session,
 	}
 
 	if (final) {
+		j1939_session_timers_cancel(session);
 		j1939_session_completed(session);
 	} else if (do_cts_eoma) {
 		j1939_tp_set_rxtimeout(session, 1250);
-- 
2.9.5

[PATCH net 3/4] can: j1939: abort multipacket broadcast session when timeout occurs

[Zhang Changzhong]

If timeout occurs, j1939_tp_rxtimer() first calls hrtimer_start() to
restart rxtimer, and then calls __j1939_session_cancel() to set
session->state = J1939_SESSION_WAITING_ABORT. At next timeout
expiration, because of the J1939_SESSION_WAITING_ABORT session state
j1939_tp_rxtimer() will call j1939_session_deactivate_activate_next()
to deactivate current session, and rxtimer won't be set.

But for multipacket broadcast session, __j1939_session_cancel() don't
set session->state = J1939_SESSION_WAITING_ABORT, thus current session
won't be deactivate and hrtimer_start() is called to start new
rxtimer again and again.

So fix it by moving session->state = J1939_SESSION_WAITING_ABORT out of
if (!j1939_cb_is_broadcast(&session->skcb)) statement.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com>
---
 net/can/j1939/transport.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index dd6a120..5757f9f 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1055,9 +1055,9 @@ static void __j1939_session_cancel(struct j1939_session *session,
 	lockdep_assert_held(&session->priv->active_session_list_lock);
 
 	session->err = j1939_xtp_abort_to_errno(priv, err);
+	session->state = J1939_SESSION_WAITING_ABORT;
 	/* do not send aborts on incoming broadcasts */
 	if (!j1939_cb_is_broadcast(&session->skcb)) {
-		session->state = J1939_SESSION_WAITING_ABORT;
 		j1939_xtp_tx_abort(priv, &session->skcb,
 				   !session->transmission,
 				   err, session->skcb.addr.pgn);
-- 
2.9.5

[PATCH net 4/4] can: j1939: add rxtimer for multipacket broadcast session

[Zhang Changzhong]

According to SAE J1939/21 (Chapter 5.12.3 and APPENDIX C), for transmit
side the required time interval between packets of a multipacket
broadcast message is 50 to 200 ms, the responder shall use a timeout of
250ms (provides margin allowing for the maximumm spacing of 200ms). For
receive side a timeout will occur when a time of greater than 750 ms
elapsed between two message packets when more packets were expected.

So this patch fix and add rxtimer for multipacket broadcast session.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com>
---
 net/can/j1939/transport.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 5757f9f..fad210e 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -716,10 +716,12 @@ static int j1939_session_tx_rts(struct j1939_session *session)
 		return ret;
 
 	session->last_txcmd = dat[0];
-	if (dat[0] == J1939_TP_CMD_BAM)
+	if (dat[0] == J1939_TP_CMD_BAM) {
 		j1939_tp_schedule_txtimer(session, 50);
-
-	j1939_tp_set_rxtimeout(session, 1250);
+		j1939_tp_set_rxtimeout(session, 250);
+	} else {
+		j1939_tp_set_rxtimeout(session, 1250);
+	}
 
 	netdev_dbg(session->priv->ndev, "%s: 0x%p\n", __func__, session);
 
@@ -1665,11 +1667,15 @@ static void j1939_xtp_rx_rts(struct j1939_priv *priv, struct sk_buff *skb,
 	}
 	session->last_cmd = cmd;
 
-	j1939_tp_set_rxtimeout(session, 1250);
-
-	if (cmd != J1939_TP_CMD_BAM && !session->transmission) {
-		j1939_session_txtimer_cancel(session);
-		j1939_tp_schedule_txtimer(session, 0);
+	if (cmd == J1939_TP_CMD_BAM) {
+		if (!session->transmission)
+			j1939_tp_set_rxtimeout(session, 750);
+	} else {
+		if (!session->transmission) {
+			j1939_session_txtimer_cancel(session);
+			j1939_tp_schedule_txtimer(session, 0);
+		}
+		j1939_tp_set_rxtimeout(session, 1250);
 	}
 
 	j1939_session_put(session);
@@ -1720,6 +1726,7 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session,
 	int offset;
 	int nbytes;
 	bool final = false;
+	bool remain = false;
 	bool do_cts_eoma = false;
 	int packet;
 
@@ -1781,6 +1788,8 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session,
 	    j1939_cb_is_broadcast(&session->skcb)) {
 		if (session->pkt.rx >= session->pkt.total)
 			final = true;
+		else
+			remain = true;
 	} else {
 		/* never final, an EOMA must follow */
 		if (session->pkt.rx >= session->pkt.last)
@@ -1790,6 +1799,9 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session,
 	if (final) {
 		j1939_session_timers_cancel(session);
 		j1939_session_completed(session);
+	} else if (remain) {
+		if (!session->transmission)
+			j1939_tp_set_rxtimeout(session, 750);
 	} else if (do_cts_eoma) {
 		j1939_tp_set_rxtimeout(session, 1250);
 		if (!session->transmission)
-- 
2.9.5

Re: [PATCH net 0/4] support multipacket broadcast message

[Oleksij Rempel]

[-- Attachment #1: Type: text/plain, Size: 1117 bytes --]

Hello,

Thank you for your patches! Currently I'm busy, but I'll take a look at it as
soon possible.

btw. can you tell me about more of your use case/work. I would like to
have some feedback about this stack. You can write a personal message,
if it is not for public.

On Wed, Aug 05, 2020 at 11:50:21AM +0800, Zhang Changzhong wrote:
> Zhang Changzhong (4):
>   can: j1939: fix support for multipacket broadcast message
>   can: j1939: cancel rxtimer on multipacket broadcast session complete
>   can: j1939: abort multipacket broadcast session when timeout occurs
>   can: j1939: add rxtimer for multipacket broadcast session
> 
>  net/can/j1939/transport.c | 48 +++++++++++++++++++++++++++++++++++------------
>  1 file changed, 36 insertions(+), 12 deletions(-)

Regards,
Oleksij

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net 0/4] support multipacket broadcast message

[Zhang Changzhong]

Hi Oleksij,

We have tested this j1939 stack according to SAE J1939-21. It works fine for
most cases, but when we test multipacket broadcast message function we found
the receiver can't receive those packets.

You can reproduce on CAN bus or vcan, for vcan case use cangw to connect vcan0
and vcan1:
sudo cangw -A -s vcan0 -d vcan1 -e
sudo cangw -A -s vcan1 -d vcan0 -e

To reproduce it use following commands:
testj1939 -B -r vcan1:0x90 &
testj1939 -B -s20 vcan0:0x80 :,0x12300

Besides, candump receives correct packets while testj1939 receives nothing.

Regards,
Zhang Changzhong

On 2020/8/7 0:10, Oleksij Rempel wrote:
> Hello,
> 
> Thank you for your patches! Currently I'm busy, but I'll take a look at it as
> soon possible.
> 
> btw. can you tell me about more of your use case/work. I would like to
> have some feedback about this stack. You can write a personal message,
> if it is not for public.
> 
> On Wed, Aug 05, 2020 at 11:50:21AM +0800, Zhang Changzhong wrote:
>> Zhang Changzhong (4):
>>   can: j1939: fix support for multipacket broadcast message
>>   can: j1939: cancel rxtimer on multipacket broadcast session complete
>>   can: j1939: abort multipacket broadcast session when timeout occurs
>>   can: j1939: add rxtimer for multipacket broadcast session
>>
>>  net/can/j1939/transport.c | 48 +++++++++++++++++++++++++++++++++++------------
>>  1 file changed, 36 insertions(+), 12 deletions(-)
> 
> Regards,
> Oleksij
> 

Re: [PATCH net 0/4] support multipacket broadcast message

[Oleksij Rempel]

[-- Attachment #1: Type: text/plain, Size: 1138 bytes --]

On Fri, Aug 07, 2020 at 05:36:38PM +0800, Zhang Changzhong wrote:
> Hi Oleksij,
> 
> We have tested this j1939 stack according to SAE J1939-21. It works fine for
> most cases, but when we test multipacket broadcast message function we found
> the receiver can't receive those packets.
> 
> You can reproduce on CAN bus or vcan, for vcan case use cangw to connect vcan0
> and vcan1:
> sudo cangw -A -s vcan0 -d vcan1 -e
> sudo cangw -A -s vcan1 -d vcan0 -e
> 
> To reproduce it use following commands:
> testj1939 -B -r vcan1:0x90 &
> testj1939 -B -s20 vcan0:0x80 :,0x12300
> 
> Besides, candump receives correct packets while testj1939 receives nothing.

Ok, thank you!

i'm able to reproduce it and added following test:
https://github.com/linux-can/can-tests/blob/master/j1939/j1939_ac_1k_bam_local0.sh

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net 0/4] support multipacket broadcast message

[Oleksij Rempel]

Hello,

On Wed, Aug 05, 2020 at 11:50:21AM +0800, Zhang Changzhong wrote:
> Zhang Changzhong (4):
>   can: j1939: fix support for multipacket broadcast message
>   can: j1939: cancel rxtimer on multipacket broadcast session complete
>   can: j1939: abort multipacket broadcast session when timeout occurs
>   can: j1939: add rxtimer for multipacket broadcast session
> 
>  net/can/j1939/transport.c | 48 +++++++++++++++++++++++++++++++++++------------
>  1 file changed, 36 insertions(+), 12 deletions(-)

Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

Thank you for your work!

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |