Re: [PATCH] Combined APM patch

Re: [PATCH] Combined APM patch

[Thomas Hood]

>> Is it not good practice to note when the code _assumes_ zero-
>> initialization?  I have seen comments like these elsewhere in
>> the kernel sources.
>
> Comments should _not_ echo code.  One should assumes that the reader
> knows the language.  Instead, comments should explain _what the
> purpose_  of that line of code or section of code is if it is not
> obvious to those that the comments are written to.

I agree that comments should not try to explain C to the
reader.  Comments should provide additional information,
such as telling the reader what the code is supposed to
do (as opposed to what it actually does, which may or may
not be the same thing).

In the line
    static int suspends_pending; /* = 0 */
the comment is not there to tell the reader that the variable
is initialized to zero.  It is there to tell the reader that
the variable _needs to be_ initialized to zero in order for
the code to work properly.  This is useful information,
because if someone later wants to modify the code to make
this variable non-static, the comment tells that person that
the variable will need an initializer.

--
Thomas

Re: [PATCH] Combined APM patch

[Russell King]

On Fri, Jan 11, 2002 at 10:22:24AM -0500, Thomas Hood wrote:
> This is useful information,
> because if someone later wants to modify the code to make
> this variable non-static, the comment tells that person that
> the variable will need an initializer.

Whether a variable is static or not doesn't change whether it ends up in
the bss segment or not.

/* top level */
static int foo;
int bar;

Both foo and bar will be initialised to 0 - since they're both placed
into the BSS segment (or maybe its common subsection):

$ gcc -S -o - t.c
@ Generated by gcc 2.96 20000731 (Red Hat Linux 7.1 2.96-80) for ARM/elf
	.file	"t.c"
gcc2_compiled.:
.bss
	.align	2
foo:
	.space	4
	.comm	bar, 4	@ 4

$ gcc -S -o - t.c -fno-common
@ Generated by gcc 2.96 20000731 (Red Hat Linux 7.1 2.96-80) for ARM/elf
	.file	"t.c"
gcc2_compiled.:
.bss
	.align	2
foo:
	.space	4
	.global	bar
	.align	2
	.type	 bar,object
	.size	 bar,4
bar:
	.space	4


-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [PATCH] Combined APM patch

[Thomas Hood]

On Fri, 2002-01-11 at 10:40, Russell King wrote:
> On Fri, Jan 11, 2002 at 10:22:24AM -0500, Thomas Hood wrote:
> > if someone later wants to modify the code to make
> > this variable non-static, the comment tells that person that
> > the variable will need an initializer.
> 
> Whether a variable is static or not doesn't change whether it ends up in
> the bss segment or not.

It does make a difference if the variable definitions are inside
a function; the non-static variable is on the stack and is not
initialized to zero.

I understand that every static or top-level global variable
is initialized to zero; but is it not useful to note when
the code _relies upon_ this zero-initialization?  

Re: [PATCH] Combined APM patch

[Russell King]

On Fri, Jan 18, 2002 at 05:43:48AM -0500, Thomas Hood wrote:
> On Fri, 2002-01-11 at 10:40, Russell King wrote:
> > On Fri, Jan 11, 2002 at 10:22:24AM -0500, Thomas Hood wrote:
> > > if someone later wants to modify the code to make
> > > this variable non-static, the comment tells that person that
> > > the variable will need an initializer.
> > 
> > Whether a variable is static or not doesn't change whether it ends up in
> > the bss segment or not.
> 
> It does make a difference if the variable definitions are inside
> a function; the non-static variable is on the stack and is not
> initialized to zero.

I should really ignore this mail, but, sigh.

I know this.  I was commenting on your code and the comment you made which,
in the context you were applying it, wasn't correct.

Hope this clears up the confusion.

> I understand that every static or top-level global variable
> is initialized to zero; but is it not useful to note when
> the code _relies upon_ this zero-initialization?  

Of course, I'm not disputing that.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [PATCH] Combined APM patch

[Bob Toxen]

> Just browsing the diff between my patch and Stephen's, I have
> a couple of questions.

> < static int			suspends_pending; /* = 0 */
> ---
> > static int			suspends_pending;

> Is it not good practice to note when the code _assumes_ zero-
> initialization?  I have seen comments like these elsewhere in
> the kernel sources.

Comments should _not_ echo code.  One should assumes that the reader
knows the language.  Instead, comments should explain _what the purpose_
of that line of code or section of code is if it is not obvious to those
that the comments are written to.

Thus, the following should be considered correct:

static int			suspends_pending;

Both of the following lines have superfluous code or commants and should
be avoided:

static int			suspends_pending; /* = 0 */
static int			suspends_pending = 0;

Likewise, parentheses where none are needed just get in the way of useful
stuff and slows down understanding the program.  Some parentheses in
complex expressions or when the precedence of operator evaluation is
confusing, such as bitwise ops can be helpful.

Btw, I have been writing system-level c code for 26 years and learned by
reading and modifying Unix system code at Berkeley.

Best regards,

