[lm-sensors] [PATCH] hwmon: (pc87360) Fix device resource

[lm-sensors] [PATCH] hwmon: (pc87360) Fix device resource

[Jean Delvare]

It's not OK to call platform_device_add_resources() multiple times
in a row. Despite its name, this functions sets the resources, it
doesn't add them. So we have to prepare an array with all the
resources, and then call platform_device_add_resources() once.

Before this fix, only the last I/O resource would be actually
registered. The other I/O resources were leaked.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Jim Cromie <jim.cromie@gmail.com>
Cc: stable@kernel.org
---
Jim, do you still have your system with a PC8736x device? If you do,
can you please test this fix and report? Thanks.

 drivers/hwmon/pc87360.c |   30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

--- linux-2.6.35-rc4.orig/drivers/hwmon/pc87360.c	2010-05-17 18:20:27.000000000 +0200
+++ linux-2.6.35-rc4/drivers/hwmon/pc87360.c	2010-07-07 14:11:03.000000000 +0200
@@ -1610,11 +1610,8 @@ static struct pc87360_data *pc87360_upda
 
 static int __init pc87360_device_add(unsigned short address)
 {
-	struct resource res = {
-		.name	= "pc87360",
-		.flags	= IORESOURCE_IO,
-	};
-	int err, i;
+	struct resource res[3];
+	int err, i, res_count;
 
 	pdev = platform_device_alloc("pc87360", address);
 	if (!pdev) {
@@ -1623,22 +1620,27 @@ static int __init pc87360_device_add(uns
 		goto exit;
 	}
 
+	memset(res, 0, 3 * sizeof(struct resource));
 	for (i = 0; i < 3; i++) {
 		if (!extra_isa[i])
 			continue;
-		res.start = extra_isa[i];
-		res.end = extra_isa[i] + PC87360_EXTENT - 1;
+		res[res_count].start = extra_isa[i];
+		res[res_count].end = extra_isa[i] + PC87360_EXTENT - 1;
+		res[res_count].name = "pc87360",
+		res[res_count].flags = IORESOURCE_IO,
 
-		err = acpi_check_resource_conflict(&res);
+		err = acpi_check_resource_conflict(&res[res_count]);
 		if (err)
 			goto exit_device_put;
 
-		err = platform_device_add_resources(pdev, &res, 1);
-		if (err) {
-			printk(KERN_ERR "pc87360: Device resource[%d] "
-			       "addition failed (%d)\n", i, err);
-			goto exit_device_put;
-		}
+		res_count++;
+	}
+
+	err = platform_device_add_resources(pdev, res, res_count);
+	if (err) {
+		printk(KERN_ERR "pc87360: Device resources addition failed "
+		       "(%d)\n", err);
+		goto exit_device_put;
 	}
 
 	err = platform_device_add(pdev);


-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (pc87360) Fix device resource

[Jim Cromie]

hi Jean

On Wed, Jul 7, 2010 at 6:58 AM, Jean Delvare <khali@linux-fr.org> wrote:
> It's not OK to call platform_device_add_resources() multiple times
> in a row. Despite its name, this functions sets the resources, it
> doesn't add them. So we have to prepare an array with all the
> resources, and then call platform_device_add_resources() once.
>
> Before this fix, only the last I/O resource would be actually
> registered. The other I/O resources were leaked.
>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Cc: Jim Cromie <jim.cromie@gmail.com>
> Cc: stable@kernel.org
> ---
> Jim, do you still have your system with a PC8736x device? If you do,
> can you please test this fix and report? Thanks.

Its not handy, but I see about arranging access.

Did you catch this by inspection?
was there another thread that triggered your recollection/association ?
Any simple check for this ?
I dont suppose a /proc/<file> is gonna shout RESOURCE LEAK ..
that said, I'll look before&after for something more subtle.

thanks Jean
Jim

>
>  drivers/hwmon/pc87360.c |   30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
>
> --- linux-2.6.35-rc4.orig/drivers/hwmon/pc87360.c       2010-05-17 18:20:27.000000000 +0200
> +++ linux-2.6.35-rc4/drivers/hwmon/pc87360.c    2010-07-07 14:11:03.000000000 +0200
> @@ -1610,11 +1610,8 @@ static struct pc87360_data *pc87360_upda
>
>  static int __init pc87360_device_add(unsigned short address)
>  {
> -       struct resource res = {
> -               .name   = "pc87360",
> -               .flags  = IORESOURCE_IO,
> -       };
> -       int err, i;
> +       struct resource res[3];
> +       int err, i, res_count;
>
>        pdev = platform_device_alloc("pc87360", address);
>        if (!pdev) {
> @@ -1623,22 +1620,27 @@ static int __init pc87360_device_add(uns
>                goto exit;
>        }
>
> +       memset(res, 0, 3 * sizeof(struct resource));
>        for (i = 0; i < 3; i++) {
>                if (!extra_isa[i])
>                        continue;
> -               res.start = extra_isa[i];
> -               res.end = extra_isa[i] + PC87360_EXTENT - 1;
> +               res[res_count].start = extra_isa[i];
> +               res[res_count].end = extra_isa[i] + PC87360_EXTENT - 1;
> +               res[res_count].name = "pc87360",
> +               res[res_count].flags = IORESOURCE_IO,
>
> -               err = acpi_check_resource_conflict(&res);
> +               err = acpi_check_resource_conflict(&res[res_count]);
>                if (err)
>                        goto exit_device_put;
>
> -               err = platform_device_add_resources(pdev, &res, 1);
> -               if (err) {
> -                       printk(KERN_ERR "pc87360: Device resource[%d] "
> -                              "addition failed (%d)\n", i, err);
> -                       goto exit_device_put;
> -               }
> +               res_count++;
> +       }
> +
> +       err = platform_device_add_resources(pdev, res, res_count);
> +       if (err) {
> +               printk(KERN_ERR "pc87360: Device resources addition failed "
> +                      "(%d)\n", err);
> +               goto exit_device_put;
>        }
>
>        err = platform_device_add(pdev);
>
>
> --
> Jean Delvare
>

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (pc87360) Fix device resource

[Jean Delvare]

Hi Jim,

On Wed, 7 Jul 2010 08:52:01 -0600, Jim Cromie wrote:
> On Wed, Jul 7, 2010 at 6:58 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > It's not OK to call platform_device_add_resources() multiple times
> > in a row. Despite its name, this functions sets the resources, it
> > doesn't add them. So we have to prepare an array with all the
> > resources, and then call platform_device_add_resources() once.
> >
> > Before this fix, only the last I/O resource would be actually
> > registered. The other I/O resources were leaked.
> >
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > Cc: Jim Cromie <jim.cromie@gmail.com>
> > Cc: stable@kernel.org
> > ---
> > Jim, do you still have your system with a PC8736x device? If you do,
> > can you please test this fix and report? Thanks.
> 
> Its not handy, but I see about arranging access.
> 
> Did you catch this by inspection?
> was there another thread that triggered your recollection/association ?
> Any simple check for this ?

I copied this code to the pc87427 driver yesterday, as I am in the
process of adding support for temperature monitoring to that driver
(currently it only supports fans). The pc87427 driver code is slightly
cleaner than the pc87360 driver code, in that it doesn't use a global
array to carry the multiple I/O addresses of the logical devices.
Instead, it retrieves them using platform_get_resource(). I found it
worked OK for the first resource, and returned NULL for the second.
Which made me realize that my two calls to
platform_device_add_resources() had resulted in a single resource being
registered. Further code inspection (of platform_device_add_resources)
revealed that both had been allocated, but the first one got lost when
the second was added.

I've fixed the pc87427 driver already, and now it's the pc87360
driver's turn.

> I dont suppose a /proc/<file> is gonna shout RESOURCE LEAK ..

Indeed not. I know it leaked memory by code inspection, I didn't
witness it. Unless you cycle the pc87360 driver, the leak of a few
dozen bytes is unnoticeable.

> that said, I'll look before&after for something more subtle.

You should definitely see a difference in /proc/ioports before and
after my fix.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (pc87360) Fix device resource

[Jim Cromie]

[-- Attachment #1.1: Type: text/plain, Size: 2088 bytes --]

On Wed, Jul 7, 2010 at 6:58 AM, Jean Delvare <khali@linux-fr.org> wrote:

> It's not OK to call platform_device_add_resources() multiple times
> in a row. Despite its name, this functions sets the resources, it
> doesn't add them. So we have to prepare an array with all the
> resources, and then call platform_device_add_resources() once.
>
> Before this fix, only the last I/O resource would be actually
> registered. The other I/O resources were leaked.
>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Cc: Jim Cromie <jim.cromie@gmail.com>
> Cc: stable@kernel.org
> ---
> Jim, do you still have your system with a PC8736x device? If you do,
> can you please test this fix and report? Thanks.
>
>
hi Jean,

I applied this patch, and it built w/o errors.
Howver I have some distro (voyage 0.6.5) vs kernel issues
to resolve before I have a solid testing env.
It looks like 2.6.35 doesnt like udev 145, which shipped along with 2.6.30
kernel

[   11.252182] udev: starting version 145
[   11.252232] <3>udev: missing sysfs features; please update the kernel or
disable the kernel's CONFIG_SYSFS_DEPRECATED option; udev may fail to work
correctly

and it does fail to work correctly;
voyage:~# modprobe pc87360
WARNING: Could not open 'kernel/drivers/hwmon/hwmon-vid.ko': No such file or
directory
FATAL: Could not open 'kernel/drivers/hwmon/pc87360.ko': No such file or
directory

I'll try to straighten this out, and follow up on hotplug ml if I need help.

In the meantime, it looks like res_count isnt initialized before 1st use.

jimc@harpo linux-2.6.git]$ grep -n res_count drivers/hwmon/pc87360.c
1614: int err, i, res_count;
1627: res[res_count].start = extra_isa[i];
1628: res[res_count].end = extra_isa[i] + PC87360_EXTENT - 1;
1629: res[res_count].name = "pc87360",
1630: res[res_count].flags = IORESOURCE_IO,
1632: err = acpi_check_resource_conflict(&res[res_count]);
1636: res_count++;
1639: err = platform_device_add_resources(pdev, res, res_count);

Oddly, gcc doesnt complain about this,
is this somehow hidden by zeroed memory due to __init on the function ??

Jim

[-- Attachment #1.2: Type: text/html, Size: 3462 bytes --]

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

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (pc87360) Fix device resource

[Jean Delvare]

Hi Jim,

On Sat, 7 Aug 2010 05:23:20 -0600, Jim Cromie wrote:
> On Wed, Jul 7, 2010 at 6:58 AM, Jean Delvare <khali@linux-fr.org> wrote:
> 
> > It's not OK to call platform_device_add_resources() multiple times
> > in a row. Despite its name, this functions sets the resources, it
> > doesn't add them. So we have to prepare an array with all the
> > resources, and then call platform_device_add_resources() once.
> >
> > Before this fix, only the last I/O resource would be actually
> > registered. The other I/O resources were leaked.
> >
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > Cc: Jim Cromie <jim.cromie@gmail.com>
> > Cc: stable@kernel.org
> > ---
> > Jim, do you still have your system with a PC8736x device? If you do,
> > can you please test this fix and report? Thanks.
> >
> >
> hi Jean,
> 
> I applied this patch, and it built w/o errors.
> Howver I have some distro (voyage 0.6.5) vs kernel issues
> to resolve before I have a solid testing env.
> It looks like 2.6.35 doesnt like udev 145, which shipped along with 2.6.30
> kernel
> 
> [   11.252182] udev: starting version 145
> [   11.252232] <3>udev: missing sysfs features; please update the kernel or
> disable the kernel's CONFIG_SYSFS_DEPRECATED option; udev may fail to work
> correctly

I can't help you much with this, I'm afraid. Just try what they say...

> and it does fail to work correctly;
> voyage:~# modprobe pc87360
> WARNING: Could not open 'kernel/drivers/hwmon/hwmon-vid.ko': No such file or
> directory
> FATAL: Could not open 'kernel/drivers/hwmon/pc87360.ko': No such file or
> directory
> 
> I'll try to straighten this out, and follow up on hotplug ml if I need help.

The errors above seem related to module dependencies and/or
installation, so I'd look at how and where the kernel modules were
installed on the system, if module-init-tools is up-to-date, try
running "depmod -a", see if "modprobe -c" complains. If nothing helps,
try running modprobe through strace, to see what exactly is failing.

> In the meantime, it looks like res_count isnt initialized before 1st use.
> 
> jimc@harpo linux-2.6.git]$ grep -n res_count drivers/hwmon/pc87360.c
> 1614: int err, i, res_count;
> 1627: res[res_count].start = extra_isa[i];
> 1628: res[res_count].end = extra_isa[i] + PC87360_EXTENT - 1;
> 1629: res[res_count].name = "pc87360",
> 1630: res[res_count].flags = IORESOURCE_IO,
> 1632: err = acpi_check_resource_conflict(&res[res_count]);
> 1636: res_count++;
> 1639: err = platform_device_add_resources(pdev, res, res_count);
> 
> Oddly, gcc doesnt complain about this,
> is this somehow hidden by zeroed memory due to __init on the function ??

This is indeed a bug, thanks a lot for finding and reporting it. It's
fixed now.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (pc87360) Fix device resource

[Jim Cromie]

[-- Attachment #1.1: Type: text/plain, Size: 3389 bytes --]

On Wed, Aug 11, 2010 at 6:59 AM, Jean Delvare <khali@linux-fr.org> wrote:

> Hi Jim,
>
> On Sat, 7 Aug 2010 05:23:20 -0600, Jim Cromie wrote:
> > On Wed, Jul 7, 2010 at 6:58 AM, Jean Delvare <khali@linux-fr.org> wrote:
> >
> > > It's not OK to call platform_device_add_resources() multiple times
> > > in a row. Despite its name, this functions sets the resources, it
> > > doesn't add them. So we have to prepare an array with all the
>

snip


> > I applied this patch, and it built w/o errors.
>
> Howver I have some distro (voyage 0.6.5) vs kernel issues
> > to resolve before I have a solid testing env.
> > It looks like 2.6.35 doesnt like udev 145, which shipped along with
> 2.6.30
> > kernel
> >
> > [   11.252182] udev: starting version 145
> > [   11.252232] <3>udev: missing sysfs features; please update the kernel
> or
> > disable the kernel's CONFIG_SYSFS_DEPRECATED option; udev may fail to
> work
> > correctly
>
> I can't help you much with this, I'm afraid. Just try what they say...
>
> FTR - udev 145 is added into the initramfs file from my laptop,
not from voyage-linux.  So I should be able to fix this and test soon.

 > In the meantime, it looks like res_count isnt initialized before 1st use.

> >
> > Oddly, gcc doesnt complain about this,
> > is this somehow hidden by zeroed memory due to __init on the function ??
>
> This is indeed a bug, thanks a lot for finding and reporting it. It's
> fixed now.
>
>
WRT lack of uninit-var warning,

[jimc@harpo sk1]$ make V=1 drivers/hwmon/pc87360.ko
...
  gcc -Wp,-MD,drivers/hwmon/.pc87360.o.d  -nostdinc -isystem
/usr/lib/gcc/i686-redhat-linux/4.4.4/include
-I/home/jimc/lx/linux-2.6.git/arch/x86/include -Iinclude
 -I/home/jimc/lx/linux-2.6.git/include -include include/generated/autoconf.h
 -I/home/jimc/lx/linux-2.6.git/drivers/hwmon -Idrivers/hwmon -D__KERNEL__
-Wall -Wundef -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing
-fno-common -Werror-implicit-function-declaration -Wno-format-security
-fno-delete-null-pointer-checks -Os -m32 -msoft-float -mregparm=3
-freg-struct-return -mpreferred-stack-boundary=2 -march=pentium-mmx
-mtune=generic -Wa,-mtune=generic32 -ffreestanding -DCONFIG_AS_CFI=1
-DCONFIG_AS_CFI_SIGNAL_FRAME=1 -DCONFIG_AS_CFI_SECTIONS=1 -pipe
-Wno-sign-compare -fno-asynchronous-unwind-tables -mno-sse -mno-mmx
-mno-sse2 -mno-3dnow -Wframe-larger-than=1024 -fno-stack-protector
-fno-omit-frame-pointer -fno-optimize-sibling-calls
-Wdeclaration-after-statement -Wno-pointer-sign -fno-strict-overflow
-fconserve-stack  -DMODULE -D"KBUILD_STR(s)=#s"
-D"KBUILD_BASENAME=KBUILD_STR(pc87360)"
 -D"KBUILD_MODNAME=KBUILD_STR(pc87360)"  -c -o drivers/hwmon/pc87360.o
/home/jimc/lx/linux-2.6.git/drivers/hwmon/pc87360.c
make -f /home/jimc/lx/linux-2.6.git/scripts/Makefile.modpost
  scripts/mod/modpost   -o /home/jimc/lx/sk1/Module.symvers    -S   -s
WARNING: modpost: Found 21 section mismatch(es).

-Wuninitialized  is not in there (explaining lack of warnings)

is this a local config problem, or has it somehow dropped out of the Kconfig
process ?
or was it never in there (seems unlikely) ?

also, regarding your patch, is there a real need for res_count at all ?
IOW, why not this (abbreviated) ?

- res[res_count].start = extra_isa[i];
+ res[i].start = extra_isa[i];
...
- err = platform_device_add_resources(pdev, res, res_count);
+ err = platform_device_add_resources(pdev, res, --i);

[-- Attachment #1.2: Type: text/html, Size: 4993 bytes --]

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

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (pc87360) Fix device resource

[Jean Delvare]

Hi Jim,

On Wed, 11 Aug 2010 15:15:31 -0600, Jim Cromie wrote:
> On Wed, Aug 11, 2010 at 6:59 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > > In the meantime, it looks like res_count isnt initialized before 1st use.
> > > (...)
> > > Oddly, gcc doesnt complain about this,
> > > is this somehow hidden by zeroed memory due to __init on the function ??
> >
> > This is indeed a bug, thanks a lot for finding and reporting it. It's
> > fixed now.
>
> WRT lack of uninit-var warning,
> 
> [jimc@harpo sk1]$ make V=1 drivers/hwmon/pc87360.ko
> ...
>   gcc -Wp,-MD,drivers/hwmon/.pc87360.o.d  -nostdinc -isystem
> /usr/lib/gcc/i686-redhat-linux/4.4.4/include
> -I/home/jimc/lx/linux-2.6.git/arch/x86/include -Iinclude
>  -I/home/jimc/lx/linux-2.6.git/include -include include/generated/autoconf.h
>  -I/home/jimc/lx/linux-2.6.git/drivers/hwmon -Idrivers/hwmon -D__KERNEL__
> -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing
> -fno-common -Werror-implicit-function-declaration -Wno-format-security
> -fno-delete-null-pointer-checks -Os -m32 -msoft-float -mregparm=3
> -freg-struct-return -mpreferred-stack-boundary=2 -march=pentium-mmx
> -mtune=generic -Wa,-mtune=generic32 -ffreestanding -DCONFIG_AS_CFI=1
> -DCONFIG_AS_CFI_SIGNAL_FRAME=1 -DCONFIG_AS_CFI_SECTIONS=1 -pipe
> -Wno-sign-compare -fno-asynchronous-unwind-tables -mno-sse -mno-mmx
> -mno-sse2 -mno-3dnow -Wframe-larger-than\x1024 -fno-stack-protector
> -fno-omit-frame-pointer -fno-optimize-sibling-calls
> -Wdeclaration-after-statement -Wno-pointer-sign -fno-strict-overflow
> -fconserve-stack  -DMODULE -D"KBUILD_STR(s)=#s"
> -D"KBUILD_BASENAME=KBUILD_STR(pc87360)"
>  -D"KBUILD_MODNAME=KBUILD_STR(pc87360)"  -c -o drivers/hwmon/pc87360.o
> /home/jimc/lx/linux-2.6.git/drivers/hwmon/pc87360.c
> make -f /home/jimc/lx/linux-2.6.git/scripts/Makefile.modpost
>   scripts/mod/modpost   -o /home/jimc/lx/sk1/Module.symvers    -S   -s
> WARNING: modpost: Found 21 section mismatch(es).
> 
> -Wuninitialized  is not in there (explaining lack of warnings)
> 
> is this a local config problem, or has it somehow dropped out of the Kconfig
> process ?
> or was it never in there (seems unlikely) ?

-Wuninitialized is implied by -Wall (which is there) "only with -O1 and
above" (quoting the man page). I'm not sure if -Os qualifies. But
anyway I'm not using CONFIG_CC_OPTIMIZE_FOR_SIZE and didn't get the
warning either...

Maybe this option is simply not 100% reliable. After all, it prints
false positives every now and then, so maybe it also fails to catch
some bugs.

> also, regarding your patch, is there a real need for res_count at all ?
> IOW, why not this (abbreviated) ?
> 
> - res[res_count].start = extra_isa[i];
> + res[i].start = extra_isa[i];
> ...
> - err = platform_device_add_resources(pdev, res, res_count);
> + err = platform_device_add_resources(pdev, res, --i);

We're iterating over 2 different arrays: res and extra_isa. res_count
is the index for res[], i is the index for extra_isa[]. If all
resources are present then both are in sync, but if any resource is
missing then they will drift. So no, I can't see any way to get rid of
res_count.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors