Re: [patch] enums to clear suspend-state confusion

Pavel Machek
2004-08-17T22:37:27Z

Hi!

> > I'd like this to be applied, so I can start fixing the drivers...
>
> Sure, let's try to get this done.
>
> > +static inline enum pci_state to_pci_state(suspend_state_t state)
> > +{
> > + if (SUSPEND_EQ(state, PM_SUSPEND_ON))
> > + return PCI_D0;
> > + if (SUSPEND_EQ(state, PM_SUSPEND_STANDBY))
> > + return PCI_D1;
> > + if (SUSPEND_EQ(state, PM_SUSPEND_MEM))
> > + return PCI_D3hot;
> > + if (SUSPEND_EQ(state, PM_SUSPEND_DISK))
> > + return PCI_D3cold;
> > + BUG();
> > + return PCI_D0; /* akpm complained about warnings? */
> > +}
> > +
> > ...
> > +/*
> > + * For now, drivers only get system state. Later, this is going to become
> > + * structure or something to enable runtime power managment.
> > + */
> > +typedef enum system_state suspend_state_t;
> > +
> > +#define SUSPEND_EQ(a, b) (a == b)
> > +
> > enum {
> > PM_DISK_FIRMWARE = 1,
> > PM_DISK_PLATFORM,
>
> This is a bit ugly, and I don't think it actually works.

I agree about the ugly bit :-(.

> If, at some time in the future you change the suspend state to a struct
> then you will want to pass that thing around by reference, not by
> value.

Actually I expect it to become struct of two members, system-state and
bus-specific state. That seems small enough to pass by value.

> Hence your new suspend_state_t will need to be typecast to a pointer to
> struct, and not a struct. And that's not a thing which we do in-kernel
> much at all. (There's nothing wrong with the practice per-se, but in the
> kernel it does violate the principle of least surprise).
>
> So if you really do intend to add more things to the suspend state I'd
> suggest that you set the final framework in place immediately. Do:
>
> struct suspend_state {
> enum system_state state;
> }

I can do that... but it will break compilation of every driver in the
tree. I can fix drivers I use and try to fix some more will sed, but
it will be painfull (and pretty big diff, and I'll probably miss some).

Should I do that?
Pavel
--
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!