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

[Yassine Oudjana <y.oudjana@protonmail.com>]

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.

> 
> 
>>   }
>>   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();

Ack.

> 
> 
>> +
>> +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;
>

Ack.

>> +
>> +	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)
...

> 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.

Ack.

> 
>> +
>> +	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.
Ack.

> 
>> +
>> +	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?

True, I guess this one is redundant.

>> +
>> +	if (dev)
>> +		device_unregister(dev);
> If this doesn't match anything I'm guessing it's a bug?   So maybe an error message?

I don't quite remember the reason I put this check in the first place 
but right now it seems to me like it should always be true so I'll just 
remove it and see what happens.

> 
>> +}
>> +
>> +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?

Ack.

> 
>> +	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.

Ack.

> 
>> +	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.

Will look into it.

> 
>> +
>> +	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.

Sure.

> 
>> +
>> +	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;
> 

Ack.

>> +	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.

Ack.

> 
>> +	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");
>>