Re: [PATCH 8/8] i8042: support device async suspend & shutdown

Re: [PATCH 8/8] i8042: support device async suspend & shutdown

[Pavel Machek]

On Wed 2009-07-15 15:38:42, Zhang Rui wrote:
> 
> i8042 controller support device async actions.
> 
> If boot option "dev_async_action" is added,
> i8042 controller and its child devices can be
> suspended/resumed/shutdown asynchronously.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/input/serio/i8042.c |   10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/drivers/input/serio/i8042.c
> ===================================================================
> --- linux-2.6.orig/drivers/input/serio/i8042.c
> +++ linux-2.6/drivers/input/serio/i8042.c
> @@ -1284,14 +1284,21 @@ static int __init i8042_init(void)
>  		goto err_unregister_driver;
>  	}
>  
> -	err = platform_device_add(i8042_platform_device);
> +	err = dev_async_register(&i8042_platform_device->dev,
> +			DEV_ASYNC_SUSPEND | DEV_ASYNC_SHUTDOWN);
>  	if (err)
>  		goto err_free_device;
>  
> +	err = platform_device_add(i8042_platform_device);
> +	if (err)
> +		goto err_dev_async_unregister;
> +
>  	panic_blink = i8042_panic_blink;
>  
>  	return 0;
>  
> + err_dev_async_unregister:
> +	dev_async_unregister(&i8042_platform_device->dev);


>   err_free_device:
>  	platform_device_put(i8042_platform_device);
>   err_unregister_driver:
> @@ -1304,6 +1311,7 @@ static int __init i8042_init(void)
>  
>  static void __exit i8042_exit(void)
>  {
> +	dev_async_unregister(&i8042_platform_device->dev);
>  	platform_device_unregister(i8042_platform_device);
>  	platform_driver_unregister(&i8042_driver);
>  	i8042_platform_exit();

Could we get the core to do async_unregister during device_unregister?

Or maybe better, just add flags during device_add?

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH 8/8] i8042: support device async suspend & shutdown

[Zhang Rui]

i8042 controller support device async actions.

If boot option "dev_async_action" is added,
i8042 controller and its child devices can be
suspended/resumed/shutdown asynchronously.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/input/serio/i8042.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/input/serio/i8042.c
===================================================================
--- linux-2.6.orig/drivers/input/serio/i8042.c
+++ linux-2.6/drivers/input/serio/i8042.c
@@ -1284,14 +1284,21 @@ static int __init i8042_init(void)
 		goto err_unregister_driver;
 	}
 
-	err = platform_device_add(i8042_platform_device);
+	err = dev_async_register(&i8042_platform_device->dev,
+			DEV_ASYNC_SUSPEND | DEV_ASYNC_SHUTDOWN);
 	if (err)
 		goto err_free_device;
 
+	err = platform_device_add(i8042_platform_device);
+	if (err)
+		goto err_dev_async_unregister;
+
 	panic_blink = i8042_panic_blink;
 
 	return 0;
 
+ err_dev_async_unregister:
+	dev_async_unregister(&i8042_platform_device->dev);
  err_free_device:
 	platform_device_put(i8042_platform_device);
  err_unregister_driver:
@@ -1304,6 +1311,7 @@ static int __init i8042_init(void)
 
 static void __exit i8042_exit(void)
 {
+	dev_async_unregister(&i8042_platform_device->dev);
 	platform_device_unregister(i8042_platform_device);
 	platform_driver_unregister(&i8042_driver);
 	i8042_platform_exit();

Re: [PATCH 8/8] i8042: support device async suspend & shutdown

[Andi Kleen]

Zhang Rui <rui.zhang@intel.com> writes:

> i8042 controller support device async actions.
>
> If boot option "dev_async_action" is added,
> i8042 controller and its child devices can be
> suspended/resumed/shutdown asynchronously.
>

>From a quick look at the i8042 driver it still 
seems to do a lot of slow actions without actually sleeping
or worse holding locks. e.g. the delay loop in i8042_flush()

Did you measure how long that one takes?

Due to the locks even preempt kernels couldn't do
something during that time.

The spinlocks are probably needed when the code is executed
during interrupts, but perhaps the suspend variant
could use a different code path.

Perhaps it only makes sense to do this when this 
code is converted to sleep during delays too?

-Andi


-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 8/8] i8042: support device async suspend & shutdown

[Zhang Rui]

On Wed, 2009-07-15 at 18:46 +0800, Andi Kleen wrote:
> Zhang Rui <rui.zhang@intel.com> writes:
> 
> > i8042 controller support device async actions.
> >
> > If boot option "dev_async_action" is added,
> > i8042 controller and its child devices can be
> > suspended/resumed/shutdown asynchronously.
> >
> 
> From a quick look at the i8042 driver it still 
> seems to do a lot of slow actions without actually sleeping
> or worse holding locks. e.g. the delay loop in i8042_flush()
> 
> Did you measure how long that one takes?
> 
> Due to the locks even preempt kernels couldn't do
> something during that time.
> 
> The spinlocks are probably needed when the code is executed
> during interrupts, but perhaps the suspend variant
> could use a different code path.
> 
> Perhaps it only makes sense to do this when this 
> code is converted to sleep during delays too?
> 
most of the psmouse suspend/shutdown time was cost by a ps2 reset
command.
In my test, the serio2 (psmouse) total suspend time is 0.42s, while
the psmouse_reset takes 0.4s.

