Re: [PATCH] PCI: Add pci shutdown ability

Re: [PATCH] PCI: Add pci shutdown ability

[Adam Belay]

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

On Mon, Apr 25, 2005 at 12:06:06PM -0700, Greg KH wrote:
> Well it seems that people are starting to want to hook the reboot
> notifier, or the device shutdown facility in order to properly shutdown
> pci drivers to make kexec work nicer.
> 
> So here's a patch for the PCI core that allows pci drivers to now just
> add a "shutdown" notifier function that will be called when the system
> is being shutdown.  It happens just after the reboot notifier happens,
> and it should happen in the proper device tree order, so everyone should
> be happy.
> 
> Any objections to this patch?
> 
> thanks,
> 
> greg k-h
> ------

Hi Greg,

I think this could be important for any type of device, so the power
management subsystem and driver core should handle it.  I'm not really
sure if it's useful in pci alone, as it lacks the necessary ordering and
coordination.

I'm currently developing an interface for quieting devices without turning
them off in my Power Management model.  Pavel seems to also have plans along
those lines:

(from the current pm.h)
> * There are 4 important states driver can be in:
> * ON     -- driver is working
> * FREEZE -- stop operations and apply whatever policy is applicable to a suspended driver
> *           of that class, freeze queues for block like IDE does, drop packets for
> *           ethernet, etc... stop DMA engine too etc... so a consistent image can be
> *           saved; but do not power any hardware down.
> * SUSPEND - like FREEZE, but hardware is doing as much powersaving as possible. Roughly
> *           pci D3.

Thanks,
Adam

> 
> PCI: Add pci shutdown ability
> 
> Now pci drivers can know when the system is going down without having to
> add a reboot notifier event.
> 
> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> 
> --- gregkh-2.6.orig/include/linux/pci.h	2005-04-20 21:25:11.000000000 -0700
> +++ gregkh-2.6/include/linux/pci.h	2005-04-25 11:54:20.000000000 -0700
> @@ -671,6 +671,7 @@
>  	int  (*suspend) (struct pci_dev *dev, pm_message_t state);	/* Device suspended */
>  	int  (*resume) (struct pci_dev *dev);	                /* Device woken up */
>  	int  (*enable_wake) (struct pci_dev *dev, pci_power_t state, int enable);   /* Enable wake event */
> +	void (*shutdown) (struct pci_dev *dev);
>  
>  	struct device_driver	driver;
>  	struct pci_dynids dynids;
> Index: gregkh-2.6/drivers/pci/pci-driver.c
> ===================================================================
> --- gregkh-2.6.orig/drivers/pci/pci-driver.c	2005-04-06 11:47:47.000000000 -0700
> +++ gregkh-2.6/drivers/pci/pci-driver.c	2005-04-25 12:02:12.000000000 -0700
> @@ -318,6 +318,14 @@
>  	return 0;
>  }
>  
> +static void pci_device_shutdown(struct device *dev)
> +{
> +	struct pci_dev *pci_dev = to_pci_dev(dev);
> +	struct pci_driver *drv = pci_dev->driver;
> +
> +	if (drv && drv->shutdown)
> +		drv->shutdown(pci_dev);
> +}
>  
>  #define kobj_to_pci_driver(obj) container_of(obj, struct device_driver, kobj)
>  #define attr_to_driver_attribute(obj) container_of(obj, struct driver_attribute, attr)
> @@ -385,6 +393,7 @@
>  	drv->driver.bus = &pci_bus_type;
>  	drv->driver.probe = pci_device_probe;
>  	drv->driver.remove = pci_device_remove;
> +	drv->driver.shutdown = pci_device_shutdown,
>  	drv->driver.owner = drv->owner;
>  	drv->driver.kobj.ktype = &pci_driver_kobj_type;
>  	pci_init_dynids(&drv->dynids);
> -


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] PCI: Add pci shutdown ability

[Greg KH]

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

On Mon, Apr 25, 2005 at 04:08:25PM -0400, Adam Belay wrote:
> I think this could be important for any type of device, so the power
> management subsystem and driver core should handle it.  I'm not really
> sure if it's useful in pci alone, as it lacks the necessary ordering and
> coordination.

