Re: [patch] pm: fix runtime powermanagement's /sys interface

Pavel Machek
2006-01-06T08:59:21Z

[-- Attachment #1: Type: text/plain, Size: 2388 bytes --]

On Čt 05-01-06 17:37:30, Patrick Mochel wrote:
>
>
> On Fri, 6 Jan 2006, Pavel Machek wrote:
>
> > On ÄŒt 05-01-06 16:04:07, Patrick Mochel wrote:
>
> > > A better point, and one that would actually be useful, would be to remove
> > > the file altogether. Let Dominik export a power file, with complete
> > > control over the values, for each pcmcia device. Then you never have to
> > > worry about breaking PCMCIA again.
> >
> > Fine with me.
>
> ACK, you beat me to it.
>
> And, appended is a patch to export PM controls for PCI devices. The file
> "pm_possible_states" exports the states a device supports, and "pm_state"
> exports the current state (and provides the interface for entering a
> state).
>
> Eventually, some drivers will want to fix up those values so that it can
> mask of states that it doesn't support, as well as offer possible device-
> specific states.
>
> What's interesting is that with this patch, I can see that two more
> devices on my system support D1 and D2 -- the cardbus controllers, which
> are actually bridges whose PM capabilities aren't exported via
lspci.

> +static ssize_t pm_possible_states_show(struct device * d,
> + struct device_attribute * a,
> + char * buffer)
> +{
> + struct pci_dev * dev = to_pci_dev(d);
> + char * s = buffer;
> +
> + s += sprintf(s, "d0");
> + if (dev->poss_states[PCI_D1])
> + s += sprintf(s, " d1");
> + if (dev->poss_states[PCI_D2])
> + s += sprintf(s, " d2");
> + if (dev->poss_states[PCI_D3hot])
> + s += sprintf(s, " d3");

Could we use same states as rest of code, i.e. "D2" instead of "d2"
and "D3hot" instead of "d3"?

> +static ssize_t pm_state_show(struct device * d, struct device_attribute * a,
> + char * buffer)

Please no space between "struct foo *" and "d".

> + if (state == dev->current_state)
> + return 0;
> +
> + if (dev->poss_states[state])
> + ret = pci_set_power_state(dev, state);

So you just set the PCI power, without any coordination with driver?
How can this work?

> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -106,6 +106,7 @@ struct pci_dev {
> this if your device has broken DMA
> or supports 64-bit transfers. */
>
> + u32 poss_states[4];

It is boolean... Can we have int instead of u32?

Pavel

--
Thanks, Sharp!