linux-kernel - RE: [PATCH v1 1/1] remoteproc: qcom: Add sysfs entry to detect device shutdown

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <DU0PR04MB94171ADF7D72882F0B88AF78884E9@DU0PR04MB9417.eurprd04.prod.outlook.com>
Date:   Thu, 22 Sep 2022 01:11:44 +0000
From:   Peng Fan <peng.fan@....com>
To:     Gokul krishna Krishnakumar <quic_gokukris@...cinc.com>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>,
        Mathieu Poirier <mathieu.poirier@...aro.org>
CC:     "linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
        "linux-remoteproc@...r.kernel.org" <linux-remoteproc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Trilok Soni <quic_tsoni@...cinc.com>,
        Satya Durga Srinivasu Prabhala <quic_satyap@...cinc.com>,
        Rajendra Nayak <quic_rjendra@...cinc.com>,
        Elliot Berman <quic_eberman@...cinc.com>,
        Guru Das Srinagesh <quic_gurus@...cinc.com>
Subject: RE: [PATCH v1 1/1] remoteproc: qcom: Add sysfs entry to detect device
 shutdown

Hi Gokul,

> Subject: [PATCH v1 1/1] remoteproc: qcom: Add sysfs entry to detect device
> shutdown
> 
> This change adds a sysfs entry which indicates whether the device is being
> shutdown to the qcom remoteproc drivers. This is going to be used in the
> following patches.

I have no knowledge of qcom platform, just a few generic comments:

I think it would be better if you post a link to give people a full picture on how
this going to work.

> 
> Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@...cinc.com>
> ---
>  drivers/remoteproc/qcom_common.c | 58
> ++++++++++++++++++++++++++++++++++++++++
>  drivers/remoteproc/qcom_common.h |  1 +
>  2 files changed, 59 insertions(+)
> 
> diff --git a/drivers/remoteproc/qcom_common.c
> b/drivers/remoteproc/qcom_common.c
> index 020349f..7959e96 100644
> --- a/drivers/remoteproc/qcom_common.c
> +++ b/drivers/remoteproc/qcom_common.c
> @@ -87,9 +87,32 @@ struct qcom_ssr_subsystem {
>  	struct list_head list;
>  };
> 
> +static struct kobject *sysfs_kobject;
> +bool qcom_device_shutdown_in_progress;
> +EXPORT_SYMBOL(qcom_device_shutdown_in_progress);
> +
>  static LIST_HEAD(qcom_ssr_subsystem_list);
>  static DEFINE_MUTEX(qcom_ssr_subsys_lock);
> 
> +static const char * const ssr_timeout_msg = "srcu notifier chain for
> +%s:%s taking too long";

I not see this variable is being used anywhere.

> +
> +static ssize_t qcom_rproc_shutdown_request_store(struct kobject *kobj,
> struct kobj_attribute *attr,
> +						 const char *buf, size_t
> count)
> +{
> +	bool val;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &val);
> +	if (ret)
> +		return ret;
> +
> +	qcom_device_shutdown_in_progress = val;
> +	pr_info("qcom rproc: Device shutdown requested: %s\n", val ?
> "true" : "false");

This is a sysfs write operation, how does it matter with device shutdown
in progress?

> +	return count;
> +}
> +static struct kobj_attribute shutdown_requested_attr =
> __ATTR(shutdown_in_progress, 0220, NULL,

How about DEVICE_ATTR_WO, but seems you use 0220, the generic
marco use 0200.

> +
> qcom_rproc_shutdown_request_store);
> +
>  static void qcom_minidump_cleanup(struct rproc *rproc)  {
>  	struct rproc_dump_segment *entry, *tmp; @@ -505,5 +528,40 @@
> void qcom_remove_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr
> *ssr)  }  EXPORT_SYMBOL_GPL(qcom_remove_ssr_subdev);
> 
> +static int __init qcom_common_init(void) {
> +	int ret = 0;
> +
> +	qcom_device_shutdown_in_progress = false;
> +
> +	sysfs_kobject = kobject_create_and_add("qcom_rproc",
> kernel_kobj);
> +	if (!sysfs_kobject) {
> +		pr_err("qcom rproc: failed to create sysfs kobject\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = sysfs_create_file(sysfs_kobject,
> &shutdown_requested_attr.attr);
> +	if (ret) {
> +		pr_err("qcom rproc: failed to create sysfs file\n");
> +		goto remove_kobject;
> +	}
> +
> +	return 0;
> +
> +	sysfs_remove_file(sysfs_kobject, &shutdown_requested_attr.attr);
> +remove_kobject:
> +	kobject_put(sysfs_kobject);
> +	return ret;
> +
> +}
> +module_init(qcom_common_init);
> +
> +static void __exit qcom_common_exit(void) {
> +	sysfs_remove_file(sysfs_kobject, &shutdown_requested_attr.attr);
> +	kobject_put(sysfs_kobject);
> +}
> +module_exit(qcom_common_exit);
> +
>  MODULE_DESCRIPTION("Qualcomm Remoteproc helper driver");
> MODULE_LICENSE("GPL v2");

As I recall, checkpatch would report GPL is enough, no need v2.

Regards,
Peng.

 diff --git
> a/drivers/remoteproc/qcom_common.h
> b/drivers/remoteproc/qcom_common.h
> index c35adf7..90b79ce 100644
> --- a/drivers/remoteproc/qcom_common.h
> +++ b/drivers/remoteproc/qcom_common.h
> @@ -34,6 +34,7 @@ struct qcom_rproc_ssr {  };
> 
>  void qcom_minidump(struct rproc *rproc, unsigned int minidump_id);
> +extern bool qcom_device_shutdown_in_progress;
> 
>  void qcom_add_glink_subdev(struct rproc *rproc, struct qcom_rproc_glink
> *glink,
>  			   const char *ssr_name);
> --
> 2.7.4