[PATCH] powerpc: add ibm,suspend-me rtas interface

Dave C Boutcher sleddog at us.ibm.com
Thu Jan 12 04:43:53 EST 2006


On Wed, Jan 11, 2006 at 11:12:52AM -0600, Olof Johansson wrote:
> On Wed, Jan 11, 2006 at 03:56:33AM -0600, Dave C Boutcher wrote:
> 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.

Sigh...I'll fix and respin.

> 
> If you use Vim, the following in your .vimrc will make some of them
> stand out easily:

VIM!!! you heretic.

> 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.

This suspend is not (sigh) the same as linux suspend.  I might have 
tried to rename it something different, but the RTAS call is 
"ibm,suspend-me".  This is going to be driven by the HMC ultimately.

This function doesn't preclude someone implementing one of the Linux
swsusp versions on Power (and in fact Santi was working on that last 
year.)

> > + ***************************************************************************
> > + * 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. :-)

I assumed EVERYONE religiously reads the legal verbage at the top of the
file.  Thats what we tell the lawyers anyways.  If deposed, I will
mention your cavalier attitude towards legal text.

> 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.

Hrm...ok.

> Why?!

Because I'm paranoid about everyone accessing that shared waiting flag.

> What's wrong with "sprintf(page, "0\n");" ?

Nothing, but there's another status function thats going to show up and
return status here.  I'll just simplify the call until I implement that.

The rest of the comments are fair enough and I'll fix them up.  Thanks
for the review.

-- 
Dave Boutcher



More information about the Linuxppc64-dev mailing list