[PATCH 0/2] net: qrtr: Nameserver fixes

[PATCH 0/2] net: qrtr: Nameserver fixes

[Bjorn Andersson]

The need to respond to the HELLO message from the firmware was lost in the
translation from the user space implementation of the nameserver. Fixing this
also means we can remove the FIXME related to launching the ns.

Bjorn Andersson (2):
  net: qrtr: Respond to HELLO message
  net: qrtr: Fix FIXME related to qrtr_ns_init()

 net/qrtr/ns.c   | 56 +++++++++++++++++++++++++++----------------------
 net/qrtr/qrtr.c |  6 +-----
 net/qrtr/qrtr.h |  2 +-
 3 files changed, 33 insertions(+), 31 deletions(-)

-- 
2.24.0

[PATCH 1/2] net: qrtr: Respond to HELLO message

[Bjorn Andersson]

Lost in the translation from the user space implementation was the
detail that HELLO mesages must be exchanged between each node pair.  As
such the incoming HELLO must be replied to.

Similar to the previous implementation no effort is made to prevent two
Linux boxes from continuously sending HELLO messages back and forth,
this is left to a follow up patch.

say_hello() is moved, to facilitate the new call site.

Fixes: 0c2204a4ad71 ("net: qrtr: Migrate nameservice to kernel from userspace")
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 net/qrtr/ns.c | 54 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
index 7bfde01f4e8a..e3f11052b5f6 100644
--- a/net/qrtr/ns.c
+++ b/net/qrtr/ns.c
@@ -286,9 +286,38 @@ static int server_del(struct qrtr_node *node, unsigned int port)
 	return 0;
 }
 
+static int say_hello(struct sockaddr_qrtr *dest)
+{
+	struct qrtr_ctrl_pkt pkt;
+	struct msghdr msg = { };
+	struct kvec iv;
+	int ret;
+
+	iv.iov_base = &pkt;
+	iv.iov_len = sizeof(pkt);
+
+	memset(&pkt, 0, sizeof(pkt));
+	pkt.cmd = cpu_to_le32(QRTR_TYPE_HELLO);
+
+	msg.msg_name = (struct sockaddr *)dest;
+	msg.msg_namelen = sizeof(*dest);
+
+	ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));
+	if (ret < 0)
+		pr_err("failed to send hello msg\n");
+
+	return ret;
+}
+
 /* Announce the list of servers registered on the local node */
 static int ctrl_cmd_hello(struct sockaddr_qrtr *sq)
 {
+	int ret;
+
+	ret = say_hello(sq);
+	if (ret < 0)
+		return ret;
+
 	return announce_servers(sq);
 }
 
@@ -566,29 +595,6 @@ static void ctrl_cmd_del_lookup(struct sockaddr_qrtr *from,
 	}
 }
 
-static int say_hello(void)
-{
-	struct qrtr_ctrl_pkt pkt;
-	struct msghdr msg = { };
-	struct kvec iv;
-	int ret;
-
-	iv.iov_base = &pkt;
-	iv.iov_len = sizeof(pkt);
-
-	memset(&pkt, 0, sizeof(pkt));
-	pkt.cmd = cpu_to_le32(QRTR_TYPE_HELLO);
-
-	msg.msg_name = (struct sockaddr *)&qrtr_ns.bcast_sq;
-	msg.msg_namelen = sizeof(qrtr_ns.bcast_sq);
-
-	ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));
-	if (ret < 0)
-		pr_err("failed to send hello msg\n");
-
-	return ret;
-}
-
 static void qrtr_ns_worker(struct work_struct *work)
 {
 	const struct qrtr_ctrl_pkt *pkt;
@@ -725,7 +731,7 @@ void qrtr_ns_init(struct work_struct *work)
 	if (!qrtr_ns.workqueue)
 		goto err_sock;
 
-	ret = say_hello();
+	ret = say_hello(&qrtr_ns.bcast_sq);
 	if (ret < 0)
 		goto err_wq;
 
-- 
2.24.0

[PATCH 2/2] net: qrtr: Fix FIXME related to qrtr_ns_init()

[Bjorn Andersson]

The 2 second delay before calling qrtr_ns_init() meant that the remote
processors would register as endpoints in qrtr and the say_hello() call
would therefor broadcast the outgoing HELLO to them. With the HELLO
handshake corrected this delay is no longer needed.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 net/qrtr/ns.c   | 2 +-
 net/qrtr/qrtr.c | 6 +-----
 net/qrtr/qrtr.h | 2 +-
 3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
index e3f11052b5f6..cfd4bd07a62b 100644
--- a/net/qrtr/ns.c
+++ b/net/qrtr/ns.c
@@ -693,7 +693,7 @@ static void qrtr_ns_data_ready(struct sock *sk)
 	queue_work(qrtr_ns.workqueue, &qrtr_ns.work);
 }
 
