Re: [v5] powerpc/powernv: Add poweroff (EPOW, DPO) events support for PowerNV platform

On Mon, 2015-18-05 at 15:18:04 UTC, Vipin K Parashar wrote:

This patch adds support for FSP EPOW (Early Power Off Warning) and

Please spell out the acronyms the first time you use them, including FSP.

DPO (Delayed Power Off) events for PowerNV platform. EPOW events are

                                    ^
				    the

generated by SPCN/FSP due to various critical system conditions that

SPCN?

need system shutdown. Few examples of these conditions are high

                        ^
s/need/require/ ?       A few

ambient temperature or system running on UPS power with low UPS battery.
DPO event is generated in response to admin initiated system request.

Blank line between paragraphs please.

	Upon receipt of EPOW and DPO events host kernel invokes

                                            ^
					    the host kernel

orderly_poweroff for performing graceful system shutdown. System admin

I like it if you spell functions with a trailing () to make it clear they are
functions, so this would be "orderly_powerof()".

can also add systemd service shutdown scripts to perform any specific
actions like graceful guest shutdown upon system poweroff. libvirt-guests
is systemd service available on recent distros for management of guests
at system start/shutdown time.

This last part about the scripts is not relevant to the kernel patch so just
leave it out please.

I don't see this in the skiboot version of opal-api.h ?

They should be kept in sync.

If it's a Linux only define it should go in opal.h

I don't see the last three of these enums used at all, so please drop them.

Has the signature of this function really changed or was it just wrong before?

If it's changed how do we know we're running on a version of OPAL that supports
the two argument version?

The parameter names don't seem very clear either. status is actually a pointer
to an array of "statuses", and length is the number of entries in that array.

Also you removed the endian annotations but then you pass it __be16 values, so
that looks incorrect, you should be using __be16 here.

+int64_t opal_get_dpo_status(int64_t *dpo_timeout);

Similarly this should be __be64 AFAICS.

Please don't shout, "opal-power" is fine.

 #include <linux/kernel.h>
+#include <linux/spinlock.h>
+#include <linux/timer.h>

Don't think you need those?

 #include <linux/reboot.h>
-#include <linux/notifier.h>
-

I think you DO need notifier.h.

+#include <linux/of.h>
 #include <asm/opal.h>
 #include <asm/machdep.h>
 
-#define SOFT_OFF 0x00
-#define SOFT_REBOOT 0x01
+/* Get EPOW status */
+static bool get_epow_status(void)

This is not a great name, "get" implies it gives you something back, but it
doesn't it just tells you true or false.

So maybe epow_event_pending() ?

+{
+	int i;
+	u16 num_classes;
+	__be16 epow_classes;

I think this would be cleaner if you just had a single num_classes and you
endian swap in the one place you use it.

+	__be16 opal_epow_status[OPAL_SYSEPOW_MAX] = {0};
+
+	/* Send kernel EPOW classes supported info to OPAL */
+	epow_classes = cpu_to_be16(OPAL_SYSEPOW_MAX);
+
+	/* Get EPOW events information from OPAL */
+	opal_get_epow_status(opal_epow_status, &epow_classes);

This could fail.

+
+	/* Look for EPOW events present */
+	num_classes = be16_to_cpu(epow_classes);
+	for (i = 0; i < num_classes; i++) {
+		if (be16_to_cpu(opal_epow_status[i]))
+			return true;
+	}
+
+	return false;
+}
+
+/* Process existing EPOW, DPO events */
+static void process_existing_poweroff_events(void)
+{
+	int rc;
+	__be64 opal_dpo_timeout;
 
+	/* Check for DPO event */
+	rc = opal_get_dpo_status(&opal_dpo_timeout);
+	if (rc != OPAL_WRONG_STATE) {
+		pr_info("Existing DPO event detected. Powering off system\n");
+		goto poweroff;
+	}
+
+	/* Check for EPOW event */
+	if (get_epow_status()) {
+		pr_info("Existing EPOW event detected. Powering off system");
+		goto poweroff;
+	}
+	return;
+
+poweroff:
+	orderly_poweroff(true);

I don't like that much, you shouldn't need to use goto for such simple logic.

Can you create a single function, maybe called event_pending(), and have it
check both EPOW and DPO and return a bool if there's any kind of event pending.

Then this can just become:

  if (event_pending())
  	orderly_poweroff(true);

+}
+
+/* OPAL EPOW, DPO event notifier */
+static int opal_epow_dpo_event(struct notifier_block *nb,
+				unsigned long msg_type, void *msg)
+{
+	switch (msg_type) {
+	case OPAL_MSG_EPOW:
+		pr_info("EPOW msg received. Powering off system\n");
+		break;
+	case OPAL_MSG_DPO:
+		pr_info("DPO msg received. Powering off system\n");
+		break;
+	default:
+		pr_err("Unknown message type %lu\n", msg_type);
+		return 0;
+	}
+
+	orderly_poweroff(true);
+	return 0;
+}

Why do we need a separate notifier function? Can't this just be folded into
opal_power_control_event() ?

Can you make that a bool and call it "supported".

+	struct device_node *node_epow;

It's typical to just call it "np".

 
+	/* Register OPAL power-control events notifier */
 	ret = opal_message_notifier_register(OPAL_MSG_SHUTDOWN,
-					     &opal_power_control_nb);
-	if (ret) {
-		pr_err("%s: Can't register OPAL event notifier (%d)\n",
-				__func__, ret);
-		return ret;
+				&opal_power_control_nb);
+	if (ret)
+		pr_err("Power-control events notifier registration "
+				"failed, ret = %d\n", ret);

Please don't split the string, and similarly below.

+
+	/* Determine EPOW, DPO support in hardware. */
+	node_epow = of_find_node_by_path("/ibm,opal/epow");
+	if (node_epow) {
+		epow_dpo_supported = of_device_is_compatible(node_epow,
+				"ibm,opal-v3-epow");
+		of_node_put(node_epow);
 	}
 
+	if (epow_dpo_supported)
+		pr_info("OPAL EPOW, DPO support detected.\n");
+	else
+		return 0;

Clearer as:

	if (!supported)
		return 0;

	pr_info("OPAL EPOW, DPO support detected.\n");

+
+	/* Register EPOW event notifier */
+	ret = opal_message_notifier_register(OPAL_MSG_EPOW,
+			&opal_epow_nb);
+	if (ret)
+		pr_err("EPOW event notifier registration failed, "
+				"ret = %d\n", ret);
+
+	/* Register DPO event notifier */
+	ret = opal_message_notifier_register(OPAL_MSG_DPO,
+			&opal_dpo_nb);
+	if (ret)
+		pr_err("DPO event notifier registration failed, "
+				"ret = %d\n", ret);
+
+	/* Check for any existing EPOW or DPO events. */
+	process_existing_poweroff_events();
+
+	pr_info("Poweroff events support initialized\n");
+
 	return 0;
 }
+

No extra blank line thanks.

 machine_subsys_initcall(powernv, opal_power_control_init);

cheers