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

[Jonathan Cameron <jic23@kernel.org>]

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?


>  }
>  EXPORT_SYMBOL_GPL(qrtr_endpoint_post);
>  

> diff --git a/net/qrtr/smd.c b/net/qrtr/smd.c
> index c91bf030fbc7..fd5ad6a8d1c3 100644
> --- a/net/qrtr/smd.c
> +++ b/net/qrtr/smd.c
> @@ -7,6 +7,7 @@

> +
> +static int qcom_smd_qrtr_uevent(const struct device *dev, struct kobj_uevent_env *env)
> +{
> +	const struct qrtr_device *qdev = to_qrtr_device(dev);
> +
> +	return add_uevent_var(env, "MODALIAS=%s%x:%x", QRTR_MODULE_PREFIX, qdev->service,
> +			      qdev->instance);
> +}


> +void qrtr_driver_unregister(struct qrtr_driver *drv)
> +{
> +	driver_unregister(&drv->driver);
> +}
> +EXPORT_SYMBOL_GPL(qrtr_driver_unregister);

Given this is a 'new thing' maybe namespace it from the start?
EXPORT_SYMBOL_NS_GPL();


> +
> +static int qcom_smd_qrtr_match_device_by_port(struct device *dev, const void *data)
> +{
> +	struct qrtr_device *qdev = to_qrtr_device(dev);
> +	unsigned int port = *(unsigned int *)data;
	unsinged int *port = data;
	
	return qdev->port == *port;

> +
> +	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 *) {
	};
and free new_server after all of these are filled in.
	
> +	qdev->node = new_server->node;
> +	qdev->port = new_server->port;
> +	qdev->service = new_server->service;
> +	qdev->instance = new_server->instance;
> +
> +	devm_kfree(qsdev->dev, new_server);

As below.

> +
> +	dev_set_name(&qdev->dev, "%d-%d", qdev->node, qdev->port);
> +
> +	qdev->dev.bus = &qrtr_bus;
> +	qdev->dev.parent = qsdev->dev;
> +	qdev->dev.release = qcom_smd_qrtr_dev_release;
> +	qdev->dev.driver = NULL;

it's kzalloc'd so no need to set this.

> +
> +	ret = device_register(&qdev->dev);
> +	if (ret) {
> +		dev_err(qsdev->dev, "Failed to register QRTR device: %pe\n", ERR_PTR(ret));
> +		put_device(&qdev->dev);
> +	}
> +}
> +
> +static void qcom_smd_qrtr_del_device_worker(struct work_struct *work)
> +{
> +	struct qrtr_del_server *del_server = container_of(work, struct qrtr_del_server, work);
> +	struct qrtr_smd_dev *qsdev = del_server->parent;
> +	struct device *dev = device_find_child(qsdev->dev, &del_server->port,
> +					       qcom_smd_qrtr_match_device_by_port);
> +
> +	devm_kfree(qsdev->dev, del_server);
If we are always going to free what was alocated in qcom_smd_qrtr_del_device()
why use devm at all?  
> +
> +	if (dev)
> +		device_unregister(dev);
If this doesn't match anything I'm guessing it's a bug?   So maybe an error message?

> +}
> +
> +static int qcom_smd_qrtr_add_device(struct qrtr_endpoint *parent, unsigned int node,
> +				    unsigned int port, u16 service, u16 instance)
> +{
> +	struct qrtr_smd_dev *qsdev = container_of(parent, struct qrtr_smd_dev, ep);
> +	struct qrtr_new_server *new_server;
> +
> +	new_server = devm_kzalloc(qsdev->dev, sizeof(struct qrtr_new_server), GFP_KERNEL);

As below. sizeof(*new_server)

> +	if (!new_server)
> +		return -ENOMEM;
> +
	*new_server = (struct qtr_new_server) {
		.parent = qsdev,
		.ndoe = node,
...
	};

perhaps a tiny bit easier to read?

> +	new_server->parent = qsdev;
> +	new_server->node = node;
> +	new_server->port = port;
> +	new_server->service = service;
> +	new_server->instance = instance;
> +
> +	INIT_WORK(&new_server->work, qcom_smd_qrtr_add_device_worker);
> +	schedule_work(&new_server->work);
> +
> +	return 0;
> +}
> +
> +static int qcom_smd_qrtr_del_device(struct qrtr_endpoint *parent, unsigned int port)
> +{
> +	struct qrtr_smd_dev *qsdev = container_of(parent, struct qrtr_smd_dev, ep);
> +	struct qrtr_del_server *del_server;
> +
> +	del_server = devm_kzalloc(qsdev->dev, sizeof(struct qrtr_del_server), GFP_KERNEL);

sizeof(*del_server)
preferred as then no one has to check types match.

> +	if (!del_server)
> +		return -ENOMEM;
> +
> +	del_server->parent = qsdev;
> +	del_server->port = port;
> +
> +	INIT_WORK(&del_server->work, qcom_smd_qrtr_del_device_worker);
> +	schedule_work(&del_server->work);
> +
> +	return 0;
> +}
> +
> +static int qcom_smd_qrtr_device_unregister(struct device *dev, void *data)
> +{
> +	device_unregister(dev);

One option that may simplify this is to do the device_unregister() handling
a devm_action_or_reset() handler that is using the parent device as it's dev
but unregistering the children.  That way the unregister is called in the
reverse order of setup and you only register a handler for those devices
registered (rather walking children).  I did this in the CXL pmu driver
for instance.

> +
> +	return 0;
> +}
> +