-void qrtr_ns_init(struct work_struct *work)
+void qrtr_ns_init(void)
 {
 	struct sockaddr_qrtr sq;
 	int ret;
diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
index 423310896285..313d3194018a 100644
--- a/net/qrtr/qrtr.c
+++ b/net/qrtr/qrtr.c
@@ -1263,11 +1263,7 @@ static int __init qrtr_proto_init(void)
 		return rc;
 	}
 
-	/* FIXME: Currently, this 2s delay is required to catch the NEW_SERVER
-	 * messages from routers. But the fix could be somewhere else.
-	 */
-	INIT_DELAYED_WORK(&qrtr_ns_work, qrtr_ns_init);
-	schedule_delayed_work(&qrtr_ns_work, msecs_to_jiffies(2000));
+	qrtr_ns_init();
 
 	return rc;
 }
diff --git a/net/qrtr/qrtr.h b/net/qrtr/qrtr.h
index 53a237a28971..dc2b67f17927 100644
--- a/net/qrtr/qrtr.h
+++ b/net/qrtr/qrtr.h
@@ -29,7 +29,7 @@ void qrtr_endpoint_unregister(struct qrtr_endpoint *ep);
 
 int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len);
 
-void qrtr_ns_init(struct work_struct *work);
+void qrtr_ns_init(void);
 
 void qrtr_ns_remove(void);
 
-- 
2.24.0

Re: [PATCH 1/2] net: qrtr: Respond to HELLO message

[Manivannan Sadhasivam]

Hi Bjorn,

Thanks for the fix. I have tested this and it works perfectly!

On Sun, Mar 01, 2020 at 07:25:26PM -0800, Bjorn Andersson wrote:
> Lost in the translation from the user space implementation was the
> detail that HELLO mesages must be exchanged between each node pair.  As
> such the incoming HELLO must be replied to.
> 

Err. I thought the say_hello() part in ctrl_cmd_hello() was redundant, so
removed it :P

Sorry for that.

