Message-ID: <X/cw+7Uq+Slg45fe@kroah.com>
Date:   Thu, 7 Jan 2021 17:04:11 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Cristian Marussi <cristian.marussi@....com>
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        sudeep.holla@....com, lukasz.luba@....com,
        Jonathan.Cameron@...wei.com, arnd@...db.de, robh@...nel.org
Subject: Re: [PATCH v4 2/2] firmware: arm_scmi: Add SCMI System Power Control
 driver

On Tue, Jan 05, 2021 at 01:09:45PM +0000, Cristian Marussi wrote:
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 3f14dffb9669..2b39453e9dd1 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -40,6 +40,18 @@ config ARM_SCMI_POWER_DOMAIN
>  	  will be called scmi_pm_domain. Note this may needed early in boot
>  	  before rootfs may be available.
>  
> +config ARM_SCMI_POWER_CONTROL
> +	bool "SCMI system power control driver"
> +	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
> +	default n

n is always the default, no need to list it here.

> +	help
> +	  This enables System Power control logic which binds system shutdown or
> +	  reboot actions to SCMI System Power notifications generated by SCP
> +	  firmware.
> +
> +	  Graceful requests' methods and timeout and can be configured using
> +	  a few available module parameters.
> +
>  config ARM_SCPI_PROTOCOL
>  	tristate "ARM System Control and Power Interface (SCPI) Message Protocol"
>  	depends on ARM || ARM64 || COMPILE_TEST
> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> index 6a2ef63306d6..ddddb4636ebd 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -9,3 +9,4 @@ scmi-module-objs := $(scmi-bus-y) $(scmi-driver-y) $(scmi-protocols-y) \
>  		    $(scmi-transport-y)
>  obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-module.o
>  obj-$(CONFIG_ARM_SCMI_POWER_DOMAIN) += scmi_pm_domain.o
> +obj-$(CONFIG_ARM_SCMI_POWER_CONTROL) += scmi_power_control.o
> diff --git a/drivers/firmware/arm_scmi/scmi_power_control.c b/drivers/firmware/arm_scmi/scmi_power_control.c
> new file mode 100644
> index 000000000000..216c2f031111
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/scmi_power_control.c
> @@ -0,0 +1,359 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SCMI Generic SystemPower Control driver.
> + *
> + * Copyright (C) 2020-2021 ARM Ltd.
> + */
> +/**
> + * DOC: Theory of operation

What does "DOC:" provide?  Did you tie this into the kernel doc build
system?  If not, then what is this tag for?

> + *
> + * In order to handle platform originated SCMI SystemPower requests (like
> + * shutdowns or cold/warm resets) we register an SCMI Notification notifier
> + * block to react when such SCMI SystemPower events are emitted.
> + *
> + * Once such a notification is received we act accordingly to perform the
> + * required system transition depending on the kind of request.
> + *
> + * By default graceful requests are routed to userspace through the same
> + * API methods (orderly_poweroff/reboot()) used by ACPI when handling ACPI
> + * Shutdown bus events: alternatively, by properly setting a couple of module
> + * parameters (scmi_graceful_*_signum), it is possible to choose to use signals
> + * to CAD pid as a mean to communicate such graceful requests to userspace.
> + *
> + * Forceful requests, instead, will simply cause an immediate emergency_sync()
> + * and subsequent Kernel-only shutdown/reboot.
> + *
> + * Additionally, setting scmi_graceful_request_tmo_ms to a non-zero value, it
> + * is possible to convert a timed-out graceful request to a forceful one.
> + *
> + * The assumption here is that even graceful requests can be upper-bound by a
> + * maximum final timeout strictly enforced by the platform itself which can
> + * ultimately cut the power off at will anytime: in order to avoid such extreme
> + * scenario, when scmi_graceful_request_tmo_ms is non-zero, we track progress of
> + * graceful requests through the means of a reboot notifier converting timed-out
> + * graceful requests to forceful ones: in such a way at least we perform a
> + * clean sync and shutdown/restart before the power is cut.
> + *
> + * Given that such platform hard-timeout, if present, is anyway highly platform
> + * and event specific and not exposed at run-time by the SCMI SytemPower
> + * protocol, the parameter scmi_graceful_request_tmo_ms defaults to zero.
> + */
> +
> +#define pr_fmt(fmt) "SCMI SystemPower - " fmt