The driver core today _does_ handle this properly, and in the correct
order.  I'm just allowing pci drivers access to that functionality, as
today they can not take advantage of it.  That's all this patch does.

> I'm currently developing an interface for quieting devices without turning
> them off in my Power Management model.  Pavel seems to also have plans along
> those lines:

<snip>

Great, then it will tie into the current driver model code, which will
then call the proper pci driver code, and everyone will be happy :)

thanks,

greg k-h

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] PCI: Add pci shutdown ability

[Adam Belay]

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

On Mon, Apr 25, 2005 at 01:19:49PM -0700, Greg KH wrote:
> On Mon, Apr 25, 2005 at 04:08:25PM -0400, Adam Belay wrote:
> > I think this could be important for any type of device, so the power
> > management subsystem and driver core should handle it.  I'm not really
> > sure if it's useful in pci alone, as it lacks the necessary ordering and
> > coordination.
> 
> The driver core today _does_ handle this properly, and in the correct
> order.  I'm just allowing pci drivers access to that functionality, as
> today they can not take advantage of it.  That's all this patch does.

sorry, I didn't notice this after a quick glance :)

> +       drv->driver.shutdown = pci_device_shutdown,

Ok, great.  I understand.

> 
> > I'm currently developing an interface for quieting devices without turning
> > them off in my Power Management model.  Pavel seems to also have plans along
> > those lines:
> 
> <snip>

I think the intention here may have been to use PM_FREEZE via *suspend.  It
isn't currently supported though.

> 
> Great, then it will tie into the current driver model code, which will
> then call the proper pci driver code, and everyone will be happy :)
> 
> thanks,
> 
> greg k-h

Thanks,
Adam

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] PCI: Add pci shutdown ability

[Adam Belay]

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

On Mon, Apr 25, 2005 at 03:23:09PM -0400, Jeff Garzik wrote:
> Greg KH wrote:
> >Well it seems that people are starting to want to hook the reboot
> >notifier, or the device shutdown facility in order to properly shutdown
> >pci drivers to make kexec work nicer.
> >
> >So here's a patch for the PCI core that allows pci drivers to now just
> >add a "shutdown" notifier function that will be called when the system
> >is being shutdown.  It happens just after the reboot notifier happens,
> >and it should happen in the proper device tree order, so everyone should
> >be happy.
> >
> >Any objections to this patch?
> 
> Traditionally the proper place -has- been
> * the reboot notifier
> * the ->remove hook (hot unplug, and module remove)
> 
> which covers all the cases.
> 
> Add a ->shutdown hook is more of a hack.  If you want to introduce this 
> facility in a systematic way, introduce a 'kexec reboot' option which 
> walks the device tree and shuts down hardware.
> 
> ->shutdown is just a piecemeal, uncoordinated effort (uncoordinated in 
> the sense that driver shutdowns occur in an undefined order).
> 
> 	Jeff

I agree, though I think "->remove" may be more than we need.  Another
potential use of this might be to prepare devices just before removing power.

Thanks,
Adam

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] PCI: Add pci shutdown ability

[Adam Belay]

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

On Tue, Apr 26, 2005 at 02:32:29PM +1000, Benjamin Herrenschmidt wrote:

-->snip

> 
> I don't like this notion of "stop" separated from power states anyway, I
> think it just doesn't work in practice.

Yeah, after giving it some additional thought, I think there are better ways.

> 
> Ben.
> 

Ok, here's a new idea.

For many devices "->suspend" and "->resume" with pm_message_t is exactly what
we need.  However, as we support more advanced power management features, such
as runtime power management, or power containers, we need something a little
more specific.  The exact power state must be specified among other issues.

We might do something like this:

Keep "->suspend" and "->resume" around unchanged. (so the states would
probably remain as PMSG_FREEZE and PMSG_SUSPEND).  If the driver doesn't
support the more advanced PM methods just use these.  They work well enough
for system sleep states etc.

Alternatively drivers could support a more rich power management interface
via the following methods:


change_state - changes a device's power state

change_state(struct device * dev, pm_state_t state, struct system_state * sys_state, int reason);  
@dev - the device
@state - the target device-specific power state
@sys_state - a data structure containing information about the intended global system power state
@reason - why the state must be changed (ex. RUNTIME_PM, SYSTEM_SLEEP, SYSTEM_RESUME, etc.)


