Re: [PATCH 3/3] driver: rpmon: add rpmon_qmi driver

[çæè]

Hi,
From: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
Date: 2020-04-12 03:23:00
To:  Wang Wenhu <wenhu.wang@xxxxxxxx>,gregkh@xxxxxxxxxxxxxxxxxxx,linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [PATCH 3/3] driver: rpmon: add rpmon_qmi driver>Hi--
>
>On 4/11/20 2:53 AM, Wang Wenhu wrote:
>> RPMON_QMI is used by RPMON to communicate with remote processors
>> with QMI APIs if enabled. RPMON_QMI itself is designed as a modular
>> framework that would introduce different kinds of message sets
>> which may be updated for versions.
>> 
>> RPMON_QMI creates a device of rpmon_device type for each remote
>> processor endpoint. All the endpoint devices shares an unique set
>> of QMI suite.
>> 
>> Signed-off-by: Wang Wenhu <wenhu.wang@xxxxxxxx>
>> ---
>>  drivers/rpmon/Kconfig     |  15 ++
>>  drivers/rpmon/Makefile    |   1 +
>>  drivers/rpmon/rpmon_qmi.c | 434 ++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 450 insertions(+)
>>  create mode 100644 drivers/rpmon/rpmon_qmi.c
>> 
>
>> diff --git a/drivers/rpmon/rpmon_qmi.c b/drivers/rpmon/rpmon_qmi.c
>> new file mode 100644
>> index 000000000000..ae49510cbb83
>> --- /dev/null
>> +++ b/drivers/rpmon/rpmon_qmi.c
>> @@ -0,0 +1,434 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2020 Vivo Communication Technology Co. Ltd.
>> + * Copyright (C) 2020 Wang Wenhu <wenhu.wang@xxxxxxxx>
>> + * All rights reserved.
>> + *
>> + * RPMON: An implementation of remote processor monitor freamwork
>
>                                                           framework
>

Addressed in v2

>> + * for platforms that multi-processors exists. RPMON is implemented
>
>      confusing sentence above.
>

Addressed in v2

>> + * with chardev and sysfs class as interfaces to communicate with
>> + * the user level. It supports different communication interfaces
>> + * added modularly with remote processors. Currently QMI implementation
>> + * is available.
>> + *
>> + * RPMON could be used to detect the stabilities of remote processors,
>> + * collect any kinds of information you are interested with, take
>
>                                               interested in, take
>

Addressed in v2

