On Tue, Dec 11, 2018 at 01:38:20PM +0100, Robin van der Gracht wrote:
> 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.
ok, thx! i'll do.
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: PGP signature
