Re: [PATCH v3] firmware: arm_scmi: Add SCMI System Power Control driver

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Nov 17, 2020 at 03:57:25PM +0000, Cristian Marussi wrote:
> +/**
> + * scmi_request_timeout  - On timeout trigger a forceful transition
> + * @t: The timer reference
> + *
> + * Actual work is deferred out of the timer IRQ context since shutdown/reboot
> + * code do not play well when !in_task().
> + */
> +static void scmi_request_timeout(struct timer_list *t)
> +{
> +	struct scmi_syspower_config *conf = from_timer(conf, t, request_timer);
> +
> +	dev_warn(conf->dev,
> +		 "SCMI Graceful System Transition request timed out !\n");

Don't be noisy please, what can a user do about this?


> +	atomic_set(&conf->status, SCMI_SYSPOWER_FORCING);
> +	/* Ensure atomic values are updated */
> +	smp_mb__after_atomic();
> +	schedule_work(&conf->forceful_work);

Um, what is that smp_mb__after_atomic() and the whole mess here for?

What exactly are you thinking this whole mess is needed for, and how is
it working and why not just use a simple lock that handles everything
instead?

> +}
> +
> +/**
> + * 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 requested
> + * one, cancel the timer work.
> + *
> + * Return: NOTIFY_OK in any case
> + */
> +static int scmi_reboot_notifier(struct notifier_block *nb,
> +				unsigned long reason, void *__unused)
> +{
> +	struct scmi_syspower_config *conf;
> +
> +	conf = container_of(nb, struct scmi_syspower_config, reboot_nb);
> +
> +	/* Ensure atomic values are updated */
> +	smp_mb__before_atomic();

What?

> +	if (unlikely(atomic_read(&conf->status) == SCMI_SYSPOWER_FORCING))
> +		return NOTIFY_OK;

Unless you can benchmark the benifit of using likely/unlikely, do not
use it, as the compiler/CPU will do it better for you.

> +
> +	switch (reason) {
> +	case SYS_HALT:
> +	case SYS_POWER_OFF:
> +		if (conf->required_state == SCMI_SYSTEM_SHUTDOWN)
> +			atomic_set(&conf->status, SCMI_SYSPOWER_INPROGRESS);

Why are you trying to use an atomic variable for a state machine?  Why
not a simple enum and a lock?

> +		break;
> +	case SYS_RESTART:
> +		if (conf->required_state == SCMI_SYSTEM_COLDRESET ||
> +		    conf->required_state == SCMI_SYSTEM_WARMRESET)
> +			atomic_set(&conf->status, SCMI_SYSPOWER_INPROGRESS);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	/* Ensure atomic values are updated */
> +	smp_mb__after_atomic();
> +	if (atomic_read(&conf->status) == SCMI_SYSPOWER_INPROGRESS) {
> +		dev_info(conf->dev,
> +			 "SCMI System State request in progress...\n");
> +		del_timer_sync(&conf->request_timer);
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static inline void scmi_send_cad_signal(struct device *dev, unsigned int sig)
> +{
> +	dev_info(dev, "SCMI Sending signal %d to CAD pid\n", sig);

When drivers work properly, they are quiet, don't be noisy.

You do that in many other places in this codebase, just remove them all,
or make them dev_dbg() if you really want to see them in the future.

> +
> +	kill_cad_pid(sig, 1);
> +}
> +
> +/**
> + * scmi_request_graceful_transition  - Request graceful SystemPower transition
> + * @conf: The current SystemPower configuration descriptor
> + *
> + * Initiates the required SystemPower transition, requesting userspace
> + * co-operation using the same orderly_ methods used by ACPI Shutdown event
> + * processing.
> + *
> + * This takes care also to register a reboot notifier and a timer callback in
> + * order to detect if userspace actions are taking too long; in such a case
> + * the timeout callback will finally perform a forceful transition, ignoring
> + * lagging userspace: the aim here is to at least perform an emergency_sync()
> + * and a clean shutdown or reboot transition when userspace is late, avoiding
> + * the brutal final power-cut from platform.
> + */
> +static void scmi_request_graceful_transition(struct scmi_syspower_config *conf)
> +{
> +	int ret;
> +	u32 status;
> +
> +	if (conf->required_state >= SCMI_SYSTEM_POWERUP) {
> +		dev_warn(conf->dev,
> +			 "Received unexpected SYSTEM POWER request: %d\n",
> +			 conf->required_state);
> +		return;
> +	}
> +
> +	status = atomic_cmpxchg(&conf->status, SCMI_SYSPOWER_IDLE,
> +				SCMI_SYSPOWER_SERVED);
> +	if (status != SCMI_SYSPOWER_IDLE)
> +		return;
> +
> +	ret = devm_register_reboot_notifier(conf->dev, &conf->reboot_nb);
> +	if (ret)
> +		dev_warn(conf->dev, "Cannot register reboot notifier !\n");

And you keep going?  Why?

> +
> +	INIT_WORK(&conf->forceful_work, scmi_forceful_work_func);
> +	conf->request_timer.expires = jiffies +
> +				msecs_to_jiffies(scmi_graceful_request_tmo_ms);
> +	timer_setup(&conf->request_timer, scmi_request_timeout, 0);
> +	add_timer(&conf->request_timer);
> +
> +	dev_info(conf->dev,
> +		 "Serving SCMI Graceful request: %d (timeout_ms:%d)\n",
> +		 conf->required_state, scmi_graceful_request_tmo_ms);

Again, be quiet.

> +
> +	switch (conf->required_state) {
> +	case SCMI_SYSTEM_SHUTDOWN:
> +		/*
> +		 * When received early at boot-time the 'orderly' part will
> +		 * fail due to the lack of userspace itself, but the force=true
> +		 * argument will anyway be able trigger a successful forced
> +		 * shutdown.
> +		 */
> +		if (!scmi_graceful_poweroff_signum)
> +			orderly_poweroff(true);
> +		else
> +			scmi_send_cad_signal(conf->dev,
> +					     scmi_graceful_poweroff_signum);
> +		break;
> +	case SCMI_SYSTEM_COLDRESET:
> +	case SCMI_SYSTEM_WARMRESET:
> +		if (!scmi_graceful_reboot_signum)
> +			orderly_reboot();
> +		else
> +			scmi_send_cad_signal(conf->dev,
> +					     scmi_graceful_reboot_signum);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +static void scmi_request_forceful_transition(struct scmi_syspower_config *conf)
> +{
> +	/* Ensure atomic values are updated */
> +	smp_mb__before_atomic();
> +	if (unlikely(atomic_read(&conf->status) == SCMI_SYSPOWER_INPROGRESS))
> +		return;
> +
> +	dev_info(conf->dev, "Serving SCMI FORCEFUL SystemPower request:%d\n",
> +		 conf->required_state);

{ssshhh}

> +
> +	emergency_sync();
> +	switch (conf->required_state) {
> +	case SCMI_SYSTEM_SHUTDOWN:
> +		kernel_power_off();
> +		break;
> +	case SCMI_SYSTEM_COLDRESET:
> +	case SCMI_SYSTEM_WARMRESET:
> +		kernel_restart(NULL);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +#define SCMI_IS_REQUEST_GRACEFUL(flags)		((flags) & BIT(0))
> +
> +/**
> + * scmi_userspace_notifier  - Notifier callback to act on SystemPower
> + * Notifications
> + * @nb: Reference to the related notifier block
> + * @event: The SystemPower notification event id
> + * @data: The SystemPower event report
> + *
> + * This callback is in charge of decoding the received SystemPower report
> + * and act accordingly triggering a graceful or forceful system transition.
> + *
> + * Return: NOTIFY_OK in any case
> + */
> +static int scmi_userspace_notifier(struct notifier_block *nb,
> +				   unsigned long event, void *data)
> +{
> +	struct scmi_system_power_state_notifier_report *er = data;
> +	struct scmi_syspower_config *conf;
> +
> +	if (unlikely(system_state > SYSTEM_RUNNING))
> +		return NOTIFY_OK;

no unlikely please.



> +
> +	conf = container_of(nb, struct scmi_syspower_config, userspace_nb);
> +	conf->required_state = er->system_state;
> +
> +	if (conf->required_state >= SCMI_SYSTEM_MAX)
> +		return NOTIFY_OK;
> +
> +	if (SCMI_IS_REQUEST_GRACEFUL(er->flags))
> +		conf->request_graceful_transition(conf);
> +	else
> +		conf->request_forceful_transition(conf);
> +
> +	return NOTIFY_OK;
> +}
> +
> +/**
> + * scmi_syspower_configure  - General SystemPower configuration init
> + * @dev: The associated struct device
> + *
> + * Return: SystemPower configuration descriptor on Success, NULL otherwise
> + */
> +static void *scmi_syspower_configure(struct device *dev)
> +{
> +	struct scmi_syspower_config *conf;
> +
> +	conf = devm_kzalloc(dev, sizeof(*conf), GFP_KERNEL);
> +	if (!conf)
> +		return NULL;
> +
> +	conf->dev = dev;
> +	conf->required_state = SCMI_SYSTEM_MAX;
> +	conf->request_graceful_transition = &scmi_request_graceful_transition;
> +	conf->request_forceful_transition = &scmi_request_forceful_transition;
> +	conf->userspace_nb.notifier_call = &scmi_userspace_notifier;
> +	conf->reboot_nb.notifier_call = &scmi_reboot_notifier;
> +	atomic_set(&conf->status, SCMI_SYSPOWER_IDLE);
> +	/* Ensure atomic values are updated */
> +	smp_mb__after_atomic();

Why is this needed?

thanks,

greg k-h