i.e.
drivers/input/serio/libps2.c

int ps2_command(struct ps2dev *ps2dev, unsigned char *param, int
command)
{
...
/*
 * The reset command takes a long time to execute.
 */
timeout = msecs_to_jiffies(command == PS2_CMD_RESET_BAT ? 4000 : 500);

timeout = wait_event_timeout(ps2dev->wait,
			!(ps2dev->flags & PS2_FLAG_CMD1), timeout);
...
}

so the patch could save suepend/shutdown time a lot, without any other
changes in psmouse driver.

thanks,
rui

Re: [PATCH 8/8] i8042: support device async suspend & shutdown

[Zhang Rui]

On Wed, 2009-07-15 at 08:37 +0800, Pavel Machek wrote:
> On Wed 2009-07-15 15:38:42, Zhang Rui wrote:
> > 
> > i8042 controller support device async actions.
> > 
> > If boot option "dev_async_action" is added,
> > i8042 controller and its child devices can be
> > suspended/resumed/shutdown asynchronously.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/input/serio/i8042.c |   10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > Index: linux-2.6/drivers/input/serio/i8042.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/input/serio/i8042.c
> > +++ linux-2.6/drivers/input/serio/i8042.c
> > @@ -1284,14 +1284,21 @@ static int __init i8042_init(void)
> >  		goto err_unregister_driver;
> >  	}
> >  
> > -	err = platform_device_add(i8042_platform_device);
> > +	err = dev_async_register(&i8042_platform_device->dev,
> > +			DEV_ASYNC_SUSPEND | DEV_ASYNC_SHUTDOWN);
> >  	if (err)
> >  		goto err_free_device;
> >  
> > +	err = platform_device_add(i8042_platform_device);
> > +	if (err)
> > +		goto err_dev_async_unregister;
> > +
> >  	panic_blink = i8042_panic_blink;
> >  
> >  	return 0;
> >  
> > + err_dev_async_unregister:
> > +	dev_async_unregister(&i8042_platform_device->dev);
> 
> 
> >   err_free_device:
> >  	platform_device_put(i8042_platform_device);
> >   err_unregister_driver:
> > @@ -1304,6 +1311,7 @@ static int __init i8042_init(void)
> >  
> >  static void __exit i8042_exit(void)
> >  {
> > +	dev_async_unregister(&i8042_platform_device->dev);
> >  	platform_device_unregister(i8042_platform_device);
> >  	platform_driver_unregister(&i8042_driver);
> >  	i8042_platform_exit();
> 
> Could we get the core to do async_unregister during device_unregister?
> 
yes, we can.
I think it's a little strange that drivers need to invoke
dev_async_register to register an async device group, but don't need to
call dev_async_unregister to unregister it.

> Or maybe better, just add flags during device_add?
> 
Many devices don't know if they support async actions or not during
device initialization time, like PCI and ACPI devices.
If we want to create an async group for a PCI device, we have to call
dev_async_register in the specified PCI device driver, rather than in
the PCI bus scan stage.

thanks,
rui

Re: [PATCH 8/8] i8042: support device async suspend & shutdown

[Rafael J. Wysocki]

On Friday 17 July 2009, Zhang Rui wrote:
> On Wed, 2009-07-15 at 08:37 +0800, Pavel Machek wrote:
> > On Wed 2009-07-15 15:38:42, Zhang Rui wrote:
> > > 
> > > i8042 controller support device async actions.
> > > 
> > > If boot option "dev_async_action" is added,
> > > i8042 controller and its child devices can be
> > > suspended/resumed/shutdown asynchronously.
> > > 
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > ---
> > >  drivers/input/serio/i8042.c |   10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > 
> > > Index: linux-2.6/drivers/input/serio/i8042.c
> > > ===================================================================
> > > --- linux-2.6.orig/drivers/input/serio/i8042.c
> > > +++ linux-2.6/drivers/input/serio/i8042.c
> > > @@ -1284,14 +1284,21 @@ static int __init i8042_init(void)
> > >  		goto err_unregister_driver;
> > >  	}
> > >  
> > > -	err = platform_device_add(i8042_platform_device);
> > > +	err = dev_async_register(&i8042_platform_device->dev,
> > > +			DEV_ASYNC_SUSPEND | DEV_ASYNC_SHUTDOWN);
> > >  	if (err)
> > >  		goto err_free_device;
> > >  
> > > +	err = platform_device_add(i8042_platform_device);
> > > +	if (err)
> > > +		goto err_dev_async_unregister;
> > > +
> > >  	panic_blink = i8042_panic_blink;
> > >  
> > >  	return 0;
> > >  
> > > + err_dev_async_unregister:
> > > +	dev_async_unregister(&i8042_platform_device->dev);
> > 
> > 
> > >   err_free_device:
> > >  	platform_device_put(i8042_platform_device);
> > >   err_unregister_driver:
> > > @@ -1304,6 +1311,7 @@ static int __init i8042_init(void)
> > >  
> > >  static void __exit i8042_exit(void)
> > >  {
> > > +	dev_async_unregister(&i8042_platform_device->dev);
> > >  	platform_device_unregister(i8042_platform_device);
> > >  	platform_driver_unregister(&i8042_driver);
> > >  	i8042_platform_exit();
> > 
> > Could we get the core to do async_unregister during device_unregister?
> > 
> yes, we can.
> I think it's a little strange that drivers need to invoke
> dev_async_register to register an async device group, but don't need to
> call dev_async_unregister to unregister it.
> 
> > Or maybe better, just add flags during device_add?
> > 
> Many devices don't know if they support async actions or not during
> device initialization time, like PCI and ACPI devices.
> If we want to create an async group for a PCI device, we have to call
> dev_async_register in the specified PCI device driver, rather than in
> the PCI bus scan stage.

That is quite a big design issue, IMO.

Did you consider any other approach to this?

Best,
Rafael

Re: [PATCH 8/8] i8042: support device async suspend & shutdown

[Zhang Rui]

On Fri, 2009-07-17 at 10:40 +0800, Rafael J. Wysocki wrote:
> On Friday 17 July 2009, Zhang Rui wrote:
> > On Wed, 2009-07-15 at 08:37 +0800, Pavel Machek wrote:
> > > On Wed 2009-07-15 15:38:42, Zhang Rui wrote:
> > > > 
> > > > i8042 controller support device async actions.
> > > > 
> > > > If boot option "dev_async_action" is added,
> > > > i8042 controller and its child devices can be
> > > > suspended/resumed/shutdown asynchronously.
> > > > 
> > > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > > ---
> > > >  drivers/input/serio/i8042.c |   10 +++++++++-
> > > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > > 
> > > > Index: linux-2.6/drivers/input/serio/i8042.c
> > > > ===================================================================
> > > > --- linux-2.6.orig/drivers/input/serio/i8042.c
> > > > +++ linux-2.6/drivers/input/serio/i8042.c
> > > > @@ -1284,14 +1284,21 @@ static int __init i8042_init(void)
> > > >  		goto err_unregister_driver;
> > > >  	}
> > > >  
> > > > -	err = platform_device_add(i8042_platform_device);
> > > > +	err = dev_async_register(&i8042_platform_device->dev,
> > > > +			DEV_ASYNC_SUSPEND | DEV_ASYNC_SHUTDOWN);
> > > >  	if (err)
> > > >  		goto err_free_device;
> > > >  
> > > > +	err = platform_device_add(i8042_platform_device);
> > > > +	if (err)
> > > > +		goto err_dev_async_unregister;
> > > > +
> > > >  	panic_blink = i8042_panic_blink;
> > > >  
> > > >  	return 0;
> > > >  
> > > > + err_dev_async_unregister:
> > > > +	dev_async_unregister(&i8042_platform_device->dev);
> > > 
> > > 
> > > >   err_free_device:
> > > >  	platform_device_put(i8042_platform_device);
> > > >   err_unregister_driver:
> > > > @@ -1304,6 +1311,7 @@ static int __init i8042_init(void)
> > > >  
> > > >  static void __exit i8042_exit(void)
> > > >  {
> > > > +	dev_async_unregister(&i8042_platform_device->dev);
> > > >  	platform_device_unregister(i8042_platform_device);
> > > >  	platform_driver_unregister(&i8042_driver);
> > > >  	i8042_platform_exit();
> > > 
> > > Could we get the core to do async_unregister during device_unregister?
> > > 
> > yes, we can.
> > I think it's a little strange that drivers need to invoke
> > dev_async_register to register an async device group, but don't need to
> > call dev_async_unregister to unregister it.
> > 
> > > Or maybe better, just add flags during device_add?
> > > 
> > Many devices don't know if they support async actions or not during
> > device initialization time, like PCI and ACPI devices.
> > If we want to create an async group for a PCI device, we have to call
> > dev_async_register in the specified PCI device driver, rather than in
> > the PCI bus scan stage.
> 
> That is quite a big design issue, IMO.
> 
> Did you consider any other approach to this?
> 
not yet.

Just like the async boot approach, this async action mechanism is not
designed for all the devices.

Plus different device actions should be handled differently.

E.g device suspend and shutdown are more complicate than resume case.
Because in resume case, child devices are ALWAYS resumed after its
parent device, so it's easy to register an DEV_ASYNC_RESUME group.
But if we want to register an async device group that supports
DEV_ASYNC_SUSPEND/DEV_ASYNC_SHUTDOWN, we must make sure that
the device groups is something like a Closure, i.e. they don't depend on
ANY OTHER devices.

Anyway, IMO, if we want to register a new async device group, we should
do it case by case.

thanks,
rui