| Message ID | 20170312142634.GA1917@roeck-us.net (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Sun, Mar 12, 2017 at 07:26:34AM -0700, Guenter Roeck wrote: > On Sun, Mar 12, 2017 at 02:15:22PM +0000, Russell King - ARM Linux wrote: > > On Sun, Mar 12, 2017 at 06:25:44AM -0700, Guenter Roeck wrote: > > > I found some reference suggesting that The IT8705F may respond on > > > both SIO addresses. Can you try the following patch ? > > > > Thanks for the patch - it'll take about a week or so for me to test, > > as I can only risk testing it when I'm physically at the machine. > > > > My pleasure. Sorry for the trouble. I attached a more comprehensive version > of the patch. > Hi Russell, did you by any chance have time to test the patch ? I would like to send the fix upstream, but I held it back in the hope to get feedback if it actually solves the problem. Thanks, Guenter > Thanks, > Guenter > > --- > From 5b3f92a9e0df49a111a96f43130d2cee370e4c7a Mon Sep 17 00:00:00 2001 > From: Guenter Roeck <linux@roeck-us.net> > Date: Sun, 12 Mar 2017 06:18:58 -0700 > Subject: [PATCH] hwmon: (it87) Avoid registering the same chip on both SIO > addresses > > IT8705F is known to respond on both SIO addresses. Other chips may have > the same behavior. Add checks to avoid registering the same chip twice. > > Fixes: e84bd9535e2b ("hwmon: (it87) Add support for second Super-IO chip") > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/hwmon/it87.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c > index 3e57a6120551..7053968fe0d3 100644 > --- a/drivers/hwmon/it87.c > +++ b/drivers/hwmon/it87.c > @@ -3197,7 +3197,7 @@ static int __init sm_it87_init(void) > { > int sioaddr[2] = { REG_2E, REG_4E }; > struct it87_sio_data sio_data; > - unsigned short isa_address; > + unsigned short isa_address[2]; > bool found = false; > int i, err; > > @@ -3207,15 +3207,29 @@ static int __init sm_it87_init(void) > > for (i = 0; i < ARRAY_SIZE(sioaddr); i++) { > memset(&sio_data, 0, sizeof(struct it87_sio_data)); > - isa_address = 0; > - err = it87_find(sioaddr[i], &isa_address, &sio_data); > - if (err || isa_address == 0) > + isa_address[i] = 0; > + err = it87_find(sioaddr[i], &isa_address[i], &sio_data); > + if (err || isa_address[i] == 0) > continue; > + /* > + * Don't register second chip if its ISA address matches > + * the first chip's ISA address. > + */ > + if (i && isa_address[i] == isa_address[0]) > + break; > > - err = it87_device_add(i, isa_address, &sio_data); > + err = it87_device_add(i, isa_address[i], &sio_data); > if (err) > goto exit_dev_unregister; > + > found = true; > + > + /* > + * IT8705F may respond on both SIO addresses. > + * Stop probing after finding one. > + */ > + if (sio_data.type == it87) > + break; > } > > if (!found) { > -- > 2.7.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 21, 2017 at 06:44:30AM -0700, Guenter Roeck wrote: > On Sun, Mar 12, 2017 at 07:26:34AM -0700, Guenter Roeck wrote: > > On Sun, Mar 12, 2017 at 02:15:22PM +0000, Russell King - ARM Linux wrote: > > > On Sun, Mar 12, 2017 at 06:25:44AM -0700, Guenter Roeck wrote: > > > > I found some reference suggesting that The IT8705F may respond on > > > > both SIO addresses. Can you try the following patch ? > > > > > > Thanks for the patch - it'll take about a week or so for me to test, > > > as I can only risk testing it when I'm physically at the machine. > > > > > > > My pleasure. Sorry for the trouble. I attached a more comprehensive version > > of the patch. > > > Hi Russell, > > did you by any chance have time to test the patch ? I would like to send > the fix upstream, but I held it back in the hope to get feedback if it > actually solves the problem. Sorry, I haven't, and given my current situation, I'm unlikely to be able to for quite some time (medical issues).
On Tue, Mar 21, 2017 at 02:08:12PM +0000, Russell King - ARM Linux wrote: > On Tue, Mar 21, 2017 at 06:44:30AM -0700, Guenter Roeck wrote: > > On Sun, Mar 12, 2017 at 07:26:34AM -0700, Guenter Roeck wrote: > > > On Sun, Mar 12, 2017 at 02:15:22PM +0000, Russell King - ARM Linux wrote: > > > > On Sun, Mar 12, 2017 at 06:25:44AM -0700, Guenter Roeck wrote: > > > > > I found some reference suggesting that The IT8705F may respond on > > > > > both SIO addresses. Can you try the following patch ? > > > > > > > > Thanks for the patch - it'll take about a week or so for me to test, > > > > as I can only risk testing it when I'm physically at the machine. > > > > > > > > > > My pleasure. Sorry for the trouble. I attached a more comprehensive version > > > of the patch. > > > > > Hi Russell, > > > > did you by any chance have time to test the patch ? I would like to send > > the fix upstream, but I held it back in the hope to get feedback if it > > actually solves the problem. > > Sorry, I haven't, and given my current situation, I'm unlikely to be > able to for quite some time (medical issues). > Sorry to hear that. Hope you'll get better soon. I'll submit the patch as-is upstream; at least it doesn't break anything. If it doesn't fix your problem, we'll have to look at it again at a later point. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guenter, On Tue, 21 Mar 2017 10:05:03 -0700, Guenter Roeck wrote: > I'll submit the patch as-is upstream; at least it doesn't break anything. > If it doesn't fix your problem, we'll have to look at it again at a later > point. Given that this patch fixes a regression in kernels v4.7 to v.4.10, shouldn't it go to stable@? As a side note, I think the second half of the patch is redundant, it only makes registration slightly faster on IT8705F, and could have bad side effects at least in theory. The first half seems sufficient to me...
On Sun, Apr 09, 2017 at 03:38:06PM +0200, Jean Delvare wrote: > Hi Guenter, > > On Tue, 21 Mar 2017 10:05:03 -0700, Guenter Roeck wrote: > > I'll submit the patch as-is upstream; at least it doesn't break anything. > > If it doesn't fix your problem, we'll have to look at it again at a later > > point. > > Given that this patch fixes a regression in kernels v4.7 to v.4.10, > shouldn't it go to stable@? > The patch has a Fixes: tag, so that should happen automatically. I'll have to check if it applies cleanly to earlier kernels and if necessary send backport(s) to Greg. > As a side note, I think the second half of the patch is redundant, it > only makes registration slightly faster on IT8705F, and could have bad > side effects at least in theory. The first half seems sufficient to > me... > It only affects systems with two Super-IO chips, and I wanted to play safe. The worst side effect I can imagine would be that a second chip in a system with IT8705 as first chip would not be accepted, which is not worse than before when only one chip was supported. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guenter, On Sun, 9 Apr 2017 08:24:07 -0700, Guenter Roeck wrote: > On Sun, Apr 09, 2017 at 03:38:06PM +0200, Jean Delvare wrote: > > On Tue, 21 Mar 2017 10:05:03 -0700, Guenter Roeck wrote: > > > I'll submit the patch as-is upstream; at least it doesn't break anything. > > > If it doesn't fix your problem, we'll have to look at it again at a later > > > point. > > > > Given that this patch fixes a regression in kernels v4.7 to v.4.10, > > shouldn't it go to stable@? > > The patch has a Fixes: tag, so that should happen automatically. > I'll have to check if it applies cleanly to earlier kernels and if > necessary send backport(s) to Greg. I took a look at stable branches v4.9 and v4.10 and I can't find this fix. Do you still plan to check if the fix applies and poke Greg about it? Or do you want me to do it? > > As a side note, I think the second half of the patch is redundant, it > > only makes registration slightly faster on IT8705F, and could have bad > > side effects at least in theory. The first half seems sufficient to > > me... > > It only affects systems with two Super-IO chips, and I wanted to play safe. The > worst side effect I can imagine would be that a second chip in a system with > IT8705 as first chip would not be accepted, which is not worse than before > when only one chip was supported. But isn't as good as doing the right thing, which would require less code. So I don't really follow your logic.
Hi Jean, On 04/25/2017 06:30 AM, Jean Delvare wrote: > Hi Guenter, > > On Sun, 9 Apr 2017 08:24:07 -0700, Guenter Roeck wrote: >> On Sun, Apr 09, 2017 at 03:38:06PM +0200, Jean Delvare wrote: >>> On Tue, 21 Mar 2017 10:05:03 -0700, Guenter Roeck wrote: >>>> I'll submit the patch as-is upstream; at least it doesn't break anything. >>>> If it doesn't fix your problem, we'll have to look at it again at a later >>>> point. >>> >>> Given that this patch fixes a regression in kernels v4.7 to v.4.10, >>> shouldn't it go to stable@? >> >> The patch has a Fixes: tag, so that should happen automatically. >> I'll have to check if it applies cleanly to earlier kernels and if >> necessary send backport(s) to Greg. > > I took a look at stable branches v4.9 and v4.10 and I can't find this > fix. Do you still plan to check if the fix applies and poke Greg about > it? Or do you want me to do it? > Severe lack of time on my side, sorry. Feel free to do it if you have some slack. >>> As a side note, I think the second half of the patch is redundant, it >>> only makes registration slightly faster on IT8705F, and could have bad >>> side effects at least in theory. The first half seems sufficient to >>> me... >> >> It only affects systems with two Super-IO chips, and I wanted to play safe. The >> worst side effect I can imagine would be that a second chip in a system with >> IT8705 as first chip would not be accepted, which is not worse than before >> when only one chip was supported. > > But isn't as good as doing the right thing, which would require less > code. So I don't really follow your logic. > Call it paranoia. I have not received feedback if the patch is actually working, and I am unable to test it myself. Feel free to send a patch to remove the redundant part if you think it should be dropped. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guenter, On Tue, 25 Apr 2017 06:49:27 -0700, Guenter Roeck wrote: > On 04/25/2017 06:30 AM, Jean Delvare wrote: > > I took a look at stable branches v4.9 and v4.10 and I can't find this > > fix. Do you still plan to check if the fix applies and poke Greg about > > it? Or do you want me to do it? > > Severe lack of time on my side, sorry. Feel free to do it if you have some slack. I finally found some time to look into this. The fix applies just fine to kernel branches 4.9 and 4.10, so I asked stable@ to pick it. > > (...) > > But isn't as good as doing the right thing, which would require less > > code. So I don't really follow your logic. > > Call it paranoia. I have not received feedback if the patch is actually working, > and I am unable to test it myself. Feel free to send a patch to remove the redundant > part if you think it should be dropped. Yes I think, and yes I will.
diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c index 3e57a6120551..7053968fe0d3 100644 --- a/drivers/hwmon/it87.c +++ b/drivers/hwmon/it87.c @@ -3197,7 +3197,7 @@ static int __init sm_it87_init(void) { int sioaddr[2] = { REG_2E, REG_4E }; struct it87_sio_data sio_data; - unsigned short isa_address; + unsigned short isa_address[2]; bool found = false; int i, err; @@ -3207,15 +3207,29 @@ static int __init sm_it87_init(void) for (i = 0; i < ARRAY_SIZE(sioaddr); i++) { memset(&sio_data, 0, sizeof(struct it87_sio_data)); - isa_address = 0; - err = it87_find(sioaddr[i], &isa_address, &sio_data); - if (err || isa_address == 0) + isa_address[i] = 0; + err = it87_find(sioaddr[i], &isa_address[i], &sio_data); + if (err || isa_address[i] == 0) continue; + /* + * Don't register second chip if its ISA address matches + * the first chip's ISA address. + */ + if (i && isa_address[i] == isa_address[0]) + break; - err = it87_device_add(i, isa_address, &sio_data); + err = it87_device_add(i, isa_address[i], &sio_data); if (err) goto exit_dev_unregister; + found = true; + + /* + * IT8705F may respond on both SIO addresses. + * Stop probing after finding one. + */ + if (sio_data.type == it87) + break; } if (!found) {