Re:Re: [PATCH 1/3] driver: rpmon: new driver Remote Processor Monitor

From: 王文虎 <wenhu.wang@vivo.com>
To: Randy Dunlap <rdunlap@infradead.org>
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org
Subject: Re:Re: [PATCH 1/3] driver: rpmon: new driver Remote Processor Monitor
Date: Sun, 12 Apr 2020 18:52:10 +0800 (GMT+08:00)	[thread overview]
Message-ID: <AHgABAClCOKoeJL-ajjSxaqg.3.1586688730769.Hmail.wenhu.wang@vivo.com> (raw)
In-Reply-To: <64b5f77a-7bc1-ee43-0f83-1ff323e4ac51@infradead.org>

Hi,
From: Randy Dunlap <rdunlap@infradead.org>
Date: 2020-04-12 02:08:09
To:  Wang Wenhu <wenhu.wang@vivo.com>,gregkh@linuxfoundation.org,linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] driver: rpmon: new driver Remote Processor Monitor>Hi--
>
>On 4/11/20 2:52 AM, Wang Wenhu wrote:
>> RPMON is a driver framework. It supports remote processor monitor
>> from user level. The baisc components are a character device
>
>                       basic

Addressed in v2

>
>> with sysfs interfaces for user space communication and different
>> kinds of message drivers introduced modularly, which are used to
>> communicate with remote processors.
>> 
>> As for user space, one can get notifications of different events
>> of remote processors, like their registrations, through standard
>> file read operation of the file discriptors related to the exported
>
>                                  descriptors

Addressed in v2

>
>> character devices. Actions can also be taken into account via
>> standard write operations to the devices. Besides, the sysfs class
>> attributes could be accessed conveniently.
>> 
>> Message drivers act as engines to communicate with remote processors.
>> Currently RPMON_QMI is available which uses QMI infrastructures
>> on Qualcomm SoC Platforms.
>> 
>> Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>
>> ---
>>  drivers/Kconfig        |   2 +
>>  drivers/Makefile       |   1 +
>>  drivers/rpmon/Kconfig  |  26 +++
>>  drivers/rpmon/Makefile |   1 +
>>  drivers/rpmon/rpmon.c  | 505 +++++++++++++++++++++++++++++++++++++++++
>>  include/linux/rpmon.h  |  68 ++++++
>>  6 files changed, 603 insertions(+)
>>  create mode 100644 drivers/rpmon/Kconfig
>>  create mode 100644 drivers/rpmon/Makefile
>>  create mode 100644 drivers/rpmon/rpmon.c
>>  create mode 100644 include/linux/rpmon.h
>> 
>> diff --git a/drivers/rpmon/Kconfig b/drivers/rpmon/Kconfig
>> new file mode 100644
>> index 000000000000..505d263e0867
>> --- /dev/null
>> +++ b/drivers/rpmon/Kconfig
>> @@ -0,0 +1,26 @@
>> +#
>> +# Remote Processor Monitor Drivers
>> +#
>> +menu "Remote Processor Monitor Drivers"
>> +
>> +config RPMON
>> +	tristate "Remote Processor Monitor Core Framework"
>> +	help
>> +	  RPMON is a driver framework. It supports remote processor monitor
>> +	  from user level. The baisc components are a character device
>
>	                       basic

Addressed in v2

>
>> +	  with sysfs interfaces for user space communication and different
>> +	  kinds of message drivers introduced modularly, which are used to
>> +	  communicate with remote processors.
>> +
>> +	  As for user space, one can get notifications of different events
>> +	  of remote processors, like their registrations, through standard
>> +	  file read operation of the file discriptors related to the exported
>
>	                                  descriptors

Addressed in v2

>
>> +	  character devices. Actions can also be taken into account via
>> +	  standard write operations to the devices. Besides, the sysfs class
>> +	  attributes could be accessed conveniently.
>> +
>> +	  Message drivers act as engines to communicate with remote processors.
>> +	  Currently RPMON_QMI is available which uses QMI infrastructures
>> +	  on Qualcomm SoC Platforms.
>> +
>> +endmenu
>
>> diff --git a/drivers/rpmon/rpmon.c b/drivers/rpmon/rpmon.c
>> new file mode 100644
>> index 000000000000..65aab4de6733
>> --- /dev/null
>> +++ b/drivers/rpmon/rpmon.c
>> @@ -0,0 +1,505 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2020 Vivo Communication Technology Co. Ltd.
>> + * Copyright (C) 2020 Wang Wenhu <wenhu.wang@vivo.com>
>> + * 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 wording 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,

Addressed in v2

