Re: [PATCH] fix tulip suspend/resume

Pavel Machek
2005-06-08T12:24:08Z

Hi!
> > > pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
> > > {
> > > if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> > > return PCI_D0;
> > >
> > > switch (state) {
> > > case 0: return PCI_D0;
> > > case 3: return PCI_D3hot;
> > > default:
> > > printk("They asked me for state %d\n", state);
> > > BUG();
> > > }
> > > return PCI_D0;
> > > }
> >
> > Gack ! I need to remember to fix that one before I change PMSG_FREEZE
> > definition to be different than PMSG_SUSPEND upstream.
> >
> > Pavel, do you know that there are other ways to deal with errors than
> > just BUG()'ing all over the place ? :)
> >
> > Ben.
>
> I think we should also use the pm_message_t defines. We will need to
> add PMSG_FREEZE eventually. I decided to default to the current state
> rather than panic. Does this patch look ok?

No.

> --- a/drivers/pci/pci.c 2005-05-27 22:06:02.000000000 -0400
> +++ b/drivers/pci/pci.c 2005-06-07 22:10:02.066151280 -0400
> @@ -320,13 +320,15 @@
> return PCI_D0;
>
> switch (state) {
> - case 0: return PCI_D0;
> - case 3: return PCI_D3hot;
> + case PMSG_ON:
> + return PCI_D0;
> + case PMSG_SUSPEND:
> + return PCI_D3hot;

Please don't do this; it will not compile when I turn on type checking
on pm_message_t. I have this:

/**
* pci_choose_state - Choose the power state of a PCI device
* @dev: PCI device to be suspended
* @state: target sleep state for the whole system. This is the value
* that is passed to suspend() function.
*
* Returns PCI power state suitable for given device and given system
* message.
*/

pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
{
switch (state.event) {
case PM_EVENT_ON:
return PCI_D0;
case PM_EVENT_FREEZE:
case PM_EVENT_SUSPEND:
return PCI_D3hot;
default:
printk("They asked me for state %d\n", state.event);
BUG();
}
return PCI_D0;
}

> default:
> - printk("They asked me for state %d\n", state);
> - BUG();
> + printk(KERN_ERR "PCI: invalid PM message state - %d\n", state);
> }
> - return PCI_D0;
> +
> + return dev->current_state;
> }

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.

Pavel