[lm-sensors] [PATCH] hwmon: Request the I/O regions in platform

[lm-sensors] [PATCH] hwmon: Request the I/O regions in platform

[Jean Delvare]

My understanding of the resource management in the Linux 2.6 device
driver model is that the devices should declare their resources, and
then when a driver attaches to a device, it should request the
resources it will be using, so as to mark them busy. This is how the
PCI and PNP subsystems work, you can clearly see the two levels of
resources (declaration and request) in /proc/ioports for these
devices.

So I believe that our platform hardware monitoring drivers should
follow the same logic. At the moment, we only declare the resources
but we do not request them. This patch adds the I/O regions request
and release calls.

It works fine with my Fintek F71805F chip. Juerg, can you please test
this patch with your VIA VT1211 chip?

Signed-off-by: Jean Delvare <khali at linux-fr.org>
---
 drivers/hwmon/f71805f.c |   16 +++++++++++++++-
 drivers/hwmon/pc87427.c |   15 ++++++++++++++-
 drivers/hwmon/vt1211.c  |   13 +++++++++++++
 3 files changed, 42 insertions(+), 2 deletions(-)

--- linux-2.6.21-rc1.orig/drivers/hwmon/f71805f.c	2007-02-21 08:34:22.000000000 +0100
+++ linux-2.6.21-rc1/drivers/hwmon/f71805f.c	2007-02-21 19:41:56.000000000 +0100
@@ -35,6 +35,7 @@
 #include <linux/err.h>
 #include <linux/mutex.h>
 #include <linux/sysfs.h>
+#include <linux/ioport.h>
 #include <asm/io.h>
 
 static struct platform_device *pdev;
