[PATCH] powerpc: add ibm,suspend-me rtas interface
Olof Johansson
olof at lixom.net
Thu Jan 12 04:12:52 EST 2006
On Wed, Jan 11, 2006 at 03:56:33AM -0600, Dave C Boutcher wrote:
> Create a proc file that lets us call the ibm,suspend-me
> RTAS call. This call has to be wrapped in some hypervisor
> calls that synchronize all the CPUs, so it can't be done
> from userland.
In general, you seem to have some whitespace issues -- 4 spaces instead
of tabs, 2 tabs instead of 1 in one place, and some trailing whitespace.
I won't comment on the specific instances but please clean them up.
If you use Vim, the following in your .vimrc will make some of them
stand out easily:
highlight RedundantWhitespace ctermbg=red guibg=red
match RedundantWhitespace /\s\+$\| \+\ze\t/
syntax on
(comes from Jeremy Kerr)
However, a bigger question is: why are you defining your own interface
for suspend? There's already at least one global interface for it
(/sys/power/state), can't you hook in there instead? This means we'll
have to carry another interface forever, since userspace tools will
depend on it.
> diff -uNr -X exclude-files linux-2.6.15/arch/powerpc/platforms/pseries/Makefile linux-2.6.15-vpm/arch/powerpc/platforms/pseries/Makefile
> --- linux-2.6.15/arch/powerpc/platforms/pseries/Makefile 2006-01-02 21:21:10.000000000 -0600
> +++ linux-2.6.15-vpm/arch/powerpc/platforms/pseries/Makefile 2006-01-10 14:32:56.000000000 -0600
> @@ -1,5 +1,5 @@
> obj-y := pci.o lpar.o hvCall.o nvram.o reconfig.o \
> - setup.o iommu.o ras.o rtasd.o
> + setup.o iommu.o ras.o rtasd.o sysstate.o
> obj-$(CONFIG_SMP) += smp.o
> obj-$(CONFIG_IBMVIO) += vio.o
> obj-$(CONFIG_XICS) += xics.o
> diff -uNr -X exclude-files linux-2.6.15/arch/powerpc/platforms/pseries/sysstate.c linux-2.6.15-vpm/arch/powerpc/platforms/pseries/sysstate.c
> --- linux-2.6.15/arch/powerpc/platforms/pseries/sysstate.c 1969-12-31 18:00:00.000000000 -0600
> +++ linux-2.6.15-vpm/arch/powerpc/platforms/pseries/sysstate.c 2006-01-11 02:17:22.000000000 -0600
> @@ -0,0 +1,157 @@
> +/*
> + * arch/ppc64/sys-state.c - Logical Partition State
Huh? Filename is inconsistent. I'd say there's no need to keep it in the
file at all, it seems to vary if it's there or not in other places of
the kernel.
> + *
> + * Copyright (c) 2005 Dave Boutcher
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + ***************************************************************************
> + * This file handles the pSeries firmware supported "ibm,suspend-me" RTAS
> + * call. It can't be done from userland, since all of the CPUs must be first
> + * synchronized using the H_JOIN hypervisor call prior to making the
> + * ibm,suspend-me RTAS call.
> + ***************************************************************************
> + */
> +
Care to move that as a separate comment block below the GPL header? I
missed it the first read-through since I usually don't read the big GPL
block at the top anyway. :-)
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/smp.h>
> +#include <linux/proc_fs.h>
> +#include <asm/uaccess.h>
> +
> +#include <asm/rtas.h>
> +
> +static int suspend_me_tok = -1;
You init to -1 and check it against RTAS_UNKNOWN_SERVICE. Please use the
symbolic name at init too.
> +
> +/*
> + * We use "waiting" to indicate our state. As long
> + * as it is >0, we are still trying to all join up.
> + * If it goes to 0, we have successfully joined up and
> + * one thread got H_Continue. If any error happens,
> + * we set it to <0;
> + */
Seems to me that this comment can go right before the loop in the
function.
> +static void cpu_suspend(void *info)
> +{
> + long rc;
> + long flags;
> + long *waiting = (long *)info;
> + int status;
> +
> + local_irq_save(flags);
> + do {
> + rc = plpar_hcall_norets(H_JOIN);
> + mb();
> + } while ((rc == H_Success) && (*waiting > 0));
(((feels like) (reading) (lisp))) :-)
Also, there should be no need for a full mb(), should there? The hvcall
in itself should be syncronizing, but if it's not, then smp_rmb() should
do just fine.
> + if (rc == H_Success)
> + goto out;
> +
> + if (rc == H_Continue) {
> + *waiting = 0;
> + if (suspend_me_tok != RTAS_UNKNOWN_SERVICE) {
> + status = rtas_call(suspend_me_tok, 0, 1, NULL);
> + if (status != 0)
> + printk(KERN_ERR
> + "RTAS ibm,suspend-me failed: %i\n",
> + status);
You go through all of this, just to check if the rtas token is set?
Why don't you just abort in the beginning of the function if it's not
set instead?
> + }
> + } else {
> + *waiting = -EBUSY;
> + printk(KERN_ERR "Error on H_Join hypervisor call\n");
> + }
> +
> + out:
> + local_irq_restore(flags);
> + return;
> +}
> +
> +static int do_suspend(void)
> +{
> + int i;
> + long waiting = 1;
> +
> + smp_rmb();
Why?!
> +
> + /* Call function on all other CPUs */
> + if (on_each_cpu(cpu_suspend,
> + &waiting,
> + 1, 0)) {
This can be done on line line, you've got 80 columns.
> + printk(KERN_ERR "Error calling on_each_cpu!\n");
> + /* TODO: Prod everyone here */
Todo? Why not just do a return value variable and a goto out (that's
right before the prod loop)?
> + return -EINVAL;
> + }
> +
> + if (waiting != 0) {
> + printk(KERN_ERR "Error doing global join!\n");
> + }
> +
> + /* Prod each CPU. This won't hurt, and will wake
> + * anyone we successfully put to sleep with H_Join
> + */
> + for_each_cpu(i)
> + plpar_hcall_norets(H_PROD,i);
> +
> + return 0;
> +}
> +
> +static int read_proc(char *page, char **start, off_t off,
> + int count, int *eof, void *data)
> +{
> + int len;
> +
> + len = sprintf(page, "%d\n",
> + 0);
What's wrong with "sprintf(page, "0\n");" ?
Also, no eof handling, no checking of count, etc.
> +
> + return len;
> +}
> +
> +static int write_proc(struct file *file, const char __user *buffer,
> + unsigned long count, void *data)
> +{
> + char command[32];
> +
> + if (count > sizeof(command))
> + count = sizeof(command);
> +
> + if (copy_from_user(command, buffer, count))
> + return -EFAULT;
> +
> + if (strncmp(command, "suspend", sizeof(command)) == 0) {
> + do_suspend();
> + }
What happens if someone echos suspend\n into the proc file?
> + return count;
> +}
> +
> +static int __init sys_state_init(void)
> +{
> + struct proc_dir_entry *entry;
> + suspend_me_tok = rtas_token("ibm,suspend-me");
> + if (suspend_me_tok == RTAS_UNKNOWN_SERVICE)
> + return 0;
> +
> + entry = create_proc_entry("ppc64/rtas/platform_state", S_IWUSR, NULL);
> + if (!entry) {
> + printk(KERN_ERR "Failed to create platform_state proc entry\n");
> + return 0;
> + }
> +
> + entry->read_proc = read_proc;
> + entry->write_proc = write_proc;
> +
> + return 0;
> +}
> +
> +arch_initcall(sys_state_init);
>
> --
> Dave Boutcher
> _______________________________________________
> Linuxppc64-dev mailing list
> Linuxppc64-dev at ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc64-dev
More information about the Linuxppc64-dev
mailing list