> Similar to the previous implementation no effort is made to prevent two
> Linux boxes from continuously sending HELLO messages back and forth,
> this is left to a follow up patch.
> 
> say_hello() is moved, to facilitate the new call site.
> 
> Fixes: 0c2204a4ad71 ("net: qrtr: Migrate nameservice to kernel from userspace")
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  net/qrtr/ns.c | 54 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 30 insertions(+), 24 deletions(-)
> 
> diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> index 7bfde01f4e8a..e3f11052b5f6 100644
> --- a/net/qrtr/ns.c
> +++ b/net/qrtr/ns.c
> @@ -286,9 +286,38 @@ static int server_del(struct qrtr_node *node, unsigned int port)
>  	return 0;
>  }
>  
> +static int say_hello(struct sockaddr_qrtr *dest)
> +{
> +	struct qrtr_ctrl_pkt pkt;
> +	struct msghdr msg = { };
> +	struct kvec iv;
> +	int ret;
> +
> +	iv.iov_base = &pkt;
> +	iv.iov_len = sizeof(pkt);
> +
> +	memset(&pkt, 0, sizeof(pkt));
> +	pkt.cmd = cpu_to_le32(QRTR_TYPE_HELLO);
> +
> +	msg.msg_name = (struct sockaddr *)dest;
> +	msg.msg_namelen = sizeof(*dest);
> +
> +	ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));
> +	if (ret < 0)
> +		pr_err("failed to send hello msg\n");
> +
> +	return ret;
> +}
> +
>  /* Announce the list of servers registered on the local node */
>  static int ctrl_cmd_hello(struct sockaddr_qrtr *sq)
>  {
> +	int ret;
> +
> +	ret = say_hello(sq);
> +	if (ret < 0)
> +		return ret;
> +
>  	return announce_servers(sq);
>  }
>  
> @@ -566,29 +595,6 @@ static void ctrl_cmd_del_lookup(struct sockaddr_qrtr *from,
>  	}
>  }
>  
> -static int say_hello(void)
> -{
> -	struct qrtr_ctrl_pkt pkt;
> -	struct msghdr msg = { };
> -	struct kvec iv;
> -	int ret;
> -
> -	iv.iov_base = &pkt;
> -	iv.iov_len = sizeof(pkt);
> -
> -	memset(&pkt, 0, sizeof(pkt));
> -	pkt.cmd = cpu_to_le32(QRTR_TYPE_HELLO);
> -
> -	msg.msg_name = (struct sockaddr *)&qrtr_ns.bcast_sq;
> -	msg.msg_namelen = sizeof(qrtr_ns.bcast_sq);
> -
> -	ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));
> -	if (ret < 0)
> -		pr_err("failed to send hello msg\n");
> -
> -	return ret;
> -}
> -
>  static void qrtr_ns_worker(struct work_struct *work)
>  {
>  	const struct qrtr_ctrl_pkt *pkt;
> @@ -725,7 +731,7 @@ void qrtr_ns_init(struct work_struct *work)
>  	if (!qrtr_ns.workqueue)
>  		goto err_sock;
>  
> -	ret = say_hello();
> +	ret = say_hello(&qrtr_ns.bcast_sq);

Why do you want to pass a global variable here? Why can't it be used directly
in say_hello() as done before?

Other than that,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

>  	if (ret < 0)
>  		goto err_wq;
>  
> -- 
> 2.24.0
> 

Re: [PATCH 2/2] net: qrtr: Fix FIXME related to qrtr_ns_init()

[Manivannan Sadhasivam]

On Sun, Mar 01, 2020 at 07:25:27PM -0800, Bjorn Andersson wrote:
> The 2 second delay before calling qrtr_ns_init() meant that the remote
> processors would register as endpoints in qrtr and the say_hello() call
> would therefor broadcast the outgoing HELLO to them. With the HELLO
> handshake corrected this delay is no longer needed.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  net/qrtr/ns.c   | 2 +-
>  net/qrtr/qrtr.c | 6 +-----
>  net/qrtr/qrtr.h | 2 +-
>  3 files changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> index e3f11052b5f6..cfd4bd07a62b 100644
> --- a/net/qrtr/ns.c
> +++ b/net/qrtr/ns.c
> @@ -693,7 +693,7 @@ static void qrtr_ns_data_ready(struct sock *sk)
>  	queue_work(qrtr_ns.workqueue, &qrtr_ns.work);
>  }
>  
> -void qrtr_ns_init(struct work_struct *work)
> +void qrtr_ns_init(void)
>  {
>  	struct sockaddr_qrtr sq;
>  	int ret;
> diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
> index 423310896285..313d3194018a 100644
> --- a/net/qrtr/qrtr.c
> +++ b/net/qrtr/qrtr.c
> @@ -1263,11 +1263,7 @@ static int __init qrtr_proto_init(void)
>  		return rc;
>  	}
>  
> -	/* FIXME: Currently, this 2s delay is required to catch the NEW_SERVER
> -	 * messages from routers. But the fix could be somewhere else.
> -	 */
> -	INIT_DELAYED_WORK(&qrtr_ns_work, qrtr_ns_init);
> -	schedule_delayed_work(&qrtr_ns_work, msecs_to_jiffies(2000));
> +	qrtr_ns_init();
>  

You forgot to remove the below instances of delayed_work:

#include <linux/workqueue.h>
struct delayed_work qrtr_ns_work;
cancel_delayed_work_sync(&qrtr_ns_work);

Other than that,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

>  	return rc;
>  }
> diff --git a/net/qrtr/qrtr.h b/net/qrtr/qrtr.h
> index 53a237a28971..dc2b67f17927 100644
> --- a/net/qrtr/qrtr.h
> +++ b/net/qrtr/qrtr.h
> @@ -29,7 +29,7 @@ void qrtr_endpoint_unregister(struct qrtr_endpoint *ep);
>  
>  int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len);
>  
> -void qrtr_ns_init(struct work_struct *work);
> +void qrtr_ns_init(void);
>  
>  void qrtr_ns_remove(void);
>  
> -- 
> 2.24.0
> 

