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. We (still?) do that prior to, and post the j1939_send_one()
call in j1939_sk_sendmsg() which is contradicting?
> 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;
--
Robin van der Gracht
Protonic Holland
tel.: +31 (0) 229 212928
fax.: +31 (0) 229 210930
Factorij 36 / 1689 AL Zwaag
