[lm-sensors] PATCH: hwmon-abituguru-suspend-resume.patch

[lm-sensors] PATCH: hwmon-abituguru-suspend-resume.patch

[Hans de Goede]

Hi All,

This patch contains rudimentary suspend / resume support for the uguru,
this protects the uguru and the driver against suspend / resume cycles,
so there is no reason to unload the driver in your suspend / resume scripts.

I'm not sure if this is needed, does the sysfs code itself guarantee
that no sysfs attr read/write calls are in progress before starting a
resume, if it does then this patch most likely isn't needed. Except for
protecting against this, it also checks the uguru is still in ready
status after a resume, but that seems to be unnescesarry (as it seems
that the uguru always is still ready after the resume).

Signed-off-by: Hans de Goede <j.w.r.degoede at hhs.nl>

Regards,

Hans
-------------- next part --------------
A non-text attachment was scrubbed...
Name: hwmon-abituguru-suspend-resume.patch
Type: text/x-patch
Size: 1394 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060810/24ab740f/attachment.bin 

[lm-sensors] PATCH: hwmon-abituguru-suspend-resume.patch

[Jean Delvare]

Hi Hans,

> This patch contains rudimentary suspend / resume support for the uguru,
> this protects the uguru and the driver against suspend / resume cycles,
> so there is no reason to unload the driver in your suspend / resume scripts.
> 
> I'm not sure if this is needed, does the sysfs code itself guarantee
> that no sysfs attr read/write calls are in progress before starting a
> resume, if it does then this patch most likely isn't needed.

Good question, I really don't know. If sysfs doesn't guarantee it, then
we should do something similar for all hardware monitoring drivers,
it's simple enough.

Maybe you can raise the question on LKML, or maybe linux-pm-devel
(doesn't seem too active though) or otherwise ask anyone with more
power management knowledge than I do?

> Except for
> protecting against this, it also checks the uguru is still in ready
> status after a resume, but that seems to be unnescesarry (as it seems
> that the uguru always is still ready after the resume).

Well this patch shouldn't harm, even if it doesn't do anything
useful ;) so I guess I can take it.

> @@ -1336,6 +1380,8 @@
>  	},
>  	.probe	= abituguru_probe,
>  	.remove	= __devexit_p(abituguru_remove),
> +	.suspend = abituguru_suspend,
> +	.resume = abituguru_resume
>  };

This makes the alignment a bit shaky, tabs and spaces mixed... I'll fix
it.

Thanks,
-- 
Jean Delvare

[lm-sensors] PATCH: hwmon-abituguru-suspend-resume.patch

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans,
> 
>> This patch contains rudimentary suspend / resume support for the uguru,
>> this protects the uguru and the driver against suspend / resume cycles,
>> so there is no reason to unload the driver in your suspend / resume scripts.
>>
>> I'm not sure if this is needed, does the sysfs code itself guarantee
>> that no sysfs attr read/write calls are in progress before starting a
>> resume, if it does then this patch most likely isn't needed.
> 
> Good question, I really don't know. If sysfs doesn't guarantee it, then
> we should do something similar for all hardware monitoring drivers,
> it's simple enough.
> 
> Maybe you can raise the question on LKML, or maybe linux-pm-devel
> (doesn't seem too active though) or otherwise ask anyone with more
> power management knowledge than I do?
> 

I'm not subscribed to LKML (too much mail from other lists already), 
maybe someone who is can forward this for me?

My guess is that this protection is actually needed as in the end the 
sysfs reads / writes are just ordinary reads / writes and I don't think 
that the kernel guarantees all reads/writes to have completed when 
suspending, because that would be a real pain to guarantee for things 
like NFS, disks, etc.

>> Except for
>> protecting against this, it also checks the uguru is still in ready
>> status after a resume, but that seems to be unnescesarry (as it seems
>> that the uguru always is still ready after the resume).
> 
> Well this patch shouldn't harm, even if it doesn't do anything
> useful ;) so I guess I can take it.
> 

I don't mind it getting applied either, that would keep upstream and my 
private copy nicely in sync.

Regards,

Hans

[lm-sensors] PATCH: hwmon-abituguru-suspend-resume.patch

[Jean Delvare]

Hans,

