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

From: Greg KH
Date: Tue Nov 17 2020 - 11:21:17 EST


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