> @@ -82,9 +276,11 @@ static int qcom_smd_qrtr_probe(struct rpmsg_device *rpdev)
>  
>  static void qcom_smd_qrtr_remove(struct rpmsg_device *rpdev)
>  {
> -	struct qrtr_smd_dev *qdev = dev_get_drvdata(&rpdev->dev);
> +	struct qrtr_smd_dev *qsdev = dev_get_drvdata(&rpdev->dev);

May be worth doing the rename in a precursor patch to simplify a little what is
in this one.

> +
> +	device_for_each_child(qsdev->dev, NULL, qcom_smd_qrtr_device_unregister);
>  
> -	qrtr_endpoint_unregister(&qdev->ep);
> +	qrtr_endpoint_unregister(&qsdev->ep);
>  
>  	dev_set_drvdata(&rpdev->dev, NULL);
>  }
> @@ -104,7 +300,27 @@ static struct rpmsg_driver qcom_smd_qrtr_driver = {
>  	},
>  };
>  
> -module_rpmsg_driver(qcom_smd_qrtr_driver);
> +static int __init qcom_smd_qrtr_init(void)
> +{
> +	int ret;
> +
> +	ret = bus_register(&qrtr_bus);
> +	if (!ret)
> +		ret = register_rpmsg_driver(&qcom_smd_qrtr_driver);
This style tends to extend badly. Go with more conventional errors
out of line style.

	if (ret)
		return ret;

	ret = register_rpmsg_driver(&qcom_smd_qrtr_driver);
	if (ret) {
		bus_unregister(&qtr_bus);
		return ret;		
	}

	return 0;

> +	else
> +		bus_unregister(&qrtr_bus);
> +
> +	return ret;
> +}
> +
> +static void __exit qcom_smd_qrtr_exit(void)
> +{
> +	bus_unregister(&qrtr_bus);

Order should be the reverse of what happened in probe so swap these round.

> +	unregister_rpmsg_driver(&qcom_smd_qrtr_driver);
> +}
> +
> +subsys_initcall(qcom_smd_qrtr_init);
> +module_exit(qcom_smd_qrtr_exit);
>  
>  MODULE_ALIAS("rpmsg:IPCRTR");
>  MODULE_DESCRIPTION("Qualcomm IPC-Router SMD interface driver");
>