>> + * actions like connection status check, and so on. Enhancements can
>> + * be made upon current implementation.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/rpmon.h>
>> +#include <linux/soc/qcom/qmi.h>
>> +#include <linux/of_platform.h>
>> +#include "rpmon_qmi.h"
>> +
>> +#define DRIVER_NAME "rpmon_qmi_drv"
>> +
>> +/*
>> + * Remote processor registered.
>> + */
>> +#define RP_REGISTERED		0x0001
>> +
>> +/*
>> + * work struct for message processing.
>> + */
>> +struct recv_work {
>> +	struct work_struct work;
>> +	struct sockaddr_qrtr sq;
>> +	void *msg;
>> +};
>> +
>> +/*
>> + * Delayed work to take a reset action when a failure is detected.
>> + */
>> +struct exec_cb_work {
>> +	struct delayed_work		dwk;
>> +	struct rpmon_qmi_device *rdev;
>> +	u32			checks;
>> +};
>> +
>> +struct rpmon_qmi_exec_fn {
>> +	int (*exec_call)(struct rpmon_qmi_device *rdev);
>> +};
>> +
>> +struct rpmon_qmi_cb_fn {
>> +	void (*callback)(struct work_struct *work);
>> +	u32 msg_len;
>> +};
>> +
>> +static DEFINE_MUTEX(rdev_list_lock);
>> +static LIST_HEAD(rdev_list);
>> +static struct rpmon_qmi *rpqmi;
>> +static struct workqueue_struct *rpqmi_wq;
>
>[snip]
>
>> +/**
>> + * rpmon_qmi_exec_cb_worker - callback worker for execution
>> + * @work: work to been done
>> + *
>> + * Called as worker handler by the signle worker thread of rpmon_wq.
>
>                                      single
>

Addressed in v2

>> + * The worker is scheduled after timeout ms duration since the execution.
>> + */
>> +static void rpmon_qmi_exec_cb_worker(struct work_struct *work)
>> +{
>> +	struct delayed_work *dwk = to_delayed_work(work);
>> +	struct exec_cb_work *ewk =
>> +			container_of(dwk, struct exec_cb_work, dwk);
>> +	struct rpmon_qmi_device *rdev = ewk->rdev;
>> +
>> +	mutex_lock(&rdev_list_lock);
>> +	if (ewk->checks <= atomic_read(&rdev->reports)) {
>> +		pr_debug("%s health check success", rdev->info->name);
>> +		goto out;
>> +	}
>> +
>> +	pr_err("subsystem %s failed to respond in time", rdev->info->name);
>> +
>> +	rpmon_event_notify(rdev->info, RPMON_EVENT_CHKCONN_FAIL);
>> +
>> +out:
>> +	mutex_unlock(&rdev_list_lock);
>> +	kfree(ewk);
>> +}
>> +
>> +static struct rpmon_qmi_cb_fn rpmon_qmi_event_callbacks[] = {
>> +	{
>> +		.callback = rpmon_qmi_register_req_cb,
>> +		.msg_len = sizeof(struct rpmon_register_req),
>> +	},
>> +	{
>> +		.callback = rpmon_qmi_conn_check_resp_cb,
>> +		.msg_len = sizeof(struct rpmon_conn_check_resp),
>> +	},
>> +};
>> +
>> +/**
>> + * rpmon_qmi_conn_check - send indication, initiate and queue callback work
>> + * @rdev: device interface of specific remote processor to be checked
>> + */
>> +static int rpmon_qmi_conn_check(struct rpmon_qmi_device *rdev)
>> +{
>> +	struct exec_cb_work *ewk;
>> +
>> +	mutex_lock(&rdev_list_lock);
>> +	if (!(rdev->flag & RP_REGISTERED)) {
>> +		pr_err("%s has not registered", rdev->info->name);
>> +		return -ENOANO;
>
>Why ENOANO?
>
My fault, should be ENONET cause the remote endpoint has not registered,
meaning "Machine is not on the network".

Addressed in v2

>> +	}
>> +
>> +	if (!__ratelimit(&rdev->ratelimit)) {
>> +		pr_err("%s rate-limited", rdev->info->name);
>> +		return 0;
>> +	}
>> +	mutex_unlock(&rdev_list_lock);
>> +
>> +	rdev->rqmi->sendmsg(rdev, NULL, 0);
>> +
>> +	ewk = kzalloc(sizeof(*ewk), GFP_KERNEL);
>> +	if (!ewk)
>> +		return -ENOANO;
>
>ditto.

My fault, should be ENOMEM;
Addressed in v2

>
>> +	ewk->rdev = rdev;
>> +	ewk->checks = atomic_inc_return(&rdev->checks);
>> +	INIT_DELAYED_WORK(&ewk->dwk, rpmon_qmi_exec_cb_worker);
>> +	queue_delayed_work(rpqmi_wq,
>> +			   &ewk->dwk, msecs_to_jiffies(rdev->timeout));
>> +
>> +	return 0;
>> +}
>> +
>> +static struct rpmon_qmi_exec_fn rpmon_qmi_exec_calls[] = {
>> +	{.exec_call = rpmon_qmi_conn_check},
>> +};
>> +
>> +static int rpmon_qmi_monitor(struct rpmon_info *info, u32 event)
>> +{
>> +	struct rpmon_qmi_device *rdev = (struct rpmon_qmi_device *)info->priv;
>> +	int i;
>> +
>> +	for (i = 0; i < RPMON_EXEC_MAX; i++) {
>> +		if (event & RPMON_ACTION(i)) {
>> +			if (i >= (sizeof(rpmon_qmi_exec_calls) /
>> +				  sizeof(struct rpmon_qmi_exec_fn)))
>> +				return -ENOTSUPP;
>> +
>
>I'm not totally up about speculative fetches, but is any of
><linux/nospec.h> needed here?
>

Surely, addressed in v2

>> +			if (rpmon_qmi_exec_calls[i].exec_call)
>> +				return rpmon_qmi_exec_calls[i].exec_call(rdev);
>> +			else
>> +				return -ENOTSUPP;
>> +		}
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>
>[snip]
>
>> +static void rpmon_qmi_msg_callback(enum rpmon_qmi_msg_type type,
>> +			struct sockaddr_qrtr *sq,
>> +			const void *msg)
>> +{
>> +	struct recv_work *rwk;
>> +
>> +	if (type >= (sizeof(rpmon_qmi_event_callbacks) /
>> +		     sizeof(struct rpmon_qmi_cb_fn))) {
>> +		pr_err("Error none supported message type.\n");
>
>		              non-supported
>

Addressed in v2

>> +		return;
>> +	}
>> +
>> +	if (rpmon_qmi_event_callbacks[type].callback) {
>> +		rwk = kzalloc(sizeof(*rwk), GFP_KERNEL);
>> +		if (!rwk) {
>> +			pr_err("Error to alloc recv_work");
>> +			return;
>> +		}
>> +
>> +		INIT_WORK(&rwk->work, rpmon_qmi_event_callbacks[type].callback);
>> +		memcpy(&rwk->sq, sq, sizeof(*sq));
>> +
>> +		rwk->msg = kzalloc(rpmon_qmi_event_callbacks[type].msg_len,
>> +				   GFP_KERNEL);
>> +		if (!rwk->msg) {
>> +			pr_err("Error to alloc message of recv_work");
>> +			kfree(rwk);
>> +			return;
>> +		}
>> +
>> +		memcpy(rwk->msg, msg, rpmon_qmi_event_callbacks[type].msg_len);
>> +		queue_work(rpqmi_wq, &rwk->work);
>> +	}
>> +}
>
>[snip]
>
>> +static void __exit rpmon_qmi_drv_exit(void)
>> +{
>> +	rpmon_qmi_del_server();
>> +
>> +	qmi_handle_release(&rpqmi->qmi);
>> +
>> +	platform_driver_unregister(&rpmon_qmi_drv);
>> +}
>> +module_exit(rpmon_qmi_drv_exit);
>> +
>> +MODULE_AUTHOR("Wang Wenhu");
>
>Please add email address above.
>

Addressed in v2

>> +MODULE_DESCRIPTION("Subsystem Monitor via QMI platform driver");
>> +MODULE_ALIAS("platform:" DRIVER_NAME);
>> +MODULE_LICENSE("GPL v2");
>> 
>
>thanks.
>-- 
>~Randy
>

Thanks,
Wenhu