halt - acts somewhat like PMSG_FREEZE, stops device activity, doesn't change power state

halt(struct device * dev, struct system_state * sys_state, int reason);
@dev - the device
@sys_state - a data structure containing information about the intended global system power state
@reason - why we are halting operation (ex. RUNTIME_CHANGES (like cpufreq), SYSTEM_SLEEP, SHUTDOWN, REBOOT)  


contine - resumes from a "halt"

continue(struct device * dev, struct system_state * sys_state, int reason);
@dev - the device
@sys_state - a data structure containing information about the intended global system power state
@reason - why we are resuming operation (ex. RUNTIME_CHANGES (like cpufreq), SYSTEM_RESUME)  


When changing system state, we call "change_state" for every device with power
resources.  Devices that do not directly consume power or have power states
will not implement "change_state" so we will call "halt" and "continue"
instead.

When shutting down the system, halt has the option of turning off the device,
as it will see the SHUTDOWN reason.  So it's a driver-knows-best approach
instead of assuming everything must be turned off, or everything must just be
stopped.

So in theory, with cpufreq, we could stop userspace, ->halt every device
(drivers won't do anything if they know it's not necessary), change the
frequency, and then resume operation.

We may want to create structures like pm_message_t for "change_state", "halt",
and "continue".  Pavel, do you have any thoughts on this?

This is just a rough idea... I look forward to any comments or suggestions.

Thanks,
Adam

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] PCI: Add pci shutdown ability

[Pavel Machek]

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

Hi!

> > I don't like this notion of "stop" separated from power states anyway, I
> > think it just doesn't work in practice.
> 
> Yeah, after giving it some additional thought, I think there are better ways.
> 
> > 
> > Ben.
> > 
> 
> Ok, here's a new idea.
> 
> For many devices "->suspend" and "->resume" with pm_message_t is exactly what
> we need.  However, as we support more advanced power management features, such
> as runtime power management, or power containers, we need something a little
> more specific.  The exact power state must be specified among other
> issues.

Okay, maybe. But not by adding 3 new callbacks that mirror existing
functionality.

> We might do something like this:
> 
> Keep "->suspend" and "->resume" around unchanged. (so the states would
> probably remain as PMSG_FREEZE and PMSG_SUSPEND).  If the driver doesn't
> support the more advanced PM methods just use these.  They work well enough
> for system sleep states etc.
> 
> Alternatively drivers could support a more rich power management interface
> via the following methods:
> 
> 
> change_state - changes a device's power state
> 
> change_state(struct device * dev, pm_state_t state, struct system_state * sys_state, int reason);  
> @dev - the device
> @state - the target device-specific power state
> @sys_state - a data structure containing information about the intended global system power state
> @reason - why the state must be changed (ex. RUNTIME_PM,
> SYSTEM_SLEEP, SYSTEM_RESUME, etc.)

If drivers really need to know system state and reason, just put it
into pm_message_t. I wanted to add "flags" there from the begining,
serving similar purpose as your "reason".

> halt - acts somewhat like PMSG_FREEZE, stops device activity, doesn't change power state
> 
> halt(struct device * dev, struct system_state * sys_state, int reason);
> @dev - the device
> @sys_state - a data structure containing information about the intended global system power state
> @reason - why we are halting operation (ex. RUNTIME_CHANGES (like cpufreq), SYSTEM_SLEEP, SHUTDOWN, REBOOT)  

If it is similar to PMSG_FREEZE, just pass PMSG_FREEZE and put
* sys_state  and reason into pm_message_t.

> contine - resumes from a "halt"
> 
> continue(struct device * dev, struct system_state * sys_state, int reason);
> @dev - the device
> @sys_state - a data structure containing information about the intended global system power state
> @reason - why we are resuming operation (ex. RUNTIME_CHANGES (like cpufreq), SYSTEM_RESUME)  

Now, here you have a point. resume() should get pm_message_t,
too. This should be rather easy to change (simple matter of coding),
and we have agreed before that it is good idea. Patches welcome.

								Pavel
-- 
Boycott Kodak -- for their patent abuse against Java.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]