Re: [PATCH v1 01/10] j1939: rework j1939_send data path

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

 



On 11.12.18 11:57, Robin van der Gracht wrote:

On Wed,  5 Dec 2018 07:07:48 +0100
Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:


currently j1939_send is hard to read. For example:
- for TP/ETP - j1939_send->j1939_tp_send->j1939_send->j1939_send_one
- for for package - j1939_send->j1939_send->j1939_send_one

With this patch we should be able to see from the code, what path
we will go.

Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
---
  net/can/j1939/j1939-priv.h |  2 +-
  net/can/j1939/main.c       | 22 +----------------
  net/can/j1939/socket.c     | 13 +++++++++-
  net/can/j1939/transport.c  | 49 +++++++++++++++++++++++---------------
  4 files changed, 44 insertions(+), 42 deletions(-)

diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
index 43964071954c..30fb7e03b102 100644
--- a/net/can/j1939/j1939-priv.h
+++ b/net/can/j1939/j1939-priv.h
@@ -144,7 +144,7 @@ static inline struct j1939_sk_buff_cb *j1939_skb_to_cb(struct sk_buff *skb)
  	return (struct j1939_sk_buff_cb *)skb->cb;
  }
-int j1939_send(struct sk_buff *skb); 
+int j1939_send_one(struct j1939_priv *priv, struct sk_buff *skb);
  void j1939_sk_recv(struct sk_buff *skb);
/* stack entries */ 
diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index 29d277e76895..13ac2661625c 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -232,7 +232,7 @@ struct j1939_priv *j1939_priv_get_by_ndev(struct net_device *ndev)
  	return priv;
  }
-static int j1939_send_one(struct j1939_priv *priv, struct sk_buff *skb) 
+int j1939_send_one(struct j1939_priv *priv, struct sk_buff *skb)
  {
  	int ret, dlc;
  	canid_t canid;
@@ -276,26 +276,6 @@ static int j1939_send_one(struct j1939_priv *priv, struct sk_buff *skb)
  	return ret;
  }
