[PATCH v2 0/5] net: qrtr: Add distant node support

[PATCH v2 0/5] net: qrtr: Add distant node support

[Loic Poulain]

QRTR protocol allows a node to communicate with an other non-immediate 
node via an intermdediate immediate node acting as a 'bridge':

node-0 <=> node-1 <=> node-2

This is currently not supported in this upstream version and this
series aim to fix that.

This series is V2 because changes 1, 2 and 3 have already been submitted
separately on LKML.

Changes in v2:
- Add reviewed-by tags from Bjorn and Mani
- Fixing checkpatch issue reported by Jakub

Loic Poulain (5):
  net: qrtr: Fix port ID for control messages
  net: qrtr: Allow forwarded services
  net: qrtr: Allow non-immediate node routing
  net: qrtr: Add GFP flags parameter to qrtr_alloc_ctrl_packet
  net: qrtr: Release distant nodes along the bridge node

 net/qrtr/ns.c   |  8 --------
 net/qrtr/qrtr.c | 51 ++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 38 insertions(+), 21 deletions(-)

-- 
2.7.4

[PATCH v2 1/5] net: qrtr: Fix port ID for control messages

[Loic Poulain]

The port ID for control messages was uncorrectly set with broadcast
node ID value, causing message to be dropped on remote side since
not passing packet filtering (cb->dst_port != QRTR_PORT_CTRL).

