diff mbox

it87 causes VIA hardware to lockup

Message ID 20170312142634.GA1917@roeck-us.net
State Not Applicable
Headers show

Commit Message

Guenter Roeck March 12, 2017, 2:26 p.m. UTC
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.

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

Comments

Guenter Roeck March 21, 2017, 1:44 p.m. UTC | #1
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
Russell King - ARM Linux admin March 21, 2017, 2:08 p.m. UTC | #2
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).
Guenter Roeck March 21, 2017, 5:05 p.m. UTC | #3
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
Jean Delvare April 9, 2017, 1:38 p.m. UTC | #4
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...
Guenter Roeck April 9, 2017, 3:24 p.m. UTC | #5
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
Jean Delvare April 25, 2017, 1:30 p.m. UTC | #6
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.
Guenter Roeck April 25, 2017, 1:49 p.m. UTC | #7
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
Jean Delvare May 4, 2017, 8:49 a.m. UTC | #8
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 mbox

Patch

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