Message-ID: <20250412115821.72f35c07@jic23-huawei>
Date: Sat, 12 Apr 2025 11:58:21 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Yassine Oudjana <y.oudjana@...tonmail.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Bjorn Andersson
 <andersson@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>, Manivannan
 Sadhasivam <manivannan.sadhasivam@...aro.org>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
 <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman
 <horms@...nel.org>, Masahiro Yamada <masahiroy@...nel.org>, Nathan
 Chancellor <nathan@...nel.org>, Nicolas Schier <nicolas.schier@...ux.dev>,
 Alexander Sverdlin <alexander.sverdlin@...il.com>, Sean Nyekjaer
 <sean@...nix.com>, Javier Carrasco <javier.carrasco.cruz@...il.com>, Matti
 Vaittinen <mazziesaccount@...il.com>, Antoniu Miclaus
 <antoniu.miclaus@...log.com>, Ramona Gradinariu
 <ramona.gradinariu@...log.com>, "Yo-Jung (Leo) Lin" <0xff07@...il.com>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, Neil Armstrong
 <neil.armstrong@...aro.org>, Barnabás Czémán
 <barnabas.czeman@...nlining.org>, Danila Tikhonov <danila@...xyga.com>,
 Antoni Pokusinski <apokusinski01@...il.com>, Vasileios Amoiridis
 <vassilisamir@...il.com>, Petar Stoykov <pd.pstoykov@...il.com>, shuaijie
 wang <wangshuaijie@...nic.com>, Yasin Lee <yasin.lee.x@...il.com>,
 "Borislav Petkov (AMD)" <bp@...en8.de>, Dave Hansen
 <dave.hansen@...ux.intel.com>, Tony Luck <tony.luck@...el.com>, Pawan Gupta
 <pawan.kumar.gupta@...ux.intel.com>, Ingo Molnar <mingo@...nel.org>,
 Yassine Oudjana <yassine.oudjana@...il.com>, linux-kernel@...r.kernel.org,
 linux-iio@...r.kernel.org, linux-arm-msm@...r.kernel.org,
 netdev@...r.kernel.org, linux-kbuild@...r.kernel.org
Subject: Re: [PATCH 1/3] net: qrtr: Turn QRTR into a bus

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

> On 06/04/2025 7:01 pm, Jonathan Cameron wrote:
> > On Sun, 06 Apr 2025 14:07:43 +0000
> > Yassine Oudjana <y.oudjana@...tonmail.com> 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@...tonmail.com>  
> > 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

Powered by blists - more mailing lists