-int j1939_send(struct sk_buff *skb) 
-{
-	struct j1939_priv *priv;
-	int ret;
-
-	priv = j1939_priv_get_by_ndev(skb->dev);
-	if (!priv)
-		return -EINVAL;
-
-	if (skb->len > 8)
-		/* re-route via transport protocol */
-		ret = j1939_tp_send(priv, skb);
-	else
-		ret = j1939_send_one(priv, skb);
-
-	j1939_priv_put(priv);
-
-	return ret;
-}
-
  static int j1939_netdev_notify(struct notifier_block *nb,
  			       unsigned long msg, void *data)
  {
diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
index 99dceda77601..d6332483350f 100644
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -617,6 +617,7 @@ static int j1939_sk_sendmsg(struct socket *sock, struct msghdr *msg,
  	struct j1939_sock *jsk = j1939_sk(sk);
  	struct sockaddr_can *addr = msg->msg_name;
  	struct j1939_sk_buff_cb *skcb;
+	struct j1939_priv *priv;
  	struct sk_buff *skb;
  	struct net_device *ndev;
  	int ifindex;
@@ -708,7 +709,17 @@ static int j1939_sk_sendmsg(struct socket *sock, struct msghdr *msg,
  		j1939_sock_pending_add(&jsk->sk);
  	}
- ret = j1939_send(skb); 
+	priv = j1939_priv_get_by_ndev(ndev);
+	if (!priv)
+		return -EINVAL;
+
+	if (skb->len > 8)
+		/* re-route via transport protocol */
+		ret = j1939_tp_send(priv, skb);
+	else
+		ret = j1939_send_one(priv, skb);
+
+	j1939_priv_put(priv);
  	if (ret < 0)
  		j1939_sock_pending_del(&jsk->sk);
diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c 
index ca51b670939d..92b3efb5f4c8 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -369,12 +369,14 @@ static struct sk_buff *j1939_tp_tx_dat_new(struct sk_buff *related,
  }
/* TP transmit packet functions */ 
-static int j1939_tp_tx_dat(struct sk_buff *related, bool extd,
+static int j1939_tp_tx_dat(struct j1939_session *session,
  			   const u8 *dat, int len)
  {
+	struct j1939_priv *priv = session->priv;


The kref counter is not touched if we acquire the priv struct
like this.
kref counter for priv is increased for each session. As long as session exist, priv will not be destroyed. 


We (still?) do that prior to, and post the j1939_send_one()
call in j1939_sk_sendmsg() which is contradicting?


i'm not sure what do you mean.


  	struct sk_buff *skb;
- skb = j1939_tp_tx_dat_new(related, extd, false, false); 
+	skb = j1939_tp_tx_dat_new(session->skb,
+				  session->extd, false, false);
  	if (IS_ERR(skb))
  		return PTR_ERR(skb);
@@ -382,10 +384,11 @@ static int j1939_tp_tx_dat(struct sk_buff *related, bool extd, 
  	if (j1939_tp_padding && len < 8)
  		memset(skb_put(skb, 8 - len), 0xff, 8 - len);
- return j1939_send(skb); 
+	return j1939_send_one(priv, skb);
  }
-static int j1939_xtp_do_tx_ctl(struct sk_buff *related, bool extd, 
+static int j1939_xtp_do_tx_ctl(struct j1939_priv *priv,
+			       struct sk_buff *related, bool extd,
  			       bool swap_src_dst, pgn_t pgn, const u8 *dat)
  {
  	struct sk_buff *skb;
@@ -404,18 +407,22 @@ static int j1939_xtp_do_tx_ctl(struct sk_buff *related, bool extd,
  	skdat[6] = (pgn >> 8);
  	skdat[7] = (pgn >> 16);
- return j1939_send(skb); 
+	return j1939_send_one(priv, skb);
  }
static inline int j1939_tp_tx_ctl(struct j1939_session *session, 
  				  bool swap_src_dst, const u8 *dat)
  {
-	return j1939_xtp_do_tx_ctl(session->skb, session->extd, swap_src_dst,
+	struct j1939_priv *priv = session->priv;
+
+	return j1939_xtp_do_tx_ctl(priv, session->skb, session->extd,
+				   swap_src_dst,
  				   session->skcb->addr.pgn, dat);
  }
-static int j1939_xtp_tx_abort(struct sk_buff *related, bool extd, 
-			      bool swap_src_dst, enum j1939_xtp_abort err,
+static int j1939_xtp_tx_abort(struct j1939_priv *priv, struct sk_buff *related,
+			      bool extd, bool swap_src_dst,
+			      enum j1939_xtp_abort err,
  			      pgn_t pgn)
  {
  	u8 dat[5];
@@ -429,7 +436,7 @@ static int j1939_xtp_tx_abort(struct sk_buff *related, bool extd,
  		dat[1] = J1939_XTP_ABORT_GENERIC;
  	else
  		dat[1] = err;
-	return j1939_xtp_do_tx_ctl(related, extd, swap_src_dst, pgn, dat);
+	return j1939_xtp_do_tx_ctl(priv, related, extd, swap_src_dst, pgn, dat);
  }
static inline void j1939_tp_schedule_txtimer(struct j1939_session *session, 
@@ -596,8 +603,7 @@ static int j1939_tp_txnext(struct j1939_session *session)
  			if (len > 7)
  				len = 7;
  			memcpy(&dat[1], &tpdat[offset], len);
-			ret = j1939_tp_tx_dat(session->skb, session->extd,
-					      dat, len + 1);
+			ret = j1939_tp_tx_dat(session, dat, len + 1);
  			if (ret < 0)
  				break;
  			session->last_txcmd = 0xff;
@@ -660,10 +666,12 @@ static void j1939_session_completed(struct j1939_session *session)
  static void j1939_session_cancel(struct j1939_session *session,
  				 enum j1939_xtp_abort err)
  {
+	struct j1939_priv *priv = session->priv;
+
  	/* do not send aborts on incoming broadcasts */
  	if (err && j1939_tp_im_involved_anydir(session->skb) &&
  	    !j1939_cb_is_broadcast(session->skcb))
-		j1939_xtp_tx_abort(session->skb, session->extd,
+		j1939_xtp_tx_abort(priv, session->skb, session->extd,
  				   !(session->skcb->src_flags & J1939_ECU_LOCAL),
  				   err, session->skcb->addr.pgn);
@@ -702,7 +710,7 @@ static void j1939_xtp_rx_bad_message_one(struct j1939_priv *priv, 
  		j1939_session_cancel(session, J1939_XTP_ABORT_FAULT);
  		goto out_session_put;
  	}
-	j1939_xtp_tx_abort(skb, extd, false, J1939_XTP_ABORT_FAULT, pgn);
+	j1939_xtp_tx_abort(priv, skb, extd, false, J1939_XTP_ABORT_FAULT, pgn);
  	if (!session)
  		return;
@@ -778,7 +786,8 @@ static void j1939_xtp_rx_eoma(struct j1939_priv *priv, struct sk_buff *skb, j1939_session_timers_cancel(session); 
  	if (session->skcb->addr.pgn != pgn) {
-		j1939_xtp_tx_abort(skb, extd, true, J1939_XTP_ABORT_BUSY, pgn);
+		j1939_xtp_tx_abort(priv, skb, extd, true, J1939_XTP_ABORT_BUSY,
+				   pgn);
  		j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
  	} else {
  		/* transmitted without problems */
@@ -807,7 +816,8 @@ static void j1939_xtp_rx_cts(struct j1939_priv *priv, struct sk_buff *skb,
if (session->skcb->addr.pgn != pgn) { 
  		/* what to do? */
-		j1939_xtp_tx_abort(skb, extd, true, J1939_XTP_ABORT_BUSY, pgn);
+		j1939_xtp_tx_abort(priv, skb, extd, true, J1939_XTP_ABORT_BUSY,
+				   pgn);
  		j1939_session_timers_cancel(session);
  		j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
  		goto out_session_put;
@@ -970,7 +980,7 @@ static void j1939_xtp_rx_rts(struct j1939_priv *priv, struct sk_buff *skb,
if (pgn != session->skcb->addr.pgn && 
  		    dat[0] != J1939_TP_CMD_BAM)
-			j1939_xtp_tx_abort(skb, extd, true,
+			j1939_xtp_tx_abort(priv, skb, extd, true,
  					   J1939_XTP_ABORT_BUSY, pgn);
goto out_session_put; 
@@ -1022,13 +1032,13 @@ static void j1939_xtp_rx_rts(struct j1939_priv *priv, struct sk_buff *skb,
  				abort = J1939_XTP_ABORT_RESOURCE;
  		}
  		if (abort) {
-			j1939_xtp_tx_abort(skb, extd, true, abort, pgn);
+			j1939_xtp_tx_abort(priv, skb, extd, true, abort, pgn);
  			return;
  		}
session = j1939_session_fresh_new(priv, len, skb, pgn); 
  		if (!session) {
-			j1939_xtp_tx_abort(skb, extd, true,
+			j1939_xtp_tx_abort(priv, skb, extd, true,
  					   J1939_XTP_ABORT_RESOURCE, pgn);
  			return;
  		}
@@ -1086,7 +1096,8 @@ static void j1939_xtp_rx_dpo(struct j1939_priv *priv, struct sk_buff *skb,
if (session->skcb->addr.pgn != pgn) { 
  		netdev_info(priv->ndev, "%s: different pgn\n", __func__);
-		j1939_xtp_tx_abort(skb, true, true, J1939_XTP_ABORT_BUSY, pgn);
+		j1939_xtp_tx_abort(priv, skb, true, true, J1939_XTP_ABORT_BUSY,
+				   pgn);
  		j1939_session_timers_cancel(session);
  		j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
  		goto out_session_put;
[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