> >> This patch contains rudimentary suspend / resume support for the uguru,
> >> this protects the uguru and the driver against suspend / resume cycles,
> >> so there is no reason to unload the driver in your suspend / resume scripts.
> >>
> >> I'm not sure if this is needed, does the sysfs code itself guarantee
> >> that no sysfs attr read/write calls are in progress before starting a
> >> resume, if it does then this patch most likely isn't needed.
> > 
> > Good question, I really don't know. If sysfs doesn't guarantee it, then
> > we should do something similar for all hardware monitoring drivers,
> > it's simple enough.
> > 
> > Maybe you can raise the question on LKML, or maybe linux-pm-devel
> > (doesn't seem too active though) or otherwise ask anyone with more
> > power management knowledge than I do?
> > 
> 
> I'm not subscribed to LKML (too much mail from other lists already), 
> maybe someone who is can forward this for me?

I'm not subscribed either, for the same reason. But you don't need to
be subscribed to post.

> My guess is that this protection is actually needed as in the end the 
> sysfs reads / writes are just ordinary reads / writes and I don't think 
> that the kernel guarantees all reads/writes to have completed when 
> suspending, because that would be a real pain to guarantee for things 
> like NFS, disks, etc.

Well, something has to take care of it anyway. If not sysfs, each
individual driver will have to do it, so it would make as much sense if
sysfs actually did it.

Anyway, I'll apply your patch, we'll see if someone complains ;)

-- 
Jean Delvare

[lm-sensors] PATCH: hwmon-abituguru-suspend-resume.patch

[Jean Delvare]

Hi Hans,

> This patch contains rudimentary suspend / resume support for the uguru,
> this protects the uguru and the driver against suspend / resume cycles,
> so there is no reason to unload the driver in your suspend / resume scripts.
> 
> I'm not sure if this is needed, does the sysfs code itself guarantee
> that no sysfs attr read/write calls are in progress before starting a
> resume, if it does then this patch most likely isn't needed. Except for
> protecting against this, it also checks the uguru is still in ready
> status after a resume, but that seems to be unnescesarry (as it seems
> that the uguru always is still ready after the resume).

I've noticed that most drivers take care to exclude the suspend/resume
functions when CONFIG_PM is disabled. I agree it shouldn't matter much
for the abituguru driver, as ACPI implies CONFIG_PM and I don't think
anyone would run these boards with ACPI support. But in an attempt to
make things as good as possible (in case others copy that code for
other hwmon driver), what about stacking the following patch on top of
yours?


Only include suspend/resume functions when CONFIG_PM is set.

Signed-off-by: Jean Delvare <khali at linux-fr.org>
---
 drivers/hwmon/abituguru.c |    5 +++++
 1 file changed, 5 insertions(+)

--- linux-2.6.18-rc5.orig/drivers/hwmon/abituguru.c	2006-08-28 11:09:39.000000000 +0200
+++ linux-2.6.18-rc5/drivers/hwmon/abituguru.c	2006-08-28 11:31:26.000000000 +0200
@@ -1354,6 +1354,7 @@
 		return NULL;
 }
 
+#ifdef CONFIG_PM
 static int abituguru_suspend(struct platform_device *pdev, pm_message_t state)
 {
 	struct abituguru_data *data = platform_get_drvdata(pdev);
@@ -1372,6 +1373,10 @@
 	mutex_unlock(&data->update_lock);
 	return 0;
 }