@@ -1140,6 +1141,13 @@ static int __devinit f71805f_probe(struc
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (!request_region(res->start + ADDR_REG_OFFSET, 2, DRVNAME)) {
+		err = -EBUSY;
+		dev_err(&pdev->dev, "Failed to request region 0x%lx-0x%lx\n",
+			(unsigned long)(res->start + ADDR_REG_OFFSET),
+			(unsigned long)(res->start + ADDR_REG_OFFSET + 1));
+		goto exit_free;
+	}
 	data->addr = res->start;
 	data->name = names[sio_data->kind];
 	mutex_init(&data->update_lock);
@@ -1165,7 +1173,7 @@ static int __devinit f71805f_probe(struc
 
 	/* Register sysfs interface files */
 	if ((err = sysfs_create_group(&pdev->dev.kobj, &f71805f_group)))
-		goto exit_free;
+		goto exit_release_region;
 	if (data->has_in & (1 << 4)) { /* in4 */
 		if ((err = sysfs_create_group(&pdev->dev.kobj,
 					      &f71805f_group_optin[0])))
@@ -1219,6 +1227,8 @@ exit_remove_files:
 	for (i = 0; i < 4; i++)
 		sysfs_remove_group(&pdev->dev.kobj, &f71805f_group_optin[i]);
 	sysfs_remove_group(&pdev->dev.kobj, &f71805f_group_pwm_freq);
+exit_release_region:
+	release_region(res->start + ADDR_REG_OFFSET, 2);
 exit_free:
 	platform_set_drvdata(pdev, NULL);
 	kfree(data);
@@ -1229,6 +1239,7 @@ exit:
 static int __devexit f71805f_remove(struct platform_device *pdev)
 {
 	struct f71805f_data *data = platform_get_drvdata(pdev);
+	struct resource *res;
 	int i;
 
 	platform_set_drvdata(pdev, NULL);
@@ -1239,6 +1250,9 @@ static int __devexit f71805f_remove(stru
 	sysfs_remove_group(&pdev->dev.kobj, &f71805f_group_pwm_freq);
 	kfree(data);
 
+	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	release_region(res->start + ADDR_REG_OFFSET, 2);
+
 	return 0;
 }
 
--- linux-2.6.21-rc1.orig/drivers/hwmon/pc87427.c	2007-02-04 19:44:54.000000000 +0100
+++ linux-2.6.21-rc1/drivers/hwmon/pc87427.c	2007-02-21 19:21:16.000000000 +0100
@@ -31,6 +31,7 @@
 #include <linux/err.h>
 #include <linux/mutex.h>
 #include <linux/sysfs.h>
+#include <linux/ioport.h>
 #include <asm/io.h>
 
 static struct platform_device *pdev;
@@ -429,6 +430,12 @@ static int __devinit pc87427_probe(struc
 	/* This will need to be revisited when we add support for
 	   temperature and voltage monitoring. */
 	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (!request_region(res->start, res->end - res->start + 1, DRVNAME)) {
+		err = -EBUSY;
+		dev_err(&pdev->dev, "Failed to request region 0x%lx-0x%lx\n",
+			(unsigned long)res->start, (unsigned long)res->end);
+		goto exit_kfree;
+	}
 	data->address[0] = res->start;
 
 	mutex_init(&data->lock);
@@ -438,7 +445,7 @@ static int __devinit pc87427_probe(struc
 
 	/* Register sysfs hooks */
 	if ((err = device_create_file(&pdev->dev, &dev_attr_name)))
-		goto exit_kfree;
+		goto exit_release_region;
 	for (i = 0; i < 8; i++) {
 		if (!(data->fan_enabled & (1 << i)))
 			continue;
@@ -462,6 +469,8 @@ exit_remove_files:
 			continue;
 		sysfs_remove_group(&pdev->dev.kobj, &pc87427_group_fan[i]);
 	}
+exit_release_region:
+	release_region(res->start, res->end - res->start + 1);
 exit_kfree:
 	platform_set_drvdata(pdev, NULL);
 	kfree(data);
@@ -472,6 +481,7 @@ exit:
 static int __devexit pc87427_remove(struct platform_device *pdev)
 {
 	struct pc87427_data *data = platform_get_drvdata(pdev);
+	struct resource *res;
 	int i;
 
 	platform_set_drvdata(pdev, NULL);
@@ -484,6 +494,9 @@ static int __devexit pc87427_remove(stru
 	}
 	kfree(data);
 
+	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	release_region(res->start, res->end - res->start + 1);
+
 	return 0;
 }
 
--- linux-2.6.21-rc1.orig/drivers/hwmon/vt1211.c	2007-02-21 08:34:22.000000000 +0100
+++ linux-2.6.21-rc1/drivers/hwmon/vt1211.c	2007-02-21 19:45:36.000000000 +0100
@@ -31,6 +31,7 @@
 #include <linux/hwmon-vid.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
+#include <linux/ioport.h>
 #include <asm/io.h>
 
 static int uch_config = -1;
@@ -1130,6 +1131,12 @@ static int __devinit vt1211_probe(struct
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (!request_region(res->start, res->end - res->start + 1, DRVNAME)) {
+		err = -EBUSY;
+		dev_err(dev, "Failed to request region 0x%lx-0x%lx\n",
+			(unsigned long)res->start, (unsigned long)res->end);
+		goto EXIT_KFREE;
+	}
 	data->addr = res->start;
 	data->name = DRVNAME;
 	mutex_init(&data->update_lock);
@@ -1197,6 +1204,8 @@ EXIT_DEV_REMOVE:
 	dev_err(dev, "Sysfs interface creation failed (%d)\n", err);
 EXIT_DEV_REMOVE_SILENT:
 	vt1211_remove_sysfs(pdev);
+	release_region(res->start, res->end - res->start + 1);
+EXIT_KFREE:
 	platform_set_drvdata(pdev, NULL);
 	kfree(data);
 EXIT:
@@ -1206,12 +1215,16 @@ EXIT:
 static int __devexit vt1211_remove(struct platform_device *pdev)
 {
 	struct vt1211_data *data = platform_get_drvdata(pdev);
+	struct resource *res;
 
 	hwmon_device_unregister(data->class_dev);
 	vt1211_remove_sysfs(pdev);
 	platform_set_drvdata(pdev, NULL);
 	kfree(data);
 
+	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	release_region(res->start, res->end - res->start + 1);
+
 	return 0;
 }
 


-- 
Jean Delvare

[lm-sensors] [PATCH] hwmon: Request the I/O regions in platform

[Juerg Haefliger]

Jean,

I get
Trying to free nonexistent resource <0000000000006000-000000000000607f>
when unloading the module.

After module load, the resources are properly allocated (cat /proc/ioports):
6000-607f : vt1211
  6000-607f : vt1211

I ran into this very same problem in the past where I get this warning
after releasing a previously requested io region. Never figured out
what's going on...

...juerg


On 2/21/07, Jean Delvare <khali at linux-fr.org> wrote:
> My understanding of the resource management in the Linux 2.6 device
> driver model is that the devices should declare their resources, and
> then when a driver attaches to a device, it should request the
> resources it will be using, so as to mark them busy. This is how the
> PCI and PNP subsystems work, you can clearly see the two levels of
> resources (declaration and request) in /proc/ioports for these
> devices.
>
> So I believe that our platform hardware monitoring drivers should
> follow the same logic. At the moment, we only declare the resources
> but we do not request them. This patch adds the I/O regions request
> and release calls.
>
> It works fine with my Fintek F71805F chip. Juerg, can you please test
> this patch with your VIA VT1211 chip?
>
> Signed-off-by: Jean Delvare <khali at linux-fr.org>
> ---
>  drivers/hwmon/f71805f.c |   16 +++++++++++++++-
>  drivers/hwmon/pc87427.c |   15 ++++++++++++++-
>  drivers/hwmon/vt1211.c  |   13 +++++++++++++
>  3 files changed, 42 insertions(+), 2 deletions(-)
>
> --- linux-2.6.21-rc1.orig/drivers/hwmon/f71805f.c       2007-02-21 08:34:22.000000000 +0100
> +++ linux-2.6.21-rc1/drivers/hwmon/f71805f.c    2007-02-21 19:41:56.000000000 +0100
> @@ -35,6 +35,7 @@
>  #include <linux/err.h>
>  #include <linux/mutex.h>
>  #include <linux/sysfs.h>
> +#include <linux/ioport.h>
>  #include <asm/io.h>
>
>  static struct platform_device *pdev;
> @@ -1140,6 +1141,13 @@ static int __devinit f71805f_probe(struc
>         }
>
>         res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> +       if (!request_region(res->start + ADDR_REG_OFFSET, 2, DRVNAME)) {
> +               err = -EBUSY;
> +               dev_err(&pdev->dev, "Failed to request region 0x%lx-0x%lx\n",
> +                       (unsigned long)(res->start + ADDR_REG_OFFSET),
> +                       (unsigned long)(res->start + ADDR_REG_OFFSET + 1));
> +               goto exit_free;
> +       }
>         data->addr = res->start;
>         data->name = names[sio_data->kind];
>         mutex_init(&data->update_lock);
> @@ -1165,7 +1173,7 @@ static int __devinit f71805f_probe(struc
>
>         /* Register sysfs interface files */
>         if ((err = sysfs_create_group(&pdev->dev.kobj, &f71805f_group)))
> -               goto exit_free;
> +               goto exit_release_region;
>         if (data->has_in & (1 << 4)) { /* in4 */
>                 if ((err = sysfs_create_group(&pdev->dev.kobj,
>                                               &f71805f_group_optin[0])))
> @@ -1219,6 +1227,8 @@ exit_remove_files:
>         for (i = 0; i < 4; i++)
>                 sysfs_remove_group(&pdev->dev.kobj, &f71805f_group_optin[i]);
>         sysfs_remove_group(&pdev->dev.kobj, &f71805f_group_pwm_freq);
> +exit_release_region:
> +       release_region(res->start + ADDR_REG_OFFSET, 2);
>  exit_free:
>         platform_set_drvdata(pdev, NULL);
>         kfree(data);
> @@ -1229,6 +1239,7 @@ exit:
>  static int __devexit f71805f_remove(struct platform_device *pdev)
>  {
>         struct f71805f_data *data = platform_get_drvdata(pdev);
> +       struct resource *res;
>         int i;
>
>         platform_set_drvdata(pdev, NULL);
> @@ -1239,6 +1250,9 @@ static int __devexit f71805f_remove(stru
>         sysfs_remove_group(&pdev->dev.kobj, &f71805f_group_pwm_freq);
>         kfree(data);
>
> +       res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> +       release_region(res->start + ADDR_REG_OFFSET, 2);
> +
>         return 0;
>  }
>
> --- linux-2.6.21-rc1.orig/drivers/hwmon/pc87427.c       2007-02-04 19:44:54.000000000 +0100
> +++ linux-2.6.21-rc1/drivers/hwmon/pc87427.c    2007-02-21 19:21:16.000000000 +0100
> @@ -31,6 +31,7 @@
>  #include <linux/err.h>
>  #include <linux/mutex.h>
>  #include <linux/sysfs.h>
> +#include <linux/ioport.h>
>  #include <asm/io.h>
>
>  static struct platform_device *pdev;
> @@ -429,6 +430,12 @@ static int __devinit pc87427_probe(struc
>         /* This will need to be revisited when we add support for
>            temperature and voltage monitoring. */
>         res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> +       if (!request_region(res->start, res->end - res->start + 1, DRVNAME)) {
> +               err = -EBUSY;
> +               dev_err(&pdev->dev, "Failed to request region 0x%lx-0x%lx\n",
> +                       (unsigned long)res->start, (unsigned long)res->end);
> +               goto exit_kfree;
> +       }
>         data->address[0] = res->start;
>
>         mutex_init(&data->lock);
> @@ -438,7 +445,7 @@ static int __devinit pc87427_probe(struc
>
>         /* Register sysfs hooks */
>         if ((err = device_create_file(&pdev->dev, &dev_attr_name)))
> -               goto exit_kfree;
> +               goto exit_release_region;
>         for (i = 0; i < 8; i++) {
>                 if (!(data->fan_enabled & (1 << i)))
>                         continue;
> @@ -462,6 +469,8 @@ exit_remove_files:
>                         continue;
>                 sysfs_remove_group(&pdev->dev.kobj, &pc87427_group_fan[i]);
>         }
> +exit_release_region:
> +       release_region(res->start, res->end - res->start + 1);
>  exit_kfree:
>         platform_set_drvdata(pdev, NULL);
>         kfree(data);
> @@ -472,6 +481,7 @@ exit:
>  static int __devexit pc87427_remove(struct platform_device *pdev)
>  {
>         struct pc87427_data *data = platform_get_drvdata(pdev);
> +       struct resource *res;
>         int i;
>
>         platform_set_drvdata(pdev, NULL);
> @@ -484,6 +494,9 @@ static int __devexit pc87427_remove(stru
>         }
>         kfree(data);
>
> +       res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> +       release_region(res->start, res->end - res->start + 1);
> +
>         return 0;
>  }
>
> --- linux-2.6.21-rc1.orig/drivers/hwmon/vt1211.c        2007-02-21 08:34:22.000000000 +0100
> +++ linux-2.6.21-rc1/drivers/hwmon/vt1211.c     2007-02-21 19:45:36.000000000 +0100
> @@ -31,6 +31,7 @@
>  #include <linux/hwmon-vid.h>
>  #include <linux/err.h>
>  #include <linux/mutex.h>
> +#include <linux/ioport.h>
>  #include <asm/io.h>
>
>  static int uch_config = -1;
> @@ -1130,6 +1131,12 @@ static int __devinit vt1211_probe(struct
>         }
>
>         res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> +       if (!request_region(res->start, res->end - res->start + 1, DRVNAME)) {
> +               err = -EBUSY;
> +               dev_err(dev, "Failed to request region 0x%lx-0x%lx\n",
> +                       (unsigned long)res->start, (unsigned long)res->end);
> +               goto EXIT_KFREE;
> +       }
>         data->addr = res->start;
>         data->name = DRVNAME;
>         mutex_init(&data->update_lock);
> @@ -1197,6 +1204,8 @@ EXIT_DEV_REMOVE:
>         dev_err(dev, "Sysfs interface creation failed (%d)\n", err);
>  EXIT_DEV_REMOVE_SILENT:
>         vt1211_remove_sysfs(pdev);
> +       release_region(res->start, res->end - res->start + 1);
> +EXIT_KFREE:
>         platform_set_drvdata(pdev, NULL);
>         kfree(data);
>  EXIT:
> @@ -1206,12 +1215,16 @@ EXIT:
>  static int __devexit vt1211_remove(struct platform_device *pdev)
>  {
>         struct vt1211_data *data = platform_get_drvdata(pdev);
> +       struct resource *res;
>
>         hwmon_device_unregister(data->class_dev);
>         vt1211_remove_sysfs(pdev);
>         platform_set_drvdata(pdev, NULL);
>         kfree(data);
>
> +       res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> +       release_region(res->start, res->end - res->start + 1);
> +
>         return 0;
>  }
>
>
>
> --
> Jean Delvare
>

[lm-sensors] [PATCH] hwmon: Request the I/O regions in platform

[Jean Delvare]

Hi Juerg,

On Thu, 22 Feb 2007 08:14:53 -0800, Juerg Haefliger wrote:
> I get
> Trying to free nonexistent resource <0000000000006000-000000000000607f>
> when unloading the module.
> 
> After module load, the resources are properly allocated (cat /proc/ioports):
> 6000-607f : vt1211
>   6000-607f : vt1211
> 
> I ran into this very same problem in the past where I get this warning
> after releasing a previously requested io region. Never figured out
> what's going on...

See this thread on LKML:
http://marc.theaimsgroup.com/?l=linux-kernel&m\x117183073419531&w=2

I posted a proposed fix, Dmitry Torokhov doesn't appear to like it. I'm
waiting for Greg KH's arbitration.

You can get my patch here if you want to give it a try:
http://lkml.org/lkml/diff/2007/2/20/310/1

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH] hwmon: Request the I/O regions in platform

[Juerg Haefliger]

Hi Jean,

OK, with your patch (http://lkml.org/lkml/diff/2007/2/20/310/1)
applied, it works as advertised :-)
However, your platform-io-region patch doesn't apply cleanly:

juno:/usr/src/linux-2.6.21-rc1# patch -p1 < ../jean.patch
patching file drivers/hwmon/f71805f.c
Hunk #1 succeeded at 35 with fuzz 2.
missing header for unified diff at line 11 of patch
can't find file to patch at input line 11
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|
| static struct platform_device *pdev;
--------------------------
File to patch:



...juerg



On 2/22/07, Jean Delvare <khali at linux-fr.org> wrote:
> Hi Juerg,
>
> On Thu, 22 Feb 2007 08:14:53 -0800, Juerg Haefliger wrote:
> > I get
> > Trying to free nonexistent resource <0000000000006000-000000000000607f>
> > when unloading the module.
> >
> > After module load, the resources are properly allocated (cat /proc/ioports):
> > 6000-607f : vt1211
> >   6000-607f : vt1211
> >
> > I ran into this very same problem in the past where I get this warning
> > after releasing a previously requested io region. Never figured out
> > what's going on...
>
> See this thread on LKML:
> http://marc.theaimsgroup.com/?l=linux-kernel&m\x117183073419531&w=2
>
> I posted a proposed fix, Dmitry Torokhov doesn't appear to like it. I'm
> waiting for Greg KH's arbitration.
>
> You can get my patch here if you want to give it a try:
> http://lkml.org/lkml/diff/2007/2/20/310/1
>
> Thanks,
> --
> Jean Delvare
>

[lm-sensors] [PATCH] hwmon: Request the I/O regions in platform

[Jean Delvare]

On Thu, 22 Feb 2007 12:06:18 -0800, Juerg Haefliger wrote:
> OK, with your patch (http://lkml.org/lkml/diff/2007/2/20/310/1)
> applied, it works as advertised :-)

Great, thanks for testing and reporting.

> However, your platform-io-region patch doesn't apply cleanly:
> 
> juno:/usr/src/linux-2.6.21-rc1# patch -p1 < ../jean.patch
> patching file drivers/hwmon/f71805f.c
> Hunk #1 succeeded at 35 with fuzz 2.
> missing header for unified diff at line 11 of patch
> can't find file to patch at input line 11
> Perhaps you used the wrong -p or --strip option?
> The text leading up to this was:
> --------------------------
> |
> | static struct platform_device *pdev;
> --------------------------
> File to patch:

I don't understand, I thought you had already applied that one? Either
way, it smells like a corrupted patch file. It's probably visible if
you open the patch file with a text editor. I guess your mailer saved
it with some encoding and you'd need to fix it before you can apply it
(e.g. using metamail.)

-- 
Jean Delvare

[lm-sensors] [PATCH] hwmon: Request the I/O regions in platform

[David Hubbard]

Hi Jean, Juerg,

On 2/21/07, Jean Delvare <khali at linux-fr.org> wrote:
> My understanding of the resource management in the Linux 2.6 device
> driver model is that the devices should declare their resources, and
> then when a driver attaches to a device, it should request the
> resources it will be using, so as to mark them busy. This is how the
> PCI and PNP subsystems work, you can clearly see the two levels of
> resources (declaration and request) in /proc/ioports for these
> devices.
>
> So I believe that our platform hardware monitoring drivers should
> follow the same logic. At the moment, we only declare the resources
> but we do not request them. This patch adds the I/O regions request
> and release calls.

Would this have any effect with the discussion Rudolf Marek started
about ACPI Thermal monitoring and a race condition with a hwmon driver
(e.g. w83627ehf), resulting in an invalid reading (which sets off the
ACPI alarm and shuts down the system)?

Would requesting the resources prevent the ACPI Thermal AML from
accessing the chip? Perhaps I have not understood the ioports region
code correctly, but it would not make sense to allow something like
the AML parser to access IO regions marked busy.

Please comment if you would,
David

[lm-sensors] [PATCH] hwmon: Request the I/O regions in platform

[Jean Delvare]

Hi David,

On Sat, 24 Feb 2007 15:06:16 -0600, David Hubbard wrote:
> Hi Jean, Juerg,
> 
> On 2/21/07, Jean Delvare <khali at linux-fr.org> wrote:
> > My understanding of the resource management in the Linux 2.6 device
> > driver model is that the devices should declare their resources, and
> > then when a driver attaches to a device, it should request the
> > resources it will be using, so as to mark them busy. This is how the
> > PCI and PNP subsystems work, you can clearly see the two levels of
> > resources (declaration and request) in /proc/ioports for these
> > devices.
> >
> > So I believe that our platform hardware monitoring drivers should
> > follow the same logic. At the moment, we only declare the resources
> > but we do not request them. This patch adds the I/O regions request
> > and release calls.
> 
> Would this have any effect with the discussion Rudolf Marek started
> about ACPI Thermal monitoring and a race condition with a hwmon driver
> (e.g. w83627ehf), resulting in an invalid reading (which sets off the
> ACPI alarm and shuts down the system)?
> 
> Would requesting the resources prevent the ACPI Thermal AML from
> accessing the chip? Perhaps I have not understood the ioports region
> code correctly, but it would not make sense to allow something like
> the AML parser to access IO regions marked busy.

It is probably one step in the right direction. However it won't solve
the problem per se, because as far as I know, the ACPI/AML code doesn't
check for regions at all at the moment. But it definitely should one
way or another, so we must do our part of the job and properly declare
the regions we are using.

One problem which will certainly arise soon though, is that the ACPI
thermal module does more than just reporting temperatures, it can also
control thermal throttling, emergency shutdown etc. In that sense, some
may argue that it should get a prioritary access over hwmon drivers.
This can only be done if the ACPI/AML code doesn't only check for busy
regions, but actually request the regions it'll be using - pretty much
how every other driver is supposed to do. Not being familiar with ACPI,
I don't know how feasable this is.

Ideally we would be able to synchronize accesses between ACPI/AML and
the other drivers, I think Rudolf made a proposal in that sense. But I
see this as a second step. The first step would be to just prevent
collisions because they are hard to diagnose and the results are
impossible to predict. If drivers would cleanly lock each other out, at
least we would know when conflicts exist.

-- 
Jean Delvare