On Tue, 11 Dec 2018 12:59:59 +0100
Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:
> 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.
Prior to this commit (in j1939_send()) we called:
j1939_send->j1939_priv_get_by_ndev->j1939_priv_get->kref_get()
j1939_send->j1939_send_one()
j1939_send->j1939_priv_put->kref_put()
and now it's just:
j1939_send_one()
So the kref counting has been removed.
But in j1939_sk_sendmsg() we still do:
j1939_send->j1939_priv_get_by_ndev->j1939_priv_get->kref_get()
j1939_send->j1939_send_one()
j1939_send->j1939_priv_put->kref_put()
But while typing this, I realized that the you probably removed all
the nested kref get/put calls since they are all enclosed in the
j1939_sk_sendmsg() kref get/put.
But because the commit message only mentions improvements in
readability, I wanted to make sure the kref put/get wasn't
accidentally left out.
Kind regards,
Robin