Bob Toxen, CTO
Fly-By-Day Consulting, Inc.           "Experts in Linux & network security"
bob@cavu.com
http://www.cavu.com                   [Linux/Unix & Network Security Consulting]
http://www.realworldlinuxsecurity.com [My 5* book: "Real World Linux Security"]
http://www.cavu.com/sunset.html       [Sunset Computer]
Quality Linux, UNIX and network security and software consulting since 1990.

GPG Public key available at http://www.cavu.com/pubkey.txt (book@cavu.com)
  and at http://pgp5.ai.mit.edu/pks-commands.html#extract
  and on the CD-ROM that comes sealed and attached to Real World Linux Security
pub  1024D/E3A1C540 2000-06-21 Bob Toxen <book@cavu.com>
     Key fingerprint = 30BA AA0A 31DD B68B 47C9  601E 96D3 533D E3A1 C540
sub  2048g/03FFCCB9 2000-06-21

Re: [PATCH] Combined APM patch

[David Balazic]

Why is the APM_IOC_REJECT ioctl functionality still not merged ?
Does somebody hate it that much or are there any technical reasons ?

Party on,
-- 
David Balazic
--------------
"Be excellent to each other." - Bill S. Preston, Esq., & "Ted" Theodore Logan
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

[PATCH] Combined APM patch

[Stephen Rothwell]

Hi All,

This is my version of the combined APM patches;

	Change notification order so that user mode is notified
		before drivers of impending suspends.
	Move the idling back into the idle loop.
	A couple of small tidy ups.

See header comments for attributions.

This works for me (including as a module).

Please test and let me know - it seems to lower my power requirements
by about 10% on my Thinkpad (over stock 2.4.17).

http://www.canb.auug.org.au/~sfr/2.4.17-APM.1.diff

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

Re: [PATCH] Combined APM patch

[Thomas Hood]

Just browsing the diff between my patch and Stephen's, I have
a couple of questions.

< static int			suspends_pending; /* = 0 */
---
> static int			suspends_pending;

Is it not good practice to note when the code _assumes_ zero-
initialization?  I have seen comments like these elsewhere in
the kernel sources.

< 	static int use_apm_idle; /* = 0 */
< 	static unsigned int last_jiffies; /* = 0 */
< 	static unsigned int last_stime; /* = 0 */
---
> 	static int use_apm_idle = 0;
> 	static unsigned int last_jiffies = 0;
> 	static unsigned int last_stime = 0;

Are static variables defined within functions not initialized
to zero at load time, as global static variables are?

< 			ignore_sys_suspend = 0;
---
> 			waiting_for_resume = 0;

Don't you think "ignore_sys_suspend" is a name more consistent
with the other "ignore_yadda_yadda" variable names?  Minor issue.

Everything else looks good to me.

On Sun, 2002-01-06 at 23:52, Stephen Rothwell wrote:
> This is my version of the combined APM patches;
> 
> 	Change notification order so that user mode is notified
> 		before drivers of impending suspends.
> 	Move the idling back into the idle loop.
> 	A couple of small tidy ups.
> 
> See header comments for attributions.
> 
> This works for me (including as a module).
> 
> Please test and let me know - it seems to lower my power requirements
> by about 10% on my Thinkpad (over stock 2.4.17).
> 
> http://www.canb.auug.org.au/~sfr/2.4.17-APM.1.diff

The kernel compiles fine with your patch; I'll test over the
next few days.

Thanks
Thomas

Re: [PATCH] Combined APM patch

[Borsenkow Andrej]

On Пнд, 2002-01-07 at 07:52, Stephen Rothwell wrote:
> Hi All,
> 
> This is my version of the combined APM patches;
> 
> 	Change notification order so that user mode is notified
> 		before drivers of impending suspends.
> 	Move the idling back into the idle loop.
> 	A couple of small tidy ups.
> 
> See header comments for attributions.
> 
> This works for me (including as a module).
> 
> Please test and let me know - it seems to lower my power requirements
> by about 10% on my Thinkpad (over stock 2.4.17).
> 
> http://www.canb.auug.org.au/~sfr/2.4.17-APM.1.diff
> 

Sorry for delay.

The patch works just fine here. The only comment is not related to your
patch - before I did not use this interrupt counting and I have feeling
that CPU temp went down faster when system became idle. I still do not
understand what is achieved by it.

regards

-andrej

Re: [PATCH] Combined APM patch

[Borsenkow Andrej]

On Пнд, 2002-01-07 at 07:52, Stephen Rothwell wrote:
> Hi All,
> 
> This is my version of the combined APM patches;
> 
> 	Change notification order so that user mode is notified
> 		before drivers of impending suspends.
> 	Move the idling back into the idle loop.
> 	A couple of small tidy ups.
> 
> See header comments for attributions.
> 
> This works for me (including as a module).
> 
> Please test and let me know - it seems to lower my power requirements
> by about 10% on my Thinkpad (over stock 2.4.17).
> 

Just to confirm (albeit too late) that this one work just fine here.
Currently using Mandrake 2.4.17-10mdk kernel based on 2.4.18-pre7

Thank you

-andrej