Re: [PATCH 1/3] net: qrtr: Turn QRTR into a bus

[Jonathan Cameron]

On Thu, 10 Apr 2025 12:10:54 +0000
Yassine Oudjana <y.oudjana@xxxxxxxxxxxxxx> wrote:

> On 06/04/2025 7:01 pm, Jonathan Cameron wrote:
> > On Sun, 06 Apr 2025 14:07:43 +0000
> > Yassine Oudjana <y.oudjana@xxxxxxxxxxxxxx> wrote:
> >   
> >> Implement a QRTR bus to allow for creating drivers for individual QRTR
> >> services. With this in place, devices are dynamically registered for QRTR
> >> services as they become available, and drivers for these devices are
> >> matched using service and instance IDs.
> >>
> >> In smd.c, replace all current occurences of qdev with qsdev in order to
> >> distinguish between the newly added QRTR device which represents a QRTR
> >> service with the existing QRTR SMD device which represents the endpoint
> >> through which services are provided.
> >>
> >> Signed-off-by: Yassine Oudjana <y.oudjana@xxxxxxxxxxxxxx>  
> > Hi Yassine
> > 
> > Just took a quick look through.
> > 
> > It might make more sense to do this with an auxiliary_bus rather
> > than defining a new bus.
> > 
> > I'd also split out the renames as a precursor patch.
> > 
> > Various other comments inline.
> > 
> > Jonathan
> >   
> >> diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
> >> index 00c51cf693f3..e11682fd7960 100644
> >> --- a/net/qrtr/af_qrtr.c
> >> +++ b/net/qrtr/af_qrtr.c
> >> @@ -435,6 +435,7 @@ static void qrtr_node_assign(struct qrtr_node *node, unsigned int nid)
> >>   int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
> >>   {
> >>   	struct qrtr_node *node = ep->node;
> >> +	const struct qrtr_ctrl_pkt *pkt;
> >>   	const struct qrtr_hdr_v1 *v1;
> >>   	const struct qrtr_hdr_v2 *v2;
> >>   	struct qrtr_sock *ipc;
> >> @@ -443,6 +444,7 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
> >>   	size_t size;
> >>   	unsigned int ver;
> >>   	size_t hdrlen;
> >> +	int ret = 0;
> >>
> >>   	if (len == 0 || len & 3)
> >>   		return -EINVAL;
> >> @@ -516,12 +518,24 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
> >>
> >>   	qrtr_node_assign(node, cb->src_node);
> >>
> >> +	pkt = data + hdrlen;
> >> +
> >>   	if (cb->type == QRTR_TYPE_NEW_SERVER) {
> >>   		/* Remote node endpoint can bridge other distant nodes */
> >> -		const struct qrtr_ctrl_pkt *pkt;
> >> -
> >> -		pkt = data + hdrlen;
> >>   		qrtr_node_assign(node, le32_to_cpu(pkt->server.node));
> >> +
> >> +		/* Create a QRTR device */
> >> +		ret = ep->add_device(ep, le32_to_cpu(pkt->server.node),
> >> +					       le32_to_cpu(pkt->server.port),
> >> +					       le32_to_cpu(pkt->server.service),
> >> +					       le32_to_cpu(pkt->server.instance));
> >> +		if (ret)
> >> +			goto err;
> >> +	} else if (cb->type == QRTR_TYPE_DEL_SERVER) {
> >> +		/* Remove QRTR device corresponding to service */
> >> +		ret = ep->del_device(ep, le32_to_cpu(pkt->server.port));
> >> +		if (ret)
> >> +			goto err;
> >>   	}
> >>
> >>   	if (cb->type == QRTR_TYPE_RESUME_TX) {
> >> @@ -543,8 +557,7 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
> >>
> >>   err:
> >>   	kfree_skb(skb);
> >> -	return -EINVAL;
> >> -
> >> +	return ret ? ret : -EINVAL;  
> > How do we get here with non error value given we couldn't before?  
> 
> We don't, but we may have errors in ret other than -EINVAL returned by 
> the newly added add_device and del_device which we should propagate.

Ah. Got it (I misread that!).  Personally I'd go for setting ret in the
other error paths explicitly to -EINVAL.  Mixing two styles of handling
where you have some paths setting ret and some not is rather confusing to read.




> >> +
> >> +	return qdev->port == port;
> >> +}
> >> +
> >> +static void qcom_smd_qrtr_add_device_worker(struct work_struct *work)
> >> +{
> >> +	struct qrtr_new_server *new_server = container_of(work, struct qrtr_new_server, work);
> >> +	struct qrtr_smd_dev *qsdev = new_server->parent;
> >> +	struct qrtr_device *qdev;
> >> +	int ret;
> >> +
> >> +	qdev = kzalloc(sizeof(*qdev), GFP_KERNEL);
> >> +	if (!qdev)
> >> +		return;
> >> +  
> > Maybe
> > 	*qdev = (struct qrtr_device *) {
> > 	};  
> 
> (struct qrtr_device)

oops. Indeed that!


Jonathan