Re: [PATCH] swsusp: misc cleanups [4/4]

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] swsusp: misc cleanups [4/4]

Pavel Machek
Hi!

> > > The following patch makes some comments and printk()s in swsusp.c up to date.
> > > In particular it adds the string "swsusp" before every message that is printed by
> > > the code in swsusp.c.
> >
> > I don't like this one. Adding swsusp everywhere just clutters the
> > screen, most of it should be clear from context.
>
> Right.  Still, I'd like to drop function names from debug messages
> and replace "suspend" with "swsusp" in some messages.  I'll send another
> patch for it after 2.6.12 (please let me know if you don't think it's a good
> idea ;-)).

Well, I'll not say "no" just now ;-). I thought 'suspend' is nicer for
non-technical users, not being acronym.

> For now, please drop the patch altogether.

Done.

> > > @@ -1226,9 +1222,6 @@ static int check_sig(void)
> > >  
> > >  /**
> > >   * data_read - Read image pages from swap.
> > > - *
> > > - * You do not need to check for overlaps, check_pagedir()
> > > - * already did that.
> > >   */
> > >  
> > >  static int data_read(struct pbe *pblist)
> >
> > Why is this comment no longer valid?
>
> It's just confusing.  Initially, I didn't intend to change it, but then I read it
> and thought "What overlaps?".  In data_read() there's nothing that could
> overlap with anything else ...

Well, if there were no checking in check_pagedir, we could end up in
situation where some page's address == some other page's original
address. That would be really bad. This comment says it can not happen
because care was taken in check_pagedir(). Perhaps it needs to be
explained better?

                                                                Pavel
--
Boycott Kodak -- for their patent abuse against Java.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] swsusp: misc cleanups [4/4]

Pavel Machek-2
Hi!

> >> The following patch makes some comments and printk()s in swsusp.c up to date.
> >> In particular it adds the string "swsusp" before every message that is printed by
> >> the code in swsusp.c.
> >
> > I don't like this one. Adding swsusp everywhere just clutters the
> > screen, most of it should be clear from context.
>
> I like it. The messages are short enough so we should not get line wraps
> and it makes stuff more clear. You know, the context is not familiar to
> everyone, just imagine the "why do we {suspend,resume} devices during
> {resume,suspend} questions.
>
> Also, i can ask for "send me output of dmesg|grep ^swsusp" to avoid
> newbies flooding me with totally irrelevant info ;-)

That would not work, anyway. You want the messages from drivers,
too... and drivers are not going to prepend "swsusp".

Ultimately, cleaning up "suspend screen" so that it is not too scary
for non-technical users might be nice... It means killing some debug
messages from drivers, too.

                                          Pavel
--
Boycott Kodak -- for their patent abuse against Java.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] swsusp: misc cleanups [4/4]

seife (Bugzilla)-2
Pavel Machek wrote:

> That would not work, anyway. You want the messages from drivers,
> too... and drivers are not going to prepend "swsusp".

Yes it would. I do not need driver messages if the reason is "no swap
partition" or something like that ;-))

> Ultimately, cleaning up "suspend screen" so that it is not too scary
> for non-technical users might be nice... It means killing some debug
> messages from drivers, too.

I'd just sweep it under the bootsplash carpet. Then we have both:
possibility to debug and nice progressbar as long as everything works
fine :-)
--
Stefan Seyfried, QA / R&D Team Mobile Devices, SUSE LINUX N├╝rnberg.

"Any ideas, John?"
"Well, surrounding them's out."
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] swsusp: misc cleanups [4/4]

Pavel Machek-2
Hi!

> > That would not work, anyway. You want the messages from drivers,
> > too... and drivers are not going to prepend "swsusp".
>
> Yes it would. I do not need driver messages if the reason is "no swap
> partition" or something like that ;-))

Umm, okay. grep -i5 swsusp ... should get most driver messages, too...

> > Ultimately, cleaning up "suspend screen" so that it is not too scary
> > for non-technical users might be nice... It means killing some debug
> > messages from drivers, too.
>
> I'd just sweep it under the bootsplash carpet. Then we have both:
> possibility to debug and nice progressbar as long as everything works
> fine :-)

I don't like bootsplash... plus I'd like to clean it properly. It
prints too much junk even for me...
                                                                Pavel

--
Boycott Kodak -- for their patent abuse against Java.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/