Fixes: d27e77a3de28 ("net: qrtr: Reset the node and port ID of broadcast messages")
Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 net/qrtr/qrtr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
index b4c0db0..e09154b 100644
--- a/net/qrtr/qrtr.c
+++ b/net/qrtr/qrtr.c
@@ -348,7 +348,7 @@ static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
 	hdr->src_port_id = cpu_to_le32(from->sq_port);
 	if (to->sq_port == QRTR_PORT_CTRL) {
 		hdr->dst_node_id = cpu_to_le32(node->nid);
-		hdr->dst_port_id = cpu_to_le32(QRTR_NODE_BCAST);
+		hdr->dst_port_id = cpu_to_le32(QRTR_PORT_CTRL);
 	} else {
 		hdr->dst_node_id = cpu_to_le32(to->sq_node);
 		hdr->dst_port_id = cpu_to_le32(to->sq_port);
-- 
2.7.4

[PATCH v2 2/5] net: qrtr: Allow forwarded services

[Loic Poulain]

A remote endpoint (immediate neighbors node) can forward services
from other nodes (non-immadiate), in that case ctrl packet node ID
(offering distant service) can differ from the qrtr source node
(forwarding the packet).

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 net/qrtr/ns.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
index d8252fd..d542d8f 100644
--- a/net/qrtr/ns.c
+++ b/net/qrtr/ns.c
@@ -469,10 +469,6 @@ static int ctrl_cmd_new_server(struct sockaddr_qrtr *from,
 		port = from->sq_port;
 	}
 
-	/* Don't accept spoofed messages */
-	if (from->sq_node != node_id)
-		return -EINVAL;
-
 	srv = server_add(service, instance, node_id, port);
 	if (!srv)
 		return -EINVAL;
@@ -511,10 +507,6 @@ static int ctrl_cmd_del_server(struct sockaddr_qrtr *from,
 		port = from->sq_port;
 	}
 
-	/* Don't accept spoofed messages */
-	if (from->sq_node != node_id)
-		return -EINVAL;
-
 	/* Local servers may only unregister themselves */
 	if (from->sq_node == qrtr_ns.local_node && from->sq_port != port)
 		return -EINVAL;
-- 
2.7.4

[PATCH v2 3/5] net: qrtr: Allow non-immediate node routing

[Loic Poulain]

In order to reach non-immediate remote node services that are
accessed through an intermediate node, the route to the remote
node needs to be saved.

E.g for a [node1 <=> node2 <=> node3] network
- node2 forwards node3 service to node1
- node1 must save node2 as route for reaching node3

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 net/qrtr/qrtr.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
index e09154b..1d12408 100644
--- a/net/qrtr/qrtr.c
+++ b/net/qrtr/qrtr.c
@@ -400,12 +400,13 @@ static void qrtr_node_assign(struct qrtr_node *node, unsigned int nid)
 {
 	unsigned long flags;
 
-	if (node->nid != QRTR_EP_NID_AUTO || nid == QRTR_EP_NID_AUTO)
+	if (nid == QRTR_EP_NID_AUTO)
 		return;
 
 	spin_lock_irqsave(&qrtr_nodes_lock, flags);
 	radix_tree_insert(&qrtr_nodes, nid, node);
-	node->nid = nid;
+	if (node->nid == QRTR_EP_NID_AUTO)
+		node->nid = nid;
 	spin_unlock_irqrestore(&qrtr_nodes_lock, flags);
 }
 
@@ -493,6 +494,13 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
 
 	qrtr_node_assign(node, cb->src_node);
 
+	if (cb->type == QRTR_TYPE_NEW_SERVER) {
+		/* Remote node endpoint can bridge other distant nodes */
+		const struct qrtr_ctrl_pkt *pkt = data + hdrlen;
+
+		qrtr_node_assign(node, le32_to_cpu(pkt->server.node));
+	}
+
 	if (cb->type == QRTR_TYPE_RESUME_TX) {
 		qrtr_tx_resume(node, skb);
 	} else {
-- 
2.7.4

[PATCH v2 4/5] net: qrtr: Add GFP flags parameter to qrtr_alloc_ctrl_packet

[Loic Poulain]

This will be requested for allocating control packet in atomic context.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 net/qrtr/qrtr.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
index 1d12408..a05d01e 100644
--- a/net/qrtr/qrtr.c
+++ b/net/qrtr/qrtr.c
@@ -526,18 +526,20 @@ EXPORT_SYMBOL_GPL(qrtr_endpoint_post);
 /**
  * qrtr_alloc_ctrl_packet() - allocate control packet skb
  * @pkt: reference to qrtr_ctrl_pkt pointer
+ * @flags: the type of memory to allocate
  *
  * Returns newly allocated sk_buff, or NULL on failure
  *
  * This function allocates a sk_buff large enough to carry a qrtr_ctrl_pkt and
  * on success returns a reference to the control packet in @pkt.
  */
-static struct sk_buff *qrtr_alloc_ctrl_packet(struct qrtr_ctrl_pkt **pkt)
+static struct sk_buff *qrtr_alloc_ctrl_packet(struct qrtr_ctrl_pkt **pkt,
+					      gfp_t flags)
 {
 	const int pkt_len = sizeof(struct qrtr_ctrl_pkt);
 	struct sk_buff *skb;
 
-	skb = alloc_skb(QRTR_HDR_MAX_SIZE + pkt_len, GFP_KERNEL);
+	skb = alloc_skb(QRTR_HDR_MAX_SIZE + pkt_len, flags);
 	if (!skb)
 		return NULL;
 
@@ -606,7 +608,7 @@ void qrtr_endpoint_unregister(struct qrtr_endpoint *ep)
 	mutex_unlock(&node->ep_lock);
 
 	/* Notify the local controller about the event */
-	skb = qrtr_alloc_ctrl_packet(&pkt);
+	skb = qrtr_alloc_ctrl_packet(&pkt, GFP_KERNEL);
 	if (skb) {
 		pkt->cmd = cpu_to_le32(QRTR_TYPE_BYE);
 		qrtr_local_enqueue(NULL, skb, QRTR_TYPE_BYE, &src, &dst);
@@ -663,7 +665,7 @@ static void qrtr_port_remove(struct qrtr_sock *ipc)
 	to.sq_node = QRTR_NODE_BCAST;
 	to.sq_port = QRTR_PORT_CTRL;
 
-	skb = qrtr_alloc_ctrl_packet(&pkt);
+	skb = qrtr_alloc_ctrl_packet(&pkt, GFP_KERNEL);
 	if (skb) {
 		pkt->cmd = cpu_to_le32(QRTR_TYPE_DEL_CLIENT);
 		pkt->client.node = cpu_to_le32(ipc->us.sq_node);
@@ -987,7 +989,7 @@ static int qrtr_send_resume_tx(struct qrtr_cb *cb)
 	if (!node)
 		return -EINVAL;
 
-	skb = qrtr_alloc_ctrl_packet(&pkt);
+	skb = qrtr_alloc_ctrl_packet(&pkt, GFP_KERNEL);
 	if (!skb)
 		return -ENOMEM;
 
-- 
2.7.4

[PATCH v2 5/5] net: qrtr: Release distant nodes along the bridge node

[Loic Poulain]

Distant QRTR nodes can be accessed via an other node that acts as
a bridge. When the a QRTR endpoint associated to a bridge node is
released, all the linked distant nodes should also be released.

This patch fixes endpoint release by:
- Submitting QRTR BYE message locally on behalf of all the nodes
accessible through the endpoint.
- Removing all the routable node IDs from radix tree pointing to
the released node endpoint.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 net/qrtr/qrtr.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
index a05d01e..e361de5 100644
--- a/net/qrtr/qrtr.c
+++ b/net/qrtr/qrtr.c
@@ -171,8 +171,13 @@ static void __qrtr_node_release(struct kref *kref)
 	void __rcu **slot;
 
 	spin_lock_irqsave(&qrtr_nodes_lock, flags);
-	if (node->nid != QRTR_EP_NID_AUTO)
-		radix_tree_delete(&qrtr_nodes, node->nid);
+	/* If the node is a bridge for other nodes, there are possibly
+	 * multiple entries pointing to our released node, delete them all.
+	 */
+	radix_tree_for_each_slot(slot, &qrtr_nodes, &iter, 0) {
+		if (*slot == node)
+			radix_tree_iter_delete(&qrtr_nodes, &iter, slot);
+	}
 	spin_unlock_irqrestore(&qrtr_nodes_lock, flags);
 
 	list_del(&node->item);
@@ -601,6 +606,7 @@ void qrtr_endpoint_unregister(struct qrtr_endpoint *ep)
 	struct qrtr_ctrl_pkt *pkt;
 	struct qrtr_tx_flow *flow;
 	struct sk_buff *skb;
+	unsigned long flags;
 	void __rcu **slot;
 
 	mutex_lock(&node->ep_lock);
@@ -608,11 +614,18 @@ void qrtr_endpoint_unregister(struct qrtr_endpoint *ep)
 	mutex_unlock(&node->ep_lock);
 
 	/* Notify the local controller about the event */
-	skb = qrtr_alloc_ctrl_packet(&pkt, GFP_KERNEL);
-	if (skb) {
-		pkt->cmd = cpu_to_le32(QRTR_TYPE_BYE);
-		qrtr_local_enqueue(NULL, skb, QRTR_TYPE_BYE, &src, &dst);
+	spin_lock_irqsave(&qrtr_nodes_lock, flags);
+	radix_tree_for_each_slot(slot, &qrtr_nodes, &iter, 0) {
+		if (*slot != node)
+			continue;
+		src.sq_node = iter.index;
+		skb = qrtr_alloc_ctrl_packet(&pkt, GFP_ATOMIC);
+		if (skb) {
+			pkt->cmd = cpu_to_le32(QRTR_TYPE_BYE);
+			qrtr_local_enqueue(NULL, skb, QRTR_TYPE_BYE, &src, &dst);
+		}
 	}
+	spin_unlock_irqrestore(&qrtr_nodes_lock, flags);
 
 	/* Wake up any transmitters waiting for resume-tx from the node */
 	mutex_lock(&node->qrtr_tx_lock);
-- 
2.7.4

Re: [PATCH v2 0/5] net: qrtr: Add distant node support

[Jakub Kicinski]

On Fri,  6 Nov 2020 18:33:25 +0100 Loic Poulain wrote:
> QRTR protocol allows a node to communicate with an other non-immediate 
> node via an intermdediate immediate node acting as a 'bridge':
> 
> node-0 <=> node-1 <=> node-2
> 
> This is currently not supported in this upstream version and this
> series aim to fix that.
> 
> This series is V2 because changes 1, 2 and 3 have already been submitted
> separately on LKML.

Looks like patch 1 is a bug fix and patches 2-5 add a new feature.
Is that correct?

If so first one needs to go to net and then onto 5.10, and the rest 
to net-next for 5.11.

Re: [PATCH v2 0/5] net: qrtr: Add distant node support

[Loic Poulain]

Hi Jakub,

On Sun, 8 Nov 2020 at 01:26, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri,  6 Nov 2020 18:33:25 +0100 Loic Poulain wrote:
> > QRTR protocol allows a node to communicate with an other non-immediate
> > node via an intermdediate immediate node acting as a 'bridge':
> >
> > node-0 <=> node-1 <=> node-2
> >
> > This is currently not supported in this upstream version and this
> > series aim to fix that.
> >
> > This series is V2 because changes 1, 2 and 3 have already been submitted
> > separately on LKML.
>
> Looks like patch 1 is a bug fix and patches 2-5 add a new feature.
> Is that correct?

That's correct, though strictly speaking 2-5 are also bug fix since remote node
communication is supposed to be supported in QRTR to be compatible with
other implementations (downstream or private implementations).

> If so first one needs to go to net and then onto 5.10, and the rest
> to net-next for 5.11.

I'm can split that into two series so that you can dispatch them at
your convenience.

Regards,
Loic

Re: [PATCH v2 0/5] net: qrtr: Add distant node support

[Jakub Kicinski]

On Mon, 9 Nov 2020 09:49:24 +0100 Loic Poulain wrote:
> > Looks like patch 1 is a bug fix and patches 2-5 add a new feature.
> > Is that correct?  
> 
> That's correct, though strictly speaking 2-5 are also bug fix since remote node
> communication is supposed to be supported in QRTR to be compatible with
> other implementations (downstream or private implementations).

Is there a spec we can quote to support that, or is QRTR purely 
a vendor interface?

What's the end user issue that we're solving? After firmware upgrade
things stop working? Things don't work on HW platforms on which this
was not tested? Don't work on new HW platforms?

Re: [PATCH v2 0/5] net: qrtr: Add distant node support

[Loic Poulain]

Hi Jakub,

On Mon, 9 Nov 2020 at 19:39, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 9 Nov 2020 09:49:24 +0100 Loic Poulain wrote:
> > > Looks like patch 1 is a bug fix and patches 2-5 add a new feature.
> > > Is that correct?
> >
> > That's correct, though strictly speaking 2-5 are also bug fix since remote node
> > communication is supposed to be supported in QRTR to be compatible with
> > other implementations (downstream or private implementations).
>
> Is there a spec we can quote to support that, or is QRTR purely
> a vendor interface?

There is no public spec AFAIK, this is a vendor interface.

> What's the end user issue that we're solving? After firmware upgrade
> things stop working? Things don't work on HW platforms on which this
> was not tested? Don't work on new HW platforms?

QRTR is usually something used in SoC context as communication
protocol for accessing the differents IPs (modem, WiFi, DSP, etc)
around the CPU. In that case, these components (nodes), identified
with a 'node ID', are directly reachable by the CPU (QRTR over shared
memory). This case is not impacted by the series, all nodes beeing CPU
immediate neighbours.

But today QRTR is no more a ARCH_QCOM thing only, It is also exposed
as communication channel for QCOM based wireless modules (e.g. SDX55
modem), over PCIe/MHI. In that case, the host is only connected to the
Modem CPU QRTR endpoint that in turn gives access to other embedded
Modem endpoints, acting as a gateway/bridge for accessing
non-immediate nodes from the host. currently, this case is not working
and the series fix it.

However, AFAIK, the only device would request this support is the
SDX55 PCIe module, that just landed in mhi-next. So I assume it's fine
if the related part of the series targets net-next.

Regards,
Loic

Re: [PATCH v2 0/5] net: qrtr: Add distant node support

[Jakub Kicinski]

On Tue, 10 Nov 2020 10:03:29 +0100 Loic Poulain wrote:
> On Mon, 9 Nov 2020 at 19:39, Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Mon, 9 Nov 2020 09:49:24 +0100 Loic Poulain wrote:  
> > > > Looks like patch 1 is a bug fix and patches 2-5 add a new feature.
> > > > Is that correct?  
> > >
> > > That's correct, though strictly speaking 2-5 are also bug fix since remote node
> > > communication is supposed to be supported in QRTR to be compatible with
> > > other implementations (downstream or private implementations).  
> >
> > Is there a spec we can quote to support that, or is QRTR purely
> > a vendor interface?  
> 
> There is no public spec AFAIK, this is a vendor interface.
> 
> > What's the end user issue that we're solving? After firmware upgrade
> > things stop working? Things don't work on HW platforms on which this
> > was not tested? Don't work on new HW platforms?  
> 
> QRTR is usually something used in SoC context as communication
> protocol for accessing the differents IPs (modem, WiFi, DSP, etc)
> around the CPU. In that case, these components (nodes), identified
> with a 'node ID', are directly reachable by the CPU (QRTR over shared
> memory). This case is not impacted by the series, all nodes beeing CPU
> immediate neighbours.
> 
> But today QRTR is no more a ARCH_QCOM thing only, It is also exposed
> as communication channel for QCOM based wireless modules (e.g. SDX55
> modem), over PCIe/MHI. In that case, the host is only connected to the
> Modem CPU QRTR endpoint that in turn gives access to other embedded
> Modem endpoints, acting as a gateway/bridge for accessing
> non-immediate nodes from the host. currently, this case is not working
> and the series fix it.
> 
> However, AFAIK, the only device would request this support is the
> SDX55 PCIe module, that just landed in mhi-next. So I assume it's fine
> if the related part of the series targets net-next.

Thanks! Sounds like net-next will work just fine, but won't you need
these changes in mhi-next, then? In which case you should send a pull
request based on Linus' tree so that both Mani and netdev can pull it
in.

Mani, WDYT?

Re: [PATCH v2 0/5] net: qrtr: Add distant node support

[Jakub Kicinski]

On Tue, 10 Nov 2020 09:44:27 -0800 Jakub Kicinski wrote:
> Thanks! Sounds like net-next will work just fine, but won't you need
> these changes in mhi-next, then? In which case you should send a pull
> request based on Linus' tree so that both Mani and netdev can pull it
> in.
> 
> Mani, WDYT?

Applied to net-next.

Re: [PATCH v2 0/5] net: qrtr: Add distant node support

[Manivannan Sadhasivam]

On Tue, Nov 10, 2020 at 09:44:27AM -0800, Jakub Kicinski wrote:
> On Tue, 10 Nov 2020 10:03:29 +0100 Loic Poulain wrote:
> > On Mon, 9 Nov 2020 at 19:39, Jakub Kicinski <kuba@kernel.org> wrote:
> > >
> > > On Mon, 9 Nov 2020 09:49:24 +0100 Loic Poulain wrote:  
> > > > > Looks like patch 1 is a bug fix and patches 2-5 add a new feature.
> > > > > Is that correct?  
> > > >
> > > > That's correct, though strictly speaking 2-5 are also bug fix since remote node
> > > > communication is supposed to be supported in QRTR to be compatible with
> > > > other implementations (downstream or private implementations).  
> > >
> > > Is there a spec we can quote to support that, or is QRTR purely
> > > a vendor interface?  
> > 
> > There is no public spec AFAIK, this is a vendor interface.
> > 
> > > What's the end user issue that we're solving? After firmware upgrade
> > > things stop working? Things don't work on HW platforms on which this
> > > was not tested? Don't work on new HW platforms?  
> > 
> > QRTR is usually something used in SoC context as communication
> > protocol for accessing the differents IPs (modem, WiFi, DSP, etc)
> > around the CPU. In that case, these components (nodes), identified
> > with a 'node ID', are directly reachable by the CPU (QRTR over shared
> > memory). This case is not impacted by the series, all nodes beeing CPU
> > immediate neighbours.
> > 
> > But today QRTR is no more a ARCH_QCOM thing only, It is also exposed
> > as communication channel for QCOM based wireless modules (e.g. SDX55
> > modem), over PCIe/MHI. In that case, the host is only connected to the
> > Modem CPU QRTR endpoint that in turn gives access to other embedded
> > Modem endpoints, acting as a gateway/bridge for accessing
> > non-immediate nodes from the host. currently, this case is not working
> > and the series fix it.
> > 
> > However, AFAIK, the only device would request this support is the
> > SDX55 PCIe module, that just landed in mhi-next. So I assume it's fine
> > if the related part of the series targets net-next.
> 
> Thanks! Sounds like net-next will work just fine, but won't you need
> these changes in mhi-next, then? In which case you should send a pull
> request based on Linus' tree so that both Mani and netdev can pull it
> in.
> 
> Mani, WDYT?

Sorry, missed this. mhi-next doesn't need these changes and since you've
applied to net-next, everything is fine!

I still need to apply the MHI patch which got applied to net-next and
provide an immutable branch to Kalle for another set of MHI patches...

Thanks,
Mani