drivers should not need this, and even if they do, that's a HUGE prefix,
be more terse please.  Always use dev_*() calls instead of pr_() as you
have access to a device at all times, right?  If not, then this isn't a
driver :)

> +enum scmi_syspower_state {
> +	SCMI_SYSPOWER_IDLE,
> +	SCMI_SYSPOWER_IN_PROGRESS,
> +	SCMI_SYSPOWER_REBOOTING
> +};
> +
> +static enum scmi_syspower_state scmi_state;
> +/* Protect access to scmi_state */
> +static DEFINE_MUTEX(scmi_state_mtx);

Why does this only handle "one" state and device?  Shouldn't this all be
per-device?

> +
> +static enum scmi_system_events scmi_required_transition = SCMI_SYSTEM_MAX;
> +
> +static unsigned int scmi_graceful_poweroff_signum;
> +static unsigned int scmi_graceful_reboot_signum;
> +static unsigned int scmi_graceful_request_tmo_ms;

why "unsigned int"?

> +
> +static struct timer_list scmi_request_timer;
> +static struct work_struct scmi_forceful_work;
> +
> +/**
> + * scmi_reboot_notifier  - A reboot notifier to catch an ongoing successful
> + * system transition
> + * @nb: Reference to the related notifier block
> + * @reason: The reason for the ongoing reboot
> + * @__unused: The cmd being executed on a restart request (unused)
> + *
> + * When an ongoing system transition is detected, compatible with the one
> + * requested by SCMI, cancel the timer work.
> + * This is registered only when a valid SCMI SystemPower transition has been
> + * received and scmi_graceful_request_tmo_ms was non-zero.
> + *
> + * Return: NOTIFY_OK in any case
> + */
> +static int scmi_reboot_notifier(struct notifier_block *nb,
> +				unsigned long reason, void *__unused)
> +{
> +	mutex_lock(&scmi_state_mtx);
> +	switch (reason) {
> +	case SYS_HALT:
> +	case SYS_POWER_OFF:
> +		if (scmi_required_transition == SCMI_SYSTEM_SHUTDOWN)
> +			scmi_state = SCMI_SYSPOWER_REBOOTING;
> +		break;
> +	case SYS_RESTART:
> +		if (scmi_required_transition == SCMI_SYSTEM_COLDRESET ||
> +		    scmi_required_transition == SCMI_SYSTEM_WARMRESET)
> +			scmi_state = SCMI_SYSPOWER_REBOOTING;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (scmi_state == SCMI_SYSPOWER_REBOOTING) {
> +		pr_debug("Reboot in progress...cancel timer.\n");

use dev_dbg() and pass it your device pointer please.

Same for all usages of pr_*() in the driver.

> +static int scmi_syspower_probe(struct scmi_device *sdev)
> +{
> +	int ret;
> +	struct scmi_handle *handle = sdev->handle;
> +	struct notifier_block *userspace_nb;
> +
> +	if (!handle)
> +		return -ENODEV;
> +
> +	userspace_nb = devm_kzalloc(&sdev->dev, sizeof(*userspace_nb),
> +				    GFP_KERNEL);
> +	if (!userspace_nb)
> +		return -ENOMEM;
> +
> +	userspace_nb->notifier_call = &scmi_userspace_notifier;
> +	ret = handle->notify_ops->register_event_notifier(handle,
> +						SCMI_PROTOCOL_SYSTEM,
> +					SCMI_EVENT_SYSTEM_POWER_STATE_NOTIFIER,
> +					    NULL, userspace_nb);

Crazy indentation, checkpatch was ok with this?

> +module_param(scmi_graceful_request_tmo_ms, uint, 0644);
> +MODULE_PARM_DESC(scmi_graceful_request_tmo_ms,
> +		 "Maximum time(ms) allowed to userspace for complying to the request. Unlimited if zero.");
> +
> +module_param(scmi_graceful_poweroff_signum, uint, 0644);
> +MODULE_PARM_DESC(scmi_graceful_poweroff_signum,
> +		 "Signal to request graceful poweroff to CAD process. Ignored if zero.");
> +
> +module_param(scmi_graceful_reboot_signum, uint, 0644);
> +MODULE_PARM_DESC(scmi_graceful_reboot_signum,
> +		 "Signal to request graceful reboot to CAD process. Ignored if zero.");

This is not the 1990's, please do not add module parameters.  Make this
"just work", or worst case, sysfs attributes.

thanks,

greg k-h

Powered by blists - more mailing lists