[lm-sensors] small regression: hwmon: (applesmc) Check key count before proceeding - 5f4513864304672

[lm-sensors] small regression: hwmon: (applesmc) Check key count before proceeding - 5f4513864304672

[Michele Baldessari]

Hi Henrik & Guenther,

via commit 5f4513864304672e6ea9eac60583eeac32e679f2 the following
code was added:
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 62c2e32..98814d1 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -525,16 +525,25 @@ static int applesmc_init_smcreg_try(void)
 {
        struct applesmc_registers *s = &smcreg;
        bool left_light_sensor, right_light_sensor;
+       unsigned int count;
        u8 tmp[1];
        int ret;
 
        if (s->init_complete)
                return 0;
 
-       ret = read_register_count(&s->key_count);
+       ret = read_register_count(&count);
        if (ret)
                return ret;
 
+       if (s->cache && s->key_count != count) {
+               pr_warn("key count changed from %d to %d\n",
+                       s->key_count, count);
+               kfree(s->cache);
+               s->cache = NULL;
+       }
+       s->key_count = count;
+
        if (!s->cache)
                s->cache = kcalloc(s->key_count, sizeof(*s->cache),
GFP_KERNEL); if (!s->cache)

The issue Chris has seen in Fedora on one MacBookPro4,1
(https://bugzilla.redhat.com/show_bug.cgi?id\x1033414) is that this
machine returns a huge number from read_register_count() so now we will
try to allocate an insane amount of memory and we will barf:
[    8.603053] applesmc: key count changed from 261 to 1392508929

Dmidecode for this box is here:
https://bugzilla.redhat.com/attachment.cgi?id‚8118

Do we need to special case this specific machine/smc version or should
we limit the kcalloc() call to a somewhat sane limit? Other thoughts?

Thanks,
Michele

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

small regression: hwmon: (applesmc) Check key count before proceeding - 5f4513864304672e6ea9eac60583eeac32e679f2

[Michele Baldessari]

Hi Henrik & Guenther,

via commit 5f4513864304672e6ea9eac60583eeac32e679f2 the following
code was added:
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 62c2e32..98814d1 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -525,16 +525,25 @@ static int applesmc_init_smcreg_try(void)
 {
        struct applesmc_registers *s = &smcreg;
        bool left_light_sensor, right_light_sensor;
+       unsigned int count;
        u8 tmp[1];
        int ret;
 
        if (s->init_complete)
                return 0;
 
-       ret = read_register_count(&s->key_count);
+       ret = read_register_count(&count);
        if (ret)
                return ret;
 
+       if (s->cache && s->key_count != count) {
+               pr_warn("key count changed from %d to %d\n",
+                       s->key_count, count);
+               kfree(s->cache);
+               s->cache = NULL;
+       }
+       s->key_count = count;
+
        if (!s->cache)
                s->cache = kcalloc(s->key_count, sizeof(*s->cache),
GFP_KERNEL); if (!s->cache)

The issue Chris has seen in Fedora on one MacBookPro4,1
(https://bugzilla.redhat.com/show_bug.cgi?id=1033414) is that this
machine returns a huge number from read_register_count() so now we will
try to allocate an insane amount of memory and we will barf:
[    8.603053] applesmc: key count changed from 261 to 1392508929

Dmidecode for this box is here:
https://bugzilla.redhat.com/attachment.cgi?id=828118

Do we need to special case this specific machine/smc version or should
we limit the kcalloc() call to a somewhat sane limit? Other thoughts?

Thanks,
Michele

Re: [lm-sensors] small regression: hwmon: (applesmc) Check key count before proceeding - 5f451386430

[Henrik Rydberg]

Hi Michele,

> The issue Chris has seen in Fedora on one MacBookPro4,1
> (https://bugzilla.redhat.com/show_bug.cgi?id\x1033414) is that this
> machine returns a huge number from read_register_count() so now we will
> try to allocate an insane amount of memory and we will barf:
> [    8.603053] applesmc: key count changed from 261 to 1392508929

I was under the impression that this machine was tested before, and that

commit 25f2bd7f5add608c1d1405938f39c96927b275ca
Author: Henrik Rydberg <rydberg@euromail.se>
Date:   Wed Oct 2 19:15:03 2013 +0200

    hwmon: (applesmc) Always read until end of data

resolved this problem? But if the kernel under test is 3.11.8, both patches are
already present... Chris, could you please sched some light on this before
moving on?

Thanks,
Henrik


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: small regression: hwmon: (applesmc) Check key count before proceeding - 5f4513864304672e6ea9eac60583eeac32e679f2

[Henrik Rydberg]

Hi Michele,

> The issue Chris has seen in Fedora on one MacBookPro4,1
> (https://bugzilla.redhat.com/show_bug.cgi?id=1033414) is that this
> machine returns a huge number from read_register_count() so now we will
> try to allocate an insane amount of memory and we will barf:
> [    8.603053] applesmc: key count changed from 261 to 1392508929

I was under the impression that this machine was tested before, and that

commit 25f2bd7f5add608c1d1405938f39c96927b275ca
Author: Henrik Rydberg <rydberg@euromail.se>
Date:   Wed Oct 2 19:15:03 2013 +0200

    hwmon: (applesmc) Always read until end of data

resolved this problem? But if the kernel under test is 3.11.8, both patches are
already present... Chris, could you please sched some light on this before
moving on?

Thanks,
Henrik

Re: [lm-sensors] small regression: hwmon: (applesmc) Check key count before proceeding - 5f451386430

[Chris Murphy]

On Nov 24, 2013, at 2:44 AM, Henrik Rydberg <rydberg@euromail.se> wrote:

> Hi Michele,
> 
>> The issue Chris has seen in Fedora on one MacBookPro4,1
>> (https://bugzilla.redhat.com/show_bug.cgi?id\x1033414) is that this
>> machine returns a huge number from read_register_count() so now we will
>> try to allocate an insane amount of memory and we will barf:
>> [    8.603053] applesmc: key count changed from 261 to 1392508929
> 
> I was under the impression that this machine was tested before, and that
> 
> commit 25f2bd7f5add608c1d1405938f39c96927b275ca
> Author: Henrik Rydberg <rydberg@euromail.se>
> Date:   Wed Oct 2 19:15:03 2013 +0200
> 
>    hwmon: (applesmc) Always read until end of data
> 
> resolved this problem? But if the kernel under test is 3.11.8, both patches are
> already present... Chris, could you please sched some light on this before
> moving on?

Well, it seems to be a another one off event. It's the same hardware as before, and it was booting from a USB stick containing Fedora 20 final test candidate 2 which uses kernel 3.11.8. An immediate reboot did not reproduce the problem, nor multiple subsequent reboots. I think I previously mentioned a preponderance of these events happen when booting from USB sticks.

It would be nice to have an identical model for this testing. I suspect most users wouldn't go to the trouble to report the occasional, seemingly one off, events like this. So unfortunately it's  uncertain if the hardware I have has a unique problem, or if it's a model specific behavior.


Chris Murphy
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: small regression: hwmon: (applesmc) Check key count before proceeding - 5f4513864304672e6ea9eac60583eeac32e679f2

[Chris Murphy]

On Nov 24, 2013, at 2:44 AM, Henrik Rydberg <rydberg@euromail.se> wrote:

> Hi Michele,
> 
>> The issue Chris has seen in Fedora on one MacBookPro4,1
>> (https://bugzilla.redhat.com/show_bug.cgi?id=1033414) is that this
>> machine returns a huge number from read_register_count() so now we will
>> try to allocate an insane amount of memory and we will barf:
>> [    8.603053] applesmc: key count changed from 261 to 1392508929
> 
> I was under the impression that this machine was tested before, and that
> 
> commit 25f2bd7f5add608c1d1405938f39c96927b275ca
> Author: Henrik Rydberg <rydberg@euromail.se>
> Date:   Wed Oct 2 19:15:03 2013 +0200
> 
>    hwmon: (applesmc) Always read until end of data
> 
> resolved this problem? But if the kernel under test is 3.11.8, both patches are
> already present... Chris, could you please sched some light on this before
> moving on?

Well, it seems to be a another one off event. It's the same hardware as before, and it was booting from a USB stick containing Fedora 20 final test candidate 2 which uses kernel 3.11.8. An immediate reboot did not reproduce the problem, nor multiple subsequent reboots. I think I previously mentioned a preponderance of these events happen when booting from USB sticks.

It would be nice to have an identical model for this testing. I suspect most users wouldn't go to the trouble to report the occasional, seemingly one off, events like this. So unfortunately it's  uncertain if the hardware I have has a unique problem, or if it's a model specific behavior.


Chris Murphy

Re: [lm-sensors] small regression: hwmon: (applesmc) Check key count before proceeding - 5f451386430

[Henrik Rydberg]

Hi Chris,

> Well, it seems to be a another one off event. It's the same hardware as
before, and it was booting from a USB stick containing Fedora 20 final test
candidate 2 which uses kernel 3.11.8. An immediate reboot did not reproduce the
problem, nor multiple subsequent reboots. I think I previously mentioned a
preponderance of these events happen when booting from USB sticks.

Ok, thanks, that makes sense. So at least one problem is still there, but
possibly very difficult to hit.

> It would be nice to have an identical model for this testing. I suspect most users wouldn't go to the trouble to report the occasional, seemingly one off,
events like this. So unfortunately it's  uncertain if the hardware I have has a
unique problem, or if it's a model specific behavior.

I will keep this in mind next time I see this particular model. It is also
possible that the SMC needs to be reset after having testing various more or
less successful patches. Given the tiny cross-section, it looks like this one
can rest for now.

Thanks,
Henrik


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: small regression: hwmon: (applesmc) Check key count before proceeding - 5f4513864304672e6ea9eac60583eeac32e679f2

[Henrik Rydberg]

Hi Chris,

> Well, it seems to be a another one off event. It's the same hardware as
before, and it was booting from a USB stick containing Fedora 20 final test
candidate 2 which uses kernel 3.11.8. An immediate reboot did not reproduce the
problem, nor multiple subsequent reboots. I think I previously mentioned a
preponderance of these events happen when booting from USB sticks.

Ok, thanks, that makes sense. So at least one problem is still there, but
possibly very difficult to hit.

> It would be nice to have an identical model for this testing. I suspect most users wouldn't go to the trouble to report the occasional, seemingly one off,
events like this. So unfortunately it's  uncertain if the hardware I have has a
unique problem, or if it's a model specific behavior.

I will keep this in mind next time I see this particular model. It is also
possible that the SMC needs to be reset after having testing various more or
less successful patches. Given the tiny cross-section, it looks like this one
can rest for now.

Thanks,
Henrik

Re: [lm-sensors] small regression: hwmon: (applesmc) Check key count before proceeding - 5f451386430

[Chris Murphy]

On Nov 24, 2013, at 9:57 AM, Henrik Rydberg <rydberg@euromail.se> wrote:

> Hi Chris,
> 
>> Well, it seems to be a another one off event. It's the same hardware as
> before, and it was booting from a USB stick containing Fedora 20 final test
> candidate 2 which uses kernel 3.11.8. An immediate reboot did not reproduce the
> problem, nor multiple subsequent reboots. I think I previously mentioned a
> preponderance of these events happen when booting from USB sticks.
> 
> Ok, thanks, that makes sense. So at least one problem is still there, but
> possibly very difficult to hit.
> 
>> It would be nice to have an identical model for this testing. I suspect most users wouldn't go to the trouble to report the occasional, seemingly one off,
> events like this. So unfortunately it's  uncertain if the hardware I have has a
> unique problem, or if it's a model specific behavior.
> 
> I will keep this in mind next time I see this particular model. It is also
> possible that the SMC needs to be reset after having testing various more or
> less successful patches. Given the tiny cross-section, it looks like this one
> can rest for now.

This SMC has been reset within that past week, after a rest without its battery or power for about 2 weeks. So unless the SMC is particularly prone to corruption, I think the cause is elsewhere.

OS X always says this battery needs servicing, but doesn't elaborate. The manifestation is merely limited battery life, about 1 hour. All testing is always done with power connected.


Chris Murphy
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: small regression: hwmon: (applesmc) Check key count before proceeding - 5f4513864304672e6ea9eac60583eeac32e679f2

[Chris Murphy]

On Nov 24, 2013, at 9:57 AM, Henrik Rydberg <rydberg@euromail.se> wrote:

> Hi Chris,
> 
>> Well, it seems to be a another one off event. It's the same hardware as
> before, and it was booting from a USB stick containing Fedora 20 final test
> candidate 2 which uses kernel 3.11.8. An immediate reboot did not reproduce the
> problem, nor multiple subsequent reboots. I think I previously mentioned a
> preponderance of these events happen when booting from USB sticks.
> 
> Ok, thanks, that makes sense. So at least one problem is still there, but
> possibly very difficult to hit.
> 
>> It would be nice to have an identical model for this testing. I suspect most users wouldn't go to the trouble to report the occasional, seemingly one off,
> events like this. So unfortunately it's  uncertain if the hardware I have has a
> unique problem, or if it's a model specific behavior.
> 
> I will keep this in mind next time I see this particular model. It is also
> possible that the SMC needs to be reset after having testing various more or
> less successful patches. Given the tiny cross-section, it looks like this one
> can rest for now.

This SMC has been reset within that past week, after a rest without its battery or power for about 2 weeks. So unless the SMC is particularly prone to corruption, I think the cause is elsewhere.

OS X always says this battery needs servicing, but doesn't elaborate. The manifestation is merely limited battery life, about 1 hour. All testing is always done with power connected.


Chris Murphy