Subject: Re: Help port swsusp to ppc.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
To: Pavel Machek <pavel@ucw.cz>
Cc: ncunningham@users.sourceforge.net, Hugang <hugang@soulinfo.com>,
       ncunningham@clear.net.nz,
       Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
       debian-powerpc@lists.debian.org
In-Reply-To: <20040120204407.GA9749@elf.ucw.cz>
References: <20040119105237.62a43f65@localhost>
	 <1074483354.10595.5.camel@gaston> <1074489645.2111.8.camel@laptop-linux>
	 <1074490463.10595.16.camel@gaston> <1074534964.2505.6.camel@laptop-linux>
	 <1074549790.10595.55.camel@gaston>  <20040120204407.GA9749@elf.ucw.cz>
Content-Type: text/plain
Message-Id: <1074635683.795.43.camel@gaston>
Mime-Version: 1.0
Date: Wed, 21 Jan 2004 08:54:44 +1100
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2654
Lines: 56


> FYI, this is that "ugly C-generated assembly" we are talking about. I
> do not think it is so bad.

The x86 version has been cleaned up and isn't _that_ bad, though it
could definitely use some comments and I don't like the "Lxxx" labels,
I'd rather either use number with the "nb" or "nf" GAS constructs or
use real words labels. Looking at it though, I fail to see the need
to get it generated by gcc in the first place :)

The PPC version that was proposed is horrible.

> FYI, there are exactly 6 variables in "nosave" section. Two loop
> variables you can see in above code, one spinlock, number of pages to
> save, pointer to directory of pages to be copied, and its length.
> 
> I could probably move spinlock and length of pgdir out of there...

That's not too much, you could probably afford having a one page header
to the suspend image with those informations and the page copy loop
(provided by the suspended kernel so you don't have _any_ compatibility
issue and can even do it from the bootloader one day...)
 
> > device_suspend is, imho, hairy too. We have some semantics that need
> > cleanup here, I'll have to talk to Patrick about them. Putting devices
> > to an idle state is what you need and what kexec need, and doesn't mean
> > putting them to _sleep_. Or maybe we could pass a specific state to
> 
> Okay, I can agree that putting them into sleep is not ideal [it
> puzzles users, at least]. But it is quite simple and should work
> okay. I do not want another round of device_suspend changes in
> 2.6.X... [Well, perhaps if it was done in right&compatible way (tm),
> it would be acceptable...]

We already have the shutdown() callback or we can simply use device_suspend()
with a D1 parameter instead of D3 or D4. That's what I'd do...

Looking at swsusp code in current 2.6, when do you do that pass ? On the
shutdown pass, you call devices_suspend(4); which is fine. But I don't see
where you call devices_suspend(X) on the resume path. IMHO, that should be
done from the boot kernel after loading the suspend image and before
starting the resume process. Your mdelay(1000) for waiting for DMA to settle
down is PLAIN WRONG imho, even dangerous. (And typically, an USB controller
will still be hapilly be DMA'ing all over your memory). That's what I call
idling devices at this point, and that's where I'd call devices_suspend(1),
and we should do that for kexec too.

Ben.


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/