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: <20100324102816.36d70c18@hyperion.delvare>
Date:	Wed, 24 Mar 2010 10:28:16 +0100
From:	Jean Delvare <khali@...ux-fr.org>
To:	Giel van Schijndel <me@...tis.eu>
Cc:	Hans de Goede <hdegoede@...hat.com>,
	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

Hi Giel,

On Wed, 24 Mar 2010 09:46:43 +0100, Giel van Schijndel wrote:
> 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).

This doesn't make much sense. We call f71882fg_find up to twice, so we
can get up to 2 error codes, but the module init function can only
return one. There is no rationale for favoring the value returned by
the second call over the value returned by the first call. So it seems
reasonable to return an arbitrary error code. Of course we could spend
code comparing the error values and returning the right one if the two
functions returned the same error value, and only use the arbitrary
default if they returned different values. But honestly I don't see any
point in doing this in practice. Let's just log errors other than
-ENODEV so that the user can see the exact cause of failure in the
kernel log.

> 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).

I disagree. If the probe of 0x2e/0x2f fails because that region was busy,
this should not prevent us from giving a try to region 0x4e/0x4f. I
can't think of any error that would warrant avoiding the probe at
0x4e/0x4f. Worse thing that can happen is that the second probe fails
too. No big deal.

> 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)
> 

I don't like either implementation, I would leave the code as it is
today.

-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/