Re: [PATCH 1/2] net: qrtr: Respond to HELLO message

[Bjorn Andersson]

On Sun 01 Mar 21:50 PST 2020, Manivannan Sadhasivam wrote:

> Hi Bjorn,
> 
> Thanks for the fix. I have tested this and it works perfectly!
> 
> On Sun, Mar 01, 2020 at 07:25:26PM -0800, Bjorn Andersson wrote:
> > Lost in the translation from the user space implementation was the
> > detail that HELLO mesages must be exchanged between each node pair.  As
> > such the incoming HELLO must be replied to.
> > 
> 
> Err. I thought the say_hello() part in ctrl_cmd_hello() was redundant, so
> removed it :P
> 
> Sorry for that.
> 

No worries.

> > Similar to the previous implementation no effort is made to prevent two
> > Linux boxes from continuously sending HELLO messages back and forth,
> > this is left to a follow up patch.
> > 
> > say_hello() is moved, to facilitate the new call site.
> > 
> > Fixes: 0c2204a4ad71 ("net: qrtr: Migrate nameservice to kernel from userspace")
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  net/qrtr/ns.c | 54 ++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 30 insertions(+), 24 deletions(-)
> > 
> > diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> > index 7bfde01f4e8a..e3f11052b5f6 100644
> > --- a/net/qrtr/ns.c
> > +++ b/net/qrtr/ns.c
> > @@ -286,9 +286,38 @@ static int server_del(struct qrtr_node *node, unsigned int port)
> >  	return 0;
> >  }
> >  
> > +static int say_hello(struct sockaddr_qrtr *dest)
> > +{
> > +	struct qrtr_ctrl_pkt pkt;
> > +	struct msghdr msg = { };
> > +	struct kvec iv;
> > +	int ret;
> > +
> > +	iv.iov_base = &pkt;
> > +	iv.iov_len = sizeof(pkt);
> > +
> > +	memset(&pkt, 0, sizeof(pkt));
> > +	pkt.cmd = cpu_to_le32(QRTR_TYPE_HELLO);
> > +
> > +	msg.msg_name = (struct sockaddr *)dest;
> > +	msg.msg_namelen = sizeof(*dest);
> > +
> > +	ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));
> > +	if (ret < 0)
> > +		pr_err("failed to send hello msg\n");
> > +
> > +	return ret;
> > +}
> > +
> >  /* Announce the list of servers registered on the local node */
> >  static int ctrl_cmd_hello(struct sockaddr_qrtr *sq)
> >  {
> > +	int ret;
> > +
> > +	ret = say_hello(sq); > > +	if (ret < 0)
> > +		return ret;
> > +
> >  	return announce_servers(sq);
> >  }
> >  
> > @@ -566,29 +595,6 @@ static void ctrl_cmd_del_lookup(struct sockaddr_qrtr *from,
> >  	}
> >  }
> >  
> > -static int say_hello(void)
> > -{
> > -	struct qrtr_ctrl_pkt pkt;
> > -	struct msghdr msg = { };
> > -	struct kvec iv;
> > -	int ret;
> > -
> > -	iv.iov_base = &pkt;
> > -	iv.iov_len = sizeof(pkt);
> > -
> > -	memset(&pkt, 0, sizeof(pkt));
> > -	pkt.cmd = cpu_to_le32(QRTR_TYPE_HELLO);
> > -
> > -	msg.msg_name = (struct sockaddr *)&qrtr_ns.bcast_sq;
> > -	msg.msg_namelen = sizeof(qrtr_ns.bcast_sq);
> > -
> > -	ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));
> > -	if (ret < 0)
> > -		pr_err("failed to send hello msg\n");
> > -
> > -	return ret;
> > -}
> > -
> >  static void qrtr_ns_worker(struct work_struct *work)
> >  {
> >  	const struct qrtr_ctrl_pkt *pkt;
> > @@ -725,7 +731,7 @@ void qrtr_ns_init(struct work_struct *work)
> >  	if (!qrtr_ns.workqueue)
> >  		goto err_sock;
> >  
> > -	ret = say_hello();
> > +	ret = say_hello(&qrtr_ns.bcast_sq);
> 
> Why do you want to pass a global variable here? Why can't it be used directly
> in say_hello() as done before?
> 

Because I changed the prototype of say_hello() so that we pass the
destination address; here that's the broadcast address, in
ctrl_cmd_hello() it's the specific sender of the incoming hello that we
want to respond to.

Regards,
Bjorn

> Other than that,
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Thanks,
> Mani
> 
> >  	if (ret < 0)
> >  		goto err_wq;
> >  
> > -- 
> > 2.24.0
> > 

Re: [PATCH 2/2] net: qrtr: Fix FIXME related to qrtr_ns_init()

[Bjorn Andersson]

On Sun 01 Mar 21:55 PST 2020, Manivannan Sadhasivam wrote:

> On Sun, Mar 01, 2020 at 07:25:27PM -0800, Bjorn Andersson wrote:
> > The 2 second delay before calling qrtr_ns_init() meant that the remote
> > processors would register as endpoints in qrtr and the say_hello() call
> > would therefor broadcast the outgoing HELLO to them. With the HELLO
> > handshake corrected this delay is no longer needed.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  net/qrtr/ns.c   | 2 +-
> >  net/qrtr/qrtr.c | 6 +-----
> >  net/qrtr/qrtr.h | 2 +-
> >  3 files changed, 3 insertions(+), 7 deletions(-)
> > 
> > diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> > index e3f11052b5f6..cfd4bd07a62b 100644
> > --- a/net/qrtr/ns.c
> > +++ b/net/qrtr/ns.c
> > @@ -693,7 +693,7 @@ static void qrtr_ns_data_ready(struct sock *sk)
> >  	queue_work(qrtr_ns.workqueue, &qrtr_ns.work);
> >  }
> >  
> > -void qrtr_ns_init(struct work_struct *work)
> > +void qrtr_ns_init(void)
> >  {
> >  	struct sockaddr_qrtr sq;
> >  	int ret;
> > diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
> > index 423310896285..313d3194018a 100644
> > --- a/net/qrtr/qrtr.c
> > +++ b/net/qrtr/qrtr.c
> > @@ -1263,11 +1263,7 @@ static int __init qrtr_proto_init(void)
> >  		return rc;
> >  	}
> >  
> > -	/* FIXME: Currently, this 2s delay is required to catch the NEW_SERVER
> > -	 * messages from routers. But the fix could be somewhere else.
> > -	 */
> > -	INIT_DELAYED_WORK(&qrtr_ns_work, qrtr_ns_init);
> > -	schedule_delayed_work(&qrtr_ns_work, msecs_to_jiffies(2000));
> > +	qrtr_ns_init();
> >  
> 
> You forgot to remove the below instances of delayed_work:
> 
> #include <linux/workqueue.h>
> struct delayed_work qrtr_ns_work;
> cancel_delayed_work_sync(&qrtr_ns_work);
> 

