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

Jonathan Cameron
2025-04-12T10:58:36Z

On Thu, 10 Apr 2025 12:10:54 +0000
Yassine Oudjana <y.oudjana@protonmail.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@protonmail.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@protonmail.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