linux-kernel - Re: [PATCH] hwmon: f71882fg: properly acquire I/O regions while probing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100324084643.GB6368@salidar.me.mortis.eu>
Date:	Wed, 24 Mar 2010 09:46:43 +0100
From:	Giel van Schijndel <me@...tis.eu>
To:	Hans de Goede <hdegoede@...hat.com>
Cc:	Jean Delvare <khali@...ux-fr.org>,
	Jonathan Cameron <jic23@....ac.uk>,
	Laurens Leemans <laurens@...nips.com>,
	lm-sensors@...sensors.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hwmon: f71882fg: properly acquire I/O regions while
 probing

On Wed, Mar 24, 2010 at 09:14:14AM +0100, Hans de Goede wrote:
> I don't have any objections against the proposed changes, but there
> are 3 unrelated changes in this patch, please break them up in
> separate patches:
> 
> 1) code cleanup: properly using, previously defined, functions rather
>    than duplicating their code.
> 2) properly acquire I/O regions while probing
> 3) Make the addresses to probe an array
> 
> And IMHO you might just as well drop number 3, it does not really make
> the code any better readable, and the old way is how all superio hwmon
> drivers do things.

Okay, broken up patches will follow as replies to this message.

Regarding number (3), my goal wasn't to put the probe addresses in an
array. My goal (as I think should have been made clear by the commit
message I had added) was to make sure that upon failure f71882fg_find's
return value gets passed back from the module's init function. This to
make sure the *proper* error code gets passed back to the user of
insmod/modprobe (as opposed to it being replaced by -ENODEV).

Further I think non -ENODEV errors should probably immediately result in
module initialisation failing (rather than retrying a probe), that's a
design choice though (which my patch doesn't bother addressing right
now).

However, regarding the for-loop/array thing, the same behaviour can be
aqcuired with a patch like the one following this line. (Please tell me
if you like this approach or something similar better. Though,
personally, I think it's "hackisher"/dirtier).

diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
index 4230729..42d6304 100644
--- a/drivers/hwmon/f71882fg.c
+++ b/drivers/hwmon/f71882fg.c
@@ -2295,9 +2295,11 @@ static int __init f71882fg_init(void)

        memset(&sio_data, 0, sizeof(sio_data));

-       if (f71882fg_find(0x2e, &address, &sio_data) &&
-           f71882fg_find(0x4e, &address, &sio_data))
-               goto exit;
+       if (f71882fg_find(0x2e, &address, &sio_data)) {
+               err = f71882fg_find(0x4e, &address, &sio_data);
+               if (err)
+                       goto exit;
+       }

        err = platform_driver_register(&f71882fg_driver);
        if (err)

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel

Download attachment "signature.asc" of type "application/pgp-signature" (199 bytes)