hwmon: f71882fg: properly acquire I/O regions while probing
in [Kernel]
|
Prev: [PATCH] staging: crystalhd: printk register accesses to debug for big-endian systems
Next: [patch 3/5] mincore: pass ranges as start,end address pairs
From: Giel van Schijndel on 23 Mar 2010 10:20 On Tue, Mar 23, 2010 at 03:12:15PM +0100, Giel van Schijndel wrote: > Acquire the I/O region for the Super I/O chip while we're working on it. > > Further alter the way multiple Super I/O addresses are probed for chips > such that errors in the probing process are passed on from the module > initialisation function. > > Some code cleanup: properly using, previously defined, functions rather > than duplicating their code. Oh yes, this in preparation for adding watchdog support for the Fintek F71808E (very similar to the F71889) to this driver. I'll be sending in a patch for adding sensor hardware monitor support for the F71808E first (i.e. before the addition of watchdog support). PS I'm using the watchdog API as described in Documentation/watchdog/watchdog-api.txt. -- Met vriendelijke groet, With kind regards, Giel van Schijndel
From: Giel van Schijndel on 23 Mar 2010 19:10 On Tue, Mar 23, 2010 at 03:12:15PM +0100, Giel van Schijndel wrote: > Acquire the I/O region for the Super I/O chip while we're working on it. > > Further alter the way multiple Super I/O addresses are probed for chips > such that errors in the probing process are passed on from the module > initialisation function. > > Some code cleanup: properly using, previously defined, functions rather > than duplicating their code. > --- > drivers/hwmon/f71882fg.c | 38 +++++++++++++++++++++++--------------- > 1 files changed, 23 insertions(+), 15 deletions(-) Sorry, forgot to sign off the commit message itself. So just to be sure it's understood to be signed off on by myself: Signed-off-by: Giel van Schijndel <me(a)mortis.eu> -- Met vriendelijke groet, With kind regards, Giel van Schijndel
From: Giel van Schijndel on 24 Mar 2010 04:50 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
From: Jean Delvare on 24 Mar 2010 05:30 On Tue, 23 Mar 2010 15:12:15 +0100, Giel van Schijndel wrote: > Acquire the I/O region for the Super I/O chip while we're working on it. > > Further alter the way multiple Super I/O addresses are probed for chips > such that errors in the probing process are passed on from the module > initialisation function. > > Some code cleanup: properly using, previously defined, functions rather > than duplicating their code. > (...) On top of Hans' comments, to which I agree: > static u16 f71882fg_read_temp(struct f71882fg_data *data, int nr) > @@ -2184,6 +2178,13 @@ static int __init f71882fg_find(int sioaddr, unsigned short *address, > int err = -ENODEV; > u16 devid; > > + /* Don't step on other driver's I/O space by accident */ > + if (!request_region(sioaddr, 2, DRVNAME)) { > + printk(KERN_ERR DRVNAME ": I/O address 0x%04x already in use\n", > + (int)sioaddr); > + return -EIO; This is the wrong error code. This isn't an I/O error. You want -EBUSY. > + } > + > superio_enter(sioaddr); > > devid = superio_inw(sioaddr, SIO_REG_MANID); -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Giel van Schijndel on 24 Mar 2010 05:40
On Wed, Mar 24, 2010 at 10:29:42AM +0100, Jean Delvare wrote: > On Tue, 23 Mar 2010 15:12:15 +0100, Giel van Schijndel wrote: >> static u16 f71882fg_read_temp(struct f71882fg_data *data, int nr) >> @@ -2184,6 +2178,13 @@ static int __init f71882fg_find(int sioaddr, unsigned short *address, >> int err = -ENODEV; >> u16 devid; >> >> + /* Don't step on other driver's I/O space by accident */ >> + if (!request_region(sioaddr, 2, DRVNAME)) { >> + printk(KERN_ERR DRVNAME ": I/O address 0x%04x already in use\n", >> + (int)sioaddr); >> + return -EIO; > > This is the wrong error code. This isn't an I/O error. You want -EBUSY. Ack. New patch follows this line. ======================================================================== hwmon: f71882fg: acquire I/O regions while we're working with them Acquire the I/O region for the Super I/O chip while we're working on it. Signed-off-by: Giel van Schijndel <me(a)mortis.eu> --- drivers/hwmon/f71882fg.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-) diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c index ca34e5c..537841e 100644 --- a/drivers/hwmon/f71882fg.c +++ b/drivers/hwmon/f71882fg.c @@ -2178,6 +2178,13 @@ static int __init f71882fg_find(int sioaddr, unsigned short *address, int err = -ENODEV; u16 devid; + /* Don't step on other drivers' I/O space by accident */ + if (!request_region(sioaddr, 2, DRVNAME)) { + printk(KERN_ERR DRVNAME ": I/O address 0x%04x already in use\n", + (int)sioaddr); + return -EBUSY; + } + superio_enter(sioaddr); devid = superio_inw(sioaddr, SIO_REG_MANID); @@ -2232,6 +2239,7 @@ static int __init f71882fg_find(int sioaddr, unsigned short *address, (int)superio_inb(sioaddr, SIO_REG_DEVREV)); exit: superio_exit(sioaddr); + release_region(sioaddr, 2); return err; } -- 1.6.4.4 -- Met vriendelijke groet, With kind regards, Giel van Schijndel |