Re: [PATCH 2/3] driver: rpmon: qmi message version 01

[çæè]

Hi,
From: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
Date: 2020-04-12 03:07:11
To:  Wang Wenhu <wenhu.wang@xxxxxxxx>,gregkh@xxxxxxxxxxxxxxxxxxx,linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [PATCH 2/3] driver: rpmon: qmi message version 01>Hi--
>
>On 4/11/20 2:53 AM, Wang Wenhu wrote:
>> Implements a RPMON_QMI message set for a certain rpmon service.
>> RPMON_QMI defines its message types modularly. Each rpmon service
>> binds to a message set and introduced as a module. This version 1.0
>> message set could be used for connection checking of remote processors.
>> 
>> RPMON_QMI messages depend on QCOM_QMI_HELPERS and should be updated
>> together with QMI related modules.
>> 
>> Signed-off-by: Wang Wenhu <wenhu.wang@xxxxxxxx>
>> ---
>>  drivers/rpmon/Kconfig            |  13 ++
>>  drivers/rpmon/Makefile           |   1 +
>>  drivers/rpmon/rpmon_qmi.h        |  77 ++++++++++
>>  drivers/rpmon/rpmon_qmi_msg_v1.c | 240 +++++++++++++++++++++++++++++++
>>  4 files changed, 331 insertions(+)
>>  create mode 100644 drivers/rpmon/rpmon_qmi.h
>>  create mode 100644 drivers/rpmon/rpmon_qmi_msg_v1.c
>> 
>
>> diff --git a/drivers/rpmon/rpmon_qmi.h b/drivers/rpmon/rpmon_qmi.h
>> new file mode 100644
>> index 000000000000..f6e7cfc97a3f
>> --- /dev/null
>> +++ b/drivers/rpmon/rpmon_qmi.h
>> @@ -0,0 +1,77 @@
>> +/* 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.
>> + */
>> +
>> +#ifndef RPMON_QMI_INTERFACE_H
>> +#define RPMON_QMI_INTERFACE_H
>> +
>> +#define RP_NAME_LEN 255
>> +#define RPQMI_BUF_SIZE	4096
>> +#define QMI_TLV_TL_SIZE	3
>> +
>> +enum rpmon_exec_result {
>> +	RPMON_EXEC_SUCCESS = 0,
>> +	RPMON_EXEC_FAILURE = 1,
>> +};
>> +
>> +struct rpmon_register_req {
>> +	uint8_t name_valid;
>> +	char name[RP_NAME_LEN + 1];
>> +	uint8_t timeout_valid;
>> +	u32 timeout;
>> +};
>
>Is this struct packed or padded?
>Is it shared with/used by other drivers?
>
It is passed and repacked by qmi interfaces and then sent out as QRTR packet,
non-shared with others. So it is safe to be left without any locking protection here.
>> +
>> +struct rpmon_conn_indication {
>> +	char placeholder;
>> +};
>> +
>> +struct rpmon_conn_check_resp {
>> +	uint8_t result_valid;
>> +	struct qmi_response_type_v01 result;
>> +};
>> +
>> +struct rpmon_response {
>> +	struct qmi_response_type_v01 resp;
>> +};
>> +
>> +struct rpmon_qmi_device {
>> +	struct list_head		list;
>> +	struct sockaddr_qrtr	addr;
>> +	u32			timeout;
>> +	u32			flag;
>> +	struct ratelimit_state	ratelimit;
>> +
>> +	atomic_t		checks;
>> +	atomic_t		reports;
>> +
>> +	struct rpmon_info *info;
>> +	struct rpmon_qmi  *rqmi;
>> +};
>> +
>> +struct rpmon_qmi {
>> +	struct rpmon_info		*info;
>> +	struct qmi_handle		qmi;
>> +	struct qmi_service		*svc;
>> +	struct qmi_ops			*ops;
>> +	struct qmi_msg_handler		*handlers;
>> +	int (*sendmsg)(const struct rpmon_qmi_device *rdev,
>> +			const void *data,
>> +			u32 len);
>> +};
>> +
>> +/* rpqmi message types currently supported. */
>> +enum rpmon_qmi_msg_type {
>> +	RPQMI_MSG_REGISTER = 0,
>> +	RPQMI_MSG_CONNCHK_RESP,
>> +	RPQMI_MSG_MAX,
>> +};
>> +
>> +int rpmon_qmi_handle_init(struct rpmon_qmi *rqmi,
>> +		void (*cb)(enum rpmon_qmi_msg_type type,
>> +			struct sockaddr_qrtr *sq,
>> +			const void *msg));
>> +
>> +#endif /* RPMON_QMI_INTERFACE_H */
>> diff --git a/drivers/rpmon/rpmon_qmi_msg_v1.c b/drivers/rpmon/rpmon_qmi_msg_v1.c
>> new file mode 100644
>> index 000000000000..10d38d8133b0
>> --- /dev/null
>> +++ b/drivers/rpmon/rpmon_qmi_msg_v1.c
>> @@ -0,0 +1,240 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * 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
>
>typo:                                                         framework  {in all 3 source files}
>
>> + * for platforms that multi-processors exists. RPMON is implemented
>
>but that sentence is confusing at best.
>

