Re: [PATCH] fix tulip suspend/resume

[Pavel Machek]

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/