>
>> + * actions like connection status check, and so on. Enhancements can
>> + * be made upon current implementation.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/cdev.h>
>> +#include <linux/rpmon.h>
>> +
>> +#define RPMON_MAX_DEVICES	(1U << MINORBITS)
>> +#define RPMON_NAME			"rpmon"
>> +
>> +static int rpmon_major;
>> +static struct cdev *rpmon_cdev;
>> +static DEFINE_IDR(rpmon_idr);
>> +static const struct file_operations rpmon_fops;
>> +
>> +/* Protect idr accesses */
>> +static DEFINE_MUTEX(minor_lock);
>> +
>
>[snip]
>
>> +/**
>> + * rpmon_event_notify - trigger an notify event
>> + * @info:  RPMON device capabilities
>> + * @event: RPMON event to be triggered
>> + */
>> +void rpmon_event_notify(struct rpmon_info *info, u32 event)
>> +{
>> +	struct rpmon_device *rpmondev = info->rpmon_dev;
>> +
>> +	if (event >= RPMON_EVENT_MAX) {
>> +		pr_err("Error un-supported rpmon event %d", event);
>
>		              unsupported

Addressed in v2

>
>> +		return;
>> +	}
>> +
>> +	atomic_set(&rpmondev->event, RPMON_EVENT(event));
>> +	wake_up_interruptible(&rpmondev->wait);
>> +	kill_fasync(&rpmondev->async_queue, SIGIO, POLL_IN);
>> +}
>> +EXPORT_SYMBOL_GPL(rpmon_event_notify);
>
>[snip]
>
>> +/**
>> + * rpmon_register_device - register a new rpmon interface device
>> + * @owner:	module that creates the new device
>> + * @parent:	parent device
>> + * @info:	romon device capabilities
>
>		s/romon/rpmon/

Addressed in v2

>
>> + *
>> + * returns zero on success or a negative error code.
>
>use kernel-doc notation:
>
>    * return: zero on success or a negative error code.
>

Addressed in v2

>> + */
>> +int __rpmon_register_device(struct module *owner,
>> +			    struct device *parent,
>> +			    struct rpmon_info *info)
>> +{
>> +	struct rpmon_device *rpmondev;
>> +	int ret = 0;
>> +
>> +	if (!rpmon_class_registered)
>> +		return -EPROBE_DEFER;
>> +
>> +	if (!parent || !info || !info->name || !info->version)
>> +		return -EINVAL;
>> +
>> +	info->rpmon_dev = NULL;
>> +
>> +	rpmondev = kzalloc(sizeof(*rpmondev), GFP_KERNEL);
>> +	if (!rpmondev)
>> +		return -ENOMEM;
>> +
>> +	rpmondev->owner = owner;
>> +	rpmondev->info = info;
>> +	mutex_init(&rpmondev->info_lock);
>> +	init_waitqueue_head(&rpmondev->wait);
>> +	atomic_set(&rpmondev->event, 0);
>> +
>> +	ret = rpmon_get_minor(rpmondev);
>> +	if (ret) {
>> +		kfree(rpmondev);
>> +		return ret;
>> +	}
>> +
>> +	device_initialize(&rpmondev->dev);
>> +	rpmondev->dev.devt = MKDEV(rpmon_major, rpmondev->minor);
>> +	rpmondev->dev.class = &rpmon_class;
>> +	rpmondev->dev.parent = parent;
>> +	rpmondev->dev.release = rpmon_device_release;
>> +	dev_set_drvdata(&rpmondev->dev, rpmondev);
>> +
>> +	ret = dev_set_name(&rpmondev->dev, RPMON_NAME"%d", rpmondev->minor);
>> +	if (ret)
>> +		goto err_device_create;
>> +
>> +	ret = device_add(&rpmondev->dev);
>> +	if (ret)
>> +		goto err_device_create;
>> +
>> +	if (rpmondev->info->rpmon_dev_add_attrs) {
>> +		ret = rpmondev->info->rpmon_dev_add_attrs(rpmondev);
>> +		if (ret)
>> +			goto err_dev_add_attrs;
>> +	}
>> +
>> +	info->rpmon_dev = rpmondev;
>> +
>> +	return 0;
>> +
>> +err_dev_add_attrs:
>> +	device_del(&rpmondev->dev);
>> +err_device_create:
>> +	rpmon_free_minor(rpmondev);
>> +	put_device(&rpmondev->dev);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(__rpmon_register_device);
>
>[snip]
>
>> +module_exit(rpmon_exit);
>> +
>> +MODULE_AUTHOR("Wang Wenhu");
>
>Please add email address in the MODULE_AUTHOR() string.
>About 3/4 of all uses of MODULE_AUTHOR() do so.
>

Addressed in v2

>> +MODULE_DESCRIPTION("Remote Processor Monitor Core Framework");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/rpmon.h b/include/linux/rpmon.h
>> new file mode 100644
>> index 000000000000..40983a3b5655
>> --- /dev/null
>> +++ b/include/linux/rpmon.h
>> @@ -0,0 +1,68 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (C) 2020 Vivo Communication Technology Co. Ltd.
>> + * Copyright (C) 2020 Wang Wenhu <wenhu.wang@vivo.com>
>> + * All rights reserved.
>> + */
>> +
>> +#ifndef RPMON_H
>> +#define RPMON_H
>> +
>> +#include <net/sock.h>
>> +
>> +/* RPMON action would be taken */
>> +enum rpmon_exec {
>> +	RPMON_EXEC_CHECK_CONN = 0,
>> +	RPMON_EXEC_MAX,
>> +};
>> +
>> +/* RPMON events that may be notified */
>> +enum rpmon_event {
>> +	RPMON_EVENT_CHKCONN_FAIL = 0,
>> +	RPMON_EVENT_REGISTER,
>> +	RPMON_EVENT_MAX,
>> +};
>> +
>> +#define RPMON_EVENT(x)	(0x1 << x)
>> +#define RPMON_ACTION(x)	(0x1 << x)
>
>Unless you are very sure that 'x' above is never more than a simple
>expression, you should put x in parentheses, like so:
>

Addressed in v2

>> +#define RPMON_EVENT(x)	(1 << (x))
>> +#define RPMON_ACTION(x)	(1 << (x))
>
>so that there cannot be any operator precedence problems.
>
>
>thanks.
>-- 
>~Randy
>

thanks, 
Wenhu