+#else
+#define abituguru_suspend	NULL
+#define abituguru_resume	NULL
+#endif /* CONFIG_PM */
 
 static struct platform_driver abituguru_driver = {
 	.driver = {

Thanks,
-- 
Jean Delvare

[lm-sensors] PATCH: hwmon-abituguru-suspend-resume.patch

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans,
> 
>> This patch contains rudimentary suspend / resume support for the uguru,
>> this protects the uguru and the driver against suspend / resume cycles,
>> so there is no reason to unload the driver in your suspend / resume scripts.
>>
>> I'm not sure if this is needed, does the sysfs code itself guarantee
>> that no sysfs attr read/write calls are in progress before starting a
>> resume, if it does then this patch most likely isn't needed. Except for
>> protecting against this, it also checks the uguru is still in ready
>> status after a resume, but that seems to be unnescesarry (as it seems
>> that the uguru always is still ready after the resume).
> 
> I've noticed that most drivers take care to exclude the suspend/resume
> functions when CONFIG_PM is disabled. I agree it shouldn't matter much
> for the abituguru driver, as ACPI implies CONFIG_PM and I don't think
> anyone would run these boards with ACPI support. But in an attempt to
> make things as good as possible (in case others copy that code for
> other hwmon driver), what about stacking the following patch on top of
> yours?
> 
> 
> Only include suspend/resume functions when CONFIG_PM is set.
> 
> Signed-off-by: Jean Delvare <khali at linux-fr.org>
> ---
>  drivers/hwmon/abituguru.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> --- linux-2.6.18-rc5.orig/drivers/hwmon/abituguru.c	2006-08-28 11:09:39.000000000 +0200
> +++ linux-2.6.18-rc5/drivers/hwmon/abituguru.c	2006-08-28 11:31:26.000000000 +0200
> @@ -1354,6 +1354,7 @@
>  		return NULL;
>  }
>  
> +#ifdef CONFIG_PM
>  static int abituguru_suspend(struct platform_device *pdev, pm_message_t state)
>  {
>  	struct abituguru_data *data = platform_get_drvdata(pdev);
> @@ -1372,6 +1373,10 @@
>  	mutex_unlock(&data->update_lock);
>  	return 0;
>  }
> +#else
> +#define abituguru_suspend	NULL
> +#define abituguru_resume	NULL
> +#endif /* CONFIG_PM */
>  
>  static struct platform_driver abituguru_driver = {
>  	.driver = {
> 
> Thanks,


Looks good, feel free to apply it I'll apply it locally too.

Regards,

Hans

[lm-sensors] PATCH: hwmon-abituguru-suspend-resume.patch

[Jean Delvare]

Hans,

> > Only include suspend/resume functions when CONFIG_PM is set.
> > 
> > Signed-off-by: Jean Delvare <khali at linux-fr.org>
> > ---
> >  drivers/hwmon/abituguru.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > --- linux-2.6.18-rc5.orig/drivers/hwmon/abituguru.c	2006-08-28 11:09:39.000000000 +0200
> > +++ linux-2.6.18-rc5/drivers/hwmon/abituguru.c	2006-08-28 11:31:26.000000000 +0200
> > @@ -1354,6 +1354,7 @@
> >  		return NULL;
> >  }
> >  
> > +#ifdef CONFIG_PM
> >  static int abituguru_suspend(struct platform_device *pdev, pm_message_t state)
> >  {
> >  	struct abituguru_data *data = platform_get_drvdata(pdev);
> > @@ -1372,6 +1373,10 @@
> >  	mutex_unlock(&data->update_lock);
> >  	return 0;
> >  }
> > +#else
> > +#define abituguru_suspend	NULL
> > +#define abituguru_resume	NULL
> > +#endif /* CONFIG_PM */
> >  
> >  static struct platform_driver abituguru_driver = {
> >  	.driver = {
> > 
> 
> Looks good, feel free to apply it I'll apply it locally too.

Can you add your Signed-off-by line? So that I can merge both and push
the change as a single patch.

Thanks,
-- 
Jean Delvare

[lm-sensors] PATCH: hwmon-abituguru-suspend-resume.patch

[Hans de Goede]

 > Can you add your Signed-off-by line? So that I can merge both and push
 > the change as a single patch.
 >
 > Thanks,

Here you are, I hope my mailcient hasn't foo-barred the patch.

Only include suspend/resume functions when CONFIG_PM is set.

Signed-off-by: Jean Delvare <khali at linux-fr.org>
Signed-off-by: Hans de Goede <j.w.r.degoede at hhs.nl>
---
  drivers/hwmon/abituguru.c |    5 +++++
  1 file changed, 5 insertions(+)

--- linux-2.6.18-rc5.orig/drivers/hwmon/abituguru.c	2006-08-28 
11:09:39.000000000 +0200
+++ linux-2.6.18-rc5/drivers/hwmon/abituguru.c	2006-08-28 
11:31:26.000000000 +0200
@@ -1354,6 +1354,7 @@
  		return NULL;
  }

+#ifdef CONFIG_PM
  static int abituguru_suspend(struct platform_device *pdev, 
pm_message_t state)
  {
  	struct abituguru_data *data = platform_get_drvdata(pdev);
@@ -1372,6 +1373,10 @@
  	mutex_unlock(&data->update_lock);
  	return 0;
  }
+#else
+#define abituguru_suspend	NULL
+#define abituguru_resume	NULL
+#endif /* CONFIG_PM */

  static struct platform_driver abituguru_driver = {
  	.driver = {