Re: [PATCH] fix tulip suspend/resume

[Pavel Machek <pavel@suse.cz>]

Hi!

> > > >         case PM_EVENT_ON:
> > > >                 return PCI_D0;
> > > >         case PM_EVENT_FREEZE:
> > > >         case PM_EVENT_SUSPEND:
> > > >                 return PCI_D3hot;
> > > 
> > > What are these new PM_EVENT_* things ? I though we defined PMSG_* ?
> > 
> > PMSG_* are for struct; you can't case on struct.
> > 
> > > > You passed invalid argument; I see no reason why you should paper over
> > > > it and risk continuing. This happens during system suspend; it is
> > > > quite possible that user will not see your printk when machine powers
> > > > off just after that; and remember that it will not be in syslog after
> > > > resume.
> > > 
> > > Crap. I don't think a BUG() makes any useful help neither in this place,
> > > and when I locally turn PMSG_FREEZE to something sane I suddenly blow up
> > > in there (and I wonder in how many other places).
> > 
> > At least you can see & report that error... That would not be a case
> > for simple printk.
> 
> Well not exactly.  The video device may have already been disabled, so
> the user wouldn't see it anyway.  The reason I don't like BUG() here is
> that it's very unlikely passing an invalid PMSG will have serious
> consequences.  The problems are likely less significant than those
> caused by calling BUG().  Many suspend routines don't set power at all
> yet (e.g. cardbus) and it still works out fine.  Defaulting to the
> current state seems like a very safe behavior.  Then, after resume, we
> can see the messages.  I'd like to see PM just work, and we have to be
> careful during an API change.

Ok, refaulting to current state seems pretty good. But you are not
going to see the messages after the resume; not in swsusp if they
happened after swsusp atomic snapshot. In recent swsusp, I'm carefull
not to blank consoles when I can avoid it, exactly so messages like
this can be seen.

> > Turn pm_message_t into struct, so that it is typechecked properly (and
> > so that we can add flags field in future). This should not go in
> > before 2.6.12.
> 
> What do you have in mind for adding to the structure in the future?

*Maybe* flags telling drivers details of transition will be
needed. And I thought "partial suspend" people will want stuff like
char * "enter state with disk spinning at at most 300 rpm" to be added
there, too.

Now its mostly for typechecking.
								Pavel