I sure did, thanks for noticing.

Regards,
Bjorn

> Other than that,
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Thanks,
> Mani
> 
> >  	return rc;
> >  }
> > diff --git a/net/qrtr/qrtr.h b/net/qrtr/qrtr.h
> > index 53a237a28971..dc2b67f17927 100644
> > --- a/net/qrtr/qrtr.h
> > +++ b/net/qrtr/qrtr.h
> > @@ -29,7 +29,7 @@ void qrtr_endpoint_unregister(struct qrtr_endpoint *ep);
> >  
> >  int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len);
> >  
> > -void qrtr_ns_init(struct work_struct *work);
> > +void qrtr_ns_init(void);
> >  
> >  void qrtr_ns_remove(void);
> >  
> > -- 
> > 2.24.0
> > 

Re: [PATCH 1/2] net: qrtr: Respond to HELLO message

[Manivannan Sadhasivam]

On Sun, Mar 01, 2020 at 10:55:02PM -0800, Bjorn Andersson wrote:
> On Sun 01 Mar 21:50 PST 2020, Manivannan Sadhasivam wrote:
> 
> > Hi Bjorn,
> > 
> > Thanks for the fix. I have tested this and it works perfectly!
> > 
> > On Sun, Mar 01, 2020 at 07:25:26PM -0800, Bjorn Andersson wrote:
> > > Lost in the translation from the user space implementation was the
> > > detail that HELLO mesages must be exchanged between each node pair.  As
> > > such the incoming HELLO must be replied to.
> > > 
> > 
> > Err. I thought the say_hello() part in ctrl_cmd_hello() was redundant, so
> > removed it :P
> > 
> > Sorry for that.
> > 
> 
> No worries.
> 
> > > Similar to the previous implementation no effort is made to prevent two
> > > Linux boxes from continuously sending HELLO messages back and forth,
> > > this is left to a follow up patch.
> > > 
> > > say_hello() is moved, to facilitate the new call site.
> > > 
> > > Fixes: 0c2204a4ad71 ("net: qrtr: Migrate nameservice to kernel from userspace")
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > ---
> > >  net/qrtr/ns.c | 54 ++++++++++++++++++++++++++++-----------------------
> > >  1 file changed, 30 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> > > index 7bfde01f4e8a..e3f11052b5f6 100644
> > > --- a/net/qrtr/ns.c
> > > +++ b/net/qrtr/ns.c
> > > @@ -286,9 +286,38 @@ static int server_del(struct qrtr_node *node, unsigned int port)
> > >  	return 0;
> > >  }
> > >  
> > > +static int say_hello(struct sockaddr_qrtr *dest)
> > > +{
> > > +	struct qrtr_ctrl_pkt pkt;
> > > +	struct msghdr msg = { };
> > > +	struct kvec iv;
> > > +	int ret;
> > > +
> > > +	iv.iov_base = &pkt;
> > > +	iv.iov_len = sizeof(pkt);
> > > +
> > > +	memset(&pkt, 0, sizeof(pkt));
> > > +	pkt.cmd = cpu_to_le32(QRTR_TYPE_HELLO);
> > > +
> > > +	msg.msg_name = (struct sockaddr *)dest;
> > > +	msg.msg_namelen = sizeof(*dest);
> > > +
> > > +	ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));
> > > +	if (ret < 0)
> > > +		pr_err("failed to send hello msg\n");
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >  /* Announce the list of servers registered on the local node */
> > >  static int ctrl_cmd_hello(struct sockaddr_qrtr *sq)
> > >  {
> > > +	int ret;
> > > +
> > > +	ret = say_hello(sq); > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > >  	return announce_servers(sq);
> > >  }
> > >  
> > > @@ -566,29 +595,6 @@ static void ctrl_cmd_del_lookup(struct sockaddr_qrtr *from,
> > >  	}
> > >  }
> > >  
> > > -static int say_hello(void)
> > > -{
> > > -	struct qrtr_ctrl_pkt pkt;
> > > -	struct msghdr msg = { };
> > > -	struct kvec iv;
> > > -	int ret;
> > > -
> > > -	iv.iov_base = &pkt;
> > > -	iv.iov_len = sizeof(pkt);
> > > -
> > > -	memset(&pkt, 0, sizeof(pkt));
> > > -	pkt.cmd = cpu_to_le32(QRTR_TYPE_HELLO);
> > > -
> > > -	msg.msg_name = (struct sockaddr *)&qrtr_ns.bcast_sq;
> > > -	msg.msg_namelen = sizeof(qrtr_ns.bcast_sq);
> > > -
> > > -	ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));
> > > -	if (ret < 0)
> > > -		pr_err("failed to send hello msg\n");
> > > -
> > > -	return ret;
> > > -}
> > > -
> > >  static void qrtr_ns_worker(struct work_struct *work)
> > >  {
> > >  	const struct qrtr_ctrl_pkt *pkt;
> > > @@ -725,7 +731,7 @@ void qrtr_ns_init(struct work_struct *work)
> > >  	if (!qrtr_ns.workqueue)
> > >  		goto err_sock;
> > >  
> > > -	ret = say_hello();
> > > +	ret = say_hello(&qrtr_ns.bcast_sq);
> > 
> > Why do you want to pass a global variable here? Why can't it be used directly
> > in say_hello() as done before?
> > 
> 
> Because I changed the prototype of say_hello() so that we pass the
> destination address; here that's the broadcast address, in
> ctrl_cmd_hello() it's the specific sender of the incoming hello that we
> want to respond to.
> 

Ah, yes. I missed that. Sounds good to me.

Thanks,
Mani

> Regards,
> Bjorn
> 
> > Other than that,
> > 
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > Thanks,
> > Mani
> > 
> > >  	if (ret < 0)
> > >  		goto err_wq;
> > >  
> > > -- 
> > > 2.24.0
> > >