On Wed, 5 Dec 2018 07:07:52 +0100
Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:
> and move part of j1939_xtp_rx_rts() to it.
>
> Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> ---
> net/can/j1939/transport.c | 87 +++++++++++++++++++++++----------------
> 1 file changed, 51 insertions(+), 36 deletions(-)
>
> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 0aade65c5250..2ccb1f448d91 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -953,11 +953,20 @@ struct j1939_session *j1939_xtp_rx_rts_new(struct j1939_priv *priv,
> struct sk_buff *skb, bool extd)
> {
> enum j1939_xtp_abort abort = J1939_XTP_ABORT_NO_ERROR;
> + struct j1939_sk_buff_cb *skcb = j1939_skb_to_cb(skb);
> struct j1939_session *session;
> const u8 *dat;
> pgn_t pgn;
> int len;
>
> + if (j1939_tp_im_transmitter(skb)) {
> + netdev_alert(priv->ndev, "%s: I should tx (%i %02x %02x)\n",
> + __func__, can_skb_prv(skb)->ifindex,
> + skcb->addr.sa, skcb->addr.da);
> +
> + return NULL;
> + }
> +
> dat = skb->data;
> pgn = j1939_xtp_ctl_to_pgn(dat);
>
> @@ -1009,30 +1018,18 @@ struct j1939_session *j1939_xtp_rx_rts_new(struct j1939_priv *priv,
> return session;
> }
>
> -static void j1939_xtp_rx_rts(struct j1939_priv *priv, struct sk_buff *skb,
> - bool extd)
> +static int j1939_xtp_rx_rts_old(struct j1939_session *session,
> + struct sk_buff *skb, bool extd)
Maybe it's nothing, but to me it feels weird calling an ongoing/active
tp session 'old' :P
Maybe we can use ('first' and) 'next' instead of ('new' and) 'old'?
> {
> struct j1939_sk_buff_cb *skcb = j1939_skb_to_cb(skb);
> - struct j1939_session *session;
> + struct j1939_priv *priv = session->priv;
> const u8 *dat;
> pgn_t pgn;
>
> dat = skb->data;
> pgn = j1939_xtp_ctl_to_pgn(dat);
>
> - if (dat[0] == J1939_TP_CMD_RTS && j1939_cb_is_broadcast(skcb)) {
> - netdev_alert(priv->ndev, "%s: rts without destination (%i %02x)\n",
> - __func__, can_skb_prv(skb)->ifindex,
> - skcb->addr.sa);
> - return;
> - }
> -
> - /* TODO: abort RTS when a similar
> - * TP is pending in the other direction
> - */
> - session = j1939_session_get_by_skb(priv, j1939_sessionq(priv, extd),
> - skb, false);
> - if (session && !j1939_tp_im_transmitter(skb)) {
> + if (!j1939_tp_im_transmitter(skb)) {
> /* RTS on pending connection */
> j1939_session_timers_cancel(session);
> j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
> @@ -1042,16 +1039,10 @@ static void j1939_xtp_rx_rts(struct j1939_priv *priv, struct sk_buff *skb,
> j1939_xtp_tx_abort(priv, skb, extd, true,
> J1939_XTP_ABORT_BUSY, pgn);
>
> - goto out_session_put;
> - } else if (!session && j1939_tp_im_transmitter(skb)) {
> - netdev_alert(priv->ndev, "%s: I should tx (%i %02x %02x)\n",
> - __func__, can_skb_prv(skb)->ifindex,
> - skcb->addr.sa, skcb->addr.da);
> -
> - return;
> + return -EBUSY;
> }
>
> - if (session && session->last_cmd != 0) {
> + if (session->last_cmd != 0) {
> /* we received a second rts on the same connection */
> netdev_alert(priv->ndev, "%s: connection exists (%i %02x %02x)\n",
> __func__, can_skb_prv(skb)->ifindex, skcb->addr.sa,
> @@ -1060,16 +1051,45 @@ static void j1939_xtp_rx_rts(struct j1939_priv *priv, struct sk_buff *skb,
> j1939_session_timers_cancel(session);
> j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
>
> - goto out_session_put;
> + return -EBUSY;
> }
>
> + /* make sure 'sa' & 'da' are correct !
> + * They may be 'not filled in yet' for sending
> + * skb's, since they did not pass the Address Claim ever.
> + */
> + session->skcb->addr.sa = skcb->addr.sa;
> + session->skcb->addr.da = skcb->addr.da;
> +
> + return 0;
> +}
> +
> +static void j1939_xtp_rx_rts(struct j1939_priv *priv, struct sk_buff *skb,
> + bool extd)
> +{
> + struct j1939_sk_buff_cb *skcb = j1939_skb_to_cb(skb);
> + struct j1939_session *session;
> + const u8 *dat;
> + pgn_t pgn;
> +
> + dat = skb->data;
> + pgn = j1939_xtp_ctl_to_pgn(dat);
> +
> + if (dat[0] == J1939_TP_CMD_RTS && j1939_cb_is_broadcast(skcb)) {
> + netdev_alert(priv->ndev, "%s: rts without destination (%i %02x)\n",
> + __func__, can_skb_prv(skb)->ifindex,
> + skcb->addr.sa);
> + return;
> + }
> +
> + /* TODO: abort RTS when a similar
> + * TP is pending in the other direction
> + */
> + session = j1939_session_get_by_skb(priv, j1939_sessionq(priv, extd),
> + skb, false);
> if (session) {
> - /* make sure 'sa' & 'da' are correct !
> - * They may be 'not filled in yet' for sending
> - * skb's, since they did not pass the Address Claim ever.
> - */
> - session->skcb->addr.sa = skcb->addr.sa;
> - session->skcb->addr.da = skcb->addr.da;
> + if (j1939_xtp_rx_rts_old(session, skb, extd))
> + goto out_session_put;
> } else {
> session = j1939_xtp_rx_rts_new(priv, skb, extd);
> if (!session)
> @@ -1084,11 +1104,6 @@ static void j1939_xtp_rx_rts(struct j1939_priv *priv, struct sk_buff *skb,
> j1939_tp_schedule_txtimer(session, 0);
> }
>
> - /* as soon as it's inserted, things can go fast
> - * protect against a long delay
> - * between spin_unlock & next statement
> - * so, only release here, at the end
> - */
> out_session_put:
> j1939_session_put(session);
> }
--
Robin van der Gracht
Protonic Holland
tel.: +31 (0) 229 212928
fax.: +31 (0) 229 210930
Factorij 36 / 1689 AL Zwaag