Both 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, and this file implements the message type v1.
>> + *
>> + * 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/soc/qcom/qmi.h>
>> +#include <linux/rpmon.h>
>> +#include "rpmon_qmi.h"
>> +
>> +#define RPMON_SVC_ID_V01	0x3c
>> +#define RPMON_SVC_VER_V01	0x01
>> +#define RPMON_SVC_INS_V01	0x00
>> +
>> +#define RPMON_CONN_REQ_MSG_ID_VO1			0x20
>> +#define RPMON_CONN_RESP_MSG_ID_VO1			0x20
>> +#define RPMON_CONN_IND_MSG_ID_V01			0x21
>> +#define RPMON_EXEC_COMP_REQ_MSG_ID_V01		0x22
>> +#define RPMON_EXEC_COMP_RESP_MSG_ID_V01		0x22
>> +
>> +static struct qmi_elem_info register_req_v01_ei[] = {
>> +	{
>> +		.data_type      = QMI_OPT_FLAG,
>> +		.elem_len       = 1,
>> +		.elem_size      = sizeof(uint8_t),
>> +		.array_type     = NO_ARRAY,
>> +		.tlv_type       = 0x10,
>
>Can you use an enum or #define value for the tlv_type magic numbers?
>(multiple places)

Definitely. But actually the definitions belong to QMI module, and I will
figure this out with another patch of #include <linux/soc/qcom/qmi.h> or so.
I will Cc you if not mind?

>
>> +		.offset         = offsetof(struct rpmon_register_req,
>> +					   name_valid),
>> +	},
>> +	{
>> +		.data_type      = QMI_STRING,
>> +		.elem_len       = RP_NAME_LEN,
>> +		.elem_size      = sizeof(char),
>> +		.array_type     = NO_ARRAY,
>> +		.tlv_type       = 0x10,
>> +		.offset         = offsetof(struct rpmon_register_req,
>> +					   name),
>> +	},
>> +	{
>> +		.data_type      = QMI_OPT_FLAG,
>> +		.elem_len       = 1,
>> +		.elem_size      = sizeof(uint8_t),
>> +		.array_type     = NO_ARRAY,
>> +		.tlv_type       = 0x11,
>> +		.offset         = offsetof(struct rpmon_register_req,
>> +					   timeout_valid),
>> +	},
>> +	{
>> +		.data_type      = QMI_UNSIGNED_4_BYTE,
>> +		.elem_len       = 1,
>> +		.elem_size      = sizeof(u32),
>> +		.array_type     = NO_ARRAY,
>> +		.tlv_type       = 0x11,
>> +		.offset         = offsetof(struct rpmon_register_req,
>> +					   timeout),
>> +	},
>> +	{
>> +		.data_type      = QMI_EOTI,
>> +		.array_type     = NO_ARRAY,
>> +		.tlv_type       = QMI_COMMON_TLV_TYPE,
>> +	},
>> +};
>> +
>> +static struct qmi_elem_info conn_check_resp_v01_ei[] = {
>> +	{
>> +		.data_type      = QMI_OPT_FLAG,
>> +		.elem_len       = 1,
>> +		.elem_size      = sizeof(uint8_t),
>> +		.array_type     = NO_ARRAY,
>> +		.tlv_type       = 0x10,
>> +		.offset         = offsetof(struct rpmon_conn_check_resp,
>> +					   result_valid),
>> +	},
>> +	{
>> +		.data_type      = QMI_SIGNED_4_BYTE_ENUM,
>> +		.elem_len       = 1,
>> +		.elem_size      = sizeof(enum rpmon_exec_result),
>> +		.array_type     = NO_ARRAY,
>> +		.tlv_type       = 0x10,
>> +		.offset         = offsetof(struct rpmon_conn_check_resp,
>> +					   result),
>> +	},
>> +	{
>> +		.data_type      = QMI_EOTI,
>> +		.array_type     = NO_ARRAY,
>> +		.tlv_type       = QMI_COMMON_TLV_TYPE,
>> +	},
>> +};
>> +
>> +static struct qmi_elem_info conn_indication_v01_ei[] = {
>> +	{
>> +		.data_type      = QMI_EOTI,
>> +		.array_type     = NO_ARRAY,
>> +		.tlv_type       = QMI_COMMON_TLV_TYPE,
>> +	},
>> +};
>> +
>> +static struct qmi_elem_info response_v01_ei[] = {
>> +	{
>> +		.data_type      = QMI_STRUCT,
>> +		.elem_len       = 1,
>> +		.elem_size      = sizeof(struct qmi_response_type_v01),
>> +		.array_type     = NO_ARRAY,
>> +		.tlv_type       = 0x02,
>> +		.offset         = offsetof(
>> +				struct rpmon_response,
>> +				resp),
>
>multiple lines not needed above.

Addressed in v2

>
>> +		.ei_array      = qmi_response_type_v01_ei,
>> +	},
>> +	{
>> +		.data_type      = QMI_EOTI,
>> +		.array_type     = NO_ARRAY,
>> +		.tlv_type       = QMI_COMMON_TLV_TYPE,
>> +	},
>> +};
>> +
>> +static void (*msg_callback)(enum rpmon_qmi_msg_type type,
>> +			    struct sockaddr_qrtr *sq,
>> +			    const void *msg);
>> +
>> +static int rpmon_qmi_sendmsg_v1(const struct rpmon_qmi_device *rdev,
>> +				const void *data,
>> +				u32 len)
>> +{
>> +	int ret;
>> +	struct sockaddr_qrtr sq;
>> +
>> +	memcpy(&sq, &rdev->addr, sizeof(sq));
>> +
>> +	ret = qmi_send_indication(&rdev->rqmi->qmi, &sq,
>> +				  RPMON_CONN_IND_MSG_ID_V01,
>> +				  QMI_TLV_TL_SIZE,
>> +				  conn_indication_v01_ei, NULL);
>> +	if (ret < 0)
>> +		pr_err("Error %d send indication failed", ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static void rpmon_qmi_recv_register_req_v1(struct qmi_handle *qmi,
>> +			struct sockaddr_qrtr *sq,
>> +			struct qmi_txn *txn,
>> +			const void *msg)
>> +{
>> +	struct rpmon_response resp;
>> +	int ret;
>> +
>> +	resp.resp.result = QMI_RESULT_SUCCESS_V01;
>> +	resp.resp.error = QMI_ERR_NONE_V01;
>> +	ret = qmi_send_response(qmi, sq, txn,
>> +				RPMON_CONN_RESP_MSG_ID_VO1,
>> +				sizeof(resp.resp) + QMI_TLV_TL_SIZE * 2,
>> +				response_v01_ei,
>> +				&resp.resp);
>> +	if (ret < 0)
>> +		pr_err("Error %d send respons failed", ret);
>> +
>> +	if (msg_callback)
>> +		msg_callback(RPQMI_MSG_REGISTER, sq, msg);
>> +}
>> +
>> +void rpmon_qmi_recv_conn_check_resp_v1(struct qmi_handle *qmi,
>> +	struct sockaddr_qrtr *sq,
>> +	struct qmi_txn *txn,
>> +	const void *msg)
>> +{
>> +	struct rpmon_response resp;
>> +	int ret;
>> +
>> +	resp.resp.result = QMI_RESULT_SUCCESS_V01;
>> +	resp.resp.error = QMI_ERR_NONE_V01;
>> +	ret = qmi_send_response(qmi, sq, txn,
>> +				RPMON_EXEC_COMP_REQ_MSG_ID_V01,
>> +				sizeof(resp.resp) + QMI_TLV_TL_SIZE * 2,
>> +				response_v01_ei,
>> +				&resp.resp);
>> +	if (ret < 0)
>> +		pr_err("Error %d send respons failed", ret);
>
>		                      response
>

Addressed in v2

>> +
>> +	if (msg_callback)
>> +		msg_callback(RPQMI_MSG_CONNCHK_RESP, sq, msg);
>> +}
>> +
>> +static struct qmi_msg_handler rpmon_qmi_msg_handlers_v01[] = {
>> +	{
>> +		.type = QMI_REQUEST,
>> +		.msg_id = RPMON_CONN_REQ_MSG_ID_VO1,
>> +		.ei = register_req_v01_ei,
>> +		.decoded_size = sizeof(struct rpmon_register_req),
>> +		.fn = rpmon_qmi_recv_register_req_v1,
>> +	},
>> +	{
>> +		.type = QMI_REQUEST,
>> +		.msg_id = RPMON_EXEC_COMP_REQ_MSG_ID_V01,
>> +		.ei = conn_check_resp_v01_ei,
>> +		.decoded_size = sizeof(struct rpmon_conn_check_resp),
>> +		.fn = rpmon_qmi_recv_conn_check_resp_v1,
>> +	},
>> +};
>> +
>> +static struct qmi_service rpmon_qmi_svc = {
>> +	.service = RPMON_SVC_ID_V01,
>> +	.version = RPMON_SVC_VER_V01,
>> +	.instance = RPMON_SVC_INS_V01,
>> +};
>> +
>> +int rpmon_qmi_handle_init(struct rpmon_qmi *rqmi,
>> +		void (*cb)(enum rpmon_qmi_msg_type type,
>> +			   struct sockaddr_qrtr *sq,
>> +			   const void *msg))
>> +{
>> +	rqmi->svc = &rpmon_qmi_svc;
>> +	rqmi->handlers = rpmon_qmi_msg_handlers_v01;
>> +	rqmi->sendmsg = rpmon_qmi_sendmsg_v1;
>> +
>> +	if (cb)
>> +		msg_callback = cb;
>> +	else
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(rpmon_qmi_handle_init);
>> +
>> +MODULE_AUTHOR("Wang Wenhu");
>
>Please include email address in the string above.
>About 3/4 of all uses of MODULE_AUTHOR() do so.
>

Addressed in v2

>> +MODULE_LICENSE("GPL v2");
>> 
>
>thanks.
>-- 
>~Randy
>

Thanks,
Wenhu