[lm-sensors] new abituguru driver in mm kernel

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

Hi,

I am interested in using this driver but don't know how. I am running
gentoo-sources-2.6.17-r1 (2.6.17.3) patched with suspend2 patch. I don't
want to use mm sources for now. I have lm_sensors-2.10.0 installed. From
this mailing list, I gather that there are three patches for the driver and
one userspace patch for the lm_sensors package itself.

In nutshell, I am confused about what patches I need to apply to the kernel
and how do I get the latest lm_sensors package which works with abit uguru
driver. Can Hans or someone who has used this driver, please post a step by
step howto to get the uguru working with my setup, I will really appreciate
it.

Thanks,
Sunil
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060704/95684bac/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Stephen Cormier]

On Tuesday 04 July 2006 15:46, Sunil Kumar wrote:
> Hi,
>
> I am interested in using this driver but don't know how. I am running
> gentoo-sources-2.6.17-r1 (2.6.17.3) patched with suspend2 patch. I don't
> want to use mm sources for now. I have lm_sensors-2.10.0 installed. From
> this mailing list, I gather that there are three patches for the driver and
> one userspace patch for the lm_sensors package itself.
>
> In nutshell, I am confused about what patches I need to apply to the kernel
> and how do I get the latest lm_sensors package which works with abit uguru
> driver. Can Hans or someone who has used this driver, please post a step by
> step howto to get the uguru working with my setup, I will really appreciate
> it.
>
> Thanks,
> Sunil

It is fairly basic you patch the kernel first then the lm-sensors 2.10.0 
source build both and install I explained it in a little more detail in this 
thread on the Abit forum starting on page 5 I'm HappyTux on there.

http://forum.abit-usa.com/showthread.php?t€051

Stephen

-- 
GPG Pubic Key: http://users.eastlink.ca/~stephencormier/publickey.asc
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060704/c2d460dc/attachment.bin 

[lm-sensors] new abituguru driver in mm kernel

[Hans de Goede]

Sunil Kumar wrote:
> I actually found that thread after posting here. I am devsk there. So, 
> is there going to be standalone tar release of version 1.1.10? I have 
> AN8-SLI and 1.1.8 is working fine with patch to lm_sensors-2.10.0 
> package. I am also wondering if I really need to go to 1.1.10?
> 

I'm not planning on doing a stand alone release of 1.1.10 as you say
1.1.8 works fine for you and so it does for most others. If the need
arises I may do a stand alone release. But the whole idea is to have
this driver in the kernel and for the kernel to be _the_ place where
development happens. So things are actually going as planned, with 1.1.8
being the last standalone release.

Regards,

Hans

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

Hans,

A related question: gkrellm2 (2.2.9) refuses to see my sensors as reported
by abit driver, but its able to see bogus sensors reported by w83627hf if I
modprobe it.  sensors and ksensors, both see the values correctly.

Why would gkrellm not see them? Is there a configuration setting that
gkrellm reads which works for w83627hf but not for abituguru.

Thanks,
Sunil


On 7/4/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
>
> Sunil Kumar wrote:
> > I actually found that thread after posting here. I am devsk there. So,
> > is there going to be standalone tar release of version 1.1.10? I have
> > AN8-SLI and 1.1.8 is working fine with patch to lm_sensors-2.10.0
> > package. I am also wondering if I really need to go to 1.1.10?
> >
>
> I'm not planning on doing a stand alone release of 1.1.10 as you say
> 1.1.8 works fine for you and so it does for most others. If the need
> arises I may do a stand alone release. But the whole idea is to have
> this driver in the kernel and for the kernel to be _the_ place where
> development happens. So things are actually going as planned, with 1.1.8
> being the last standalone release.
>
> Regards,
>
> Hans
>
>
>
> _______________________________________________
> lm-sensors mailing list
> lm-sensors at lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060705/d206aed9/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Jean Delvare]

Hi Sunil,

> A related question: gkrellm2 (2.2.9) refuses to see my sensors as reported
> by abit driver, but its able to see bogus sensors reported by w83627hf if I
> modprobe it.  sensors and ksensors, both see the values correctly.
> 
> Why would gkrellm not see them? Is there a configuration setting that
> gkrellm reads which works for w83627hf but not for abituguru.

gkrellm accesses /sys directly instead of using libsensors. The
consequence of this unfortunate choice is that the results you get with
gkrellm can differ from what all other hardware monitoring frontends
report, and when support for new chips is added gkrellm doesn't benefit
from the libsensors update and must implement support on its own.

So you better complain to the gkrellm folks, there's nothing we can do
about it.

-- 
Jean Delvare

[lm-sensors] new abituguru driver in mm kernel

[Hans de Goede]

Sunil Kumar wrote:
> Hans,
> 
> A related question: gkrellm2 (2.2.9) refuses to see my sensors as reported
> by abit driver, but its able to see bogus sensors reported by w83627hf if I
> modprobe it.  sensors and ksensors, both see the values correctly.
> 
> Why would gkrellm not see them? Is there a configuration setting that
> gkrellm reads which works for w83627hf but not for abituguru.
> 

Known problem with gkrellm and newer drivers for ISA chips which
correctly report them self as platform drivers rather then fake I2C drivers.

I've contacted the gkrellm author on this and he is willing to take a
patch both for this issue and for the more general issue of not using
libsensors. Know all that I (any volunteers?) have todo is writ
libsensors support for gkrellm.

Regards,

Hans

[lm-sensors] new abituguru driver in mm kernel

[Hans de Goede]

Jean Delvare wrote:
> Hi Sunil,
> 
>> A related question: gkrellm2 (2.2.9) refuses to see my sensors as reported
>> by abit driver, but its able to see bogus sensors reported by w83627hf if I
>> modprobe it.  sensors and ksensors, both see the values correctly.
>>
>> Why would gkrellm not see them? Is there a configuration setting that
>> gkrellm reads which works for w83627hf but not for abituguru.
> 
> gkrellm accesses /sys directly instead of using libsensors. The
> consequence of this unfortunate choice is that the results you get with
> gkrellm can differ from what all other hardware monitoring frontends
> report, and when support for new chips is added gkrellm doesn't benefit
> from the libsensors update and must implement support on its own.
> 
> So you better complain to the gkrellm folks, there's nothing we can do
> about it.
> 

Jean,

I have complained (politely) and he is willing to take a patch to use
libsensors. Writing such a patch is on my todo, just like an abituguru2
driver and ... and .... and don't forget XXXX and YYYY etc. You get he
picture, right?

Regards,

Hans

[lm-sensors] new abituguru driver in mm kernel

[Hans de Goede]

Sunil Kumar wrote:
> Hans,
> 
> A related question: gkrellm2 (2.2.9) refuses to see my sensors as reported
> by abit driver, but its able to see bogus sensors reported by w83627hf if I
> modprobe it.  sensors and ksensors, both see the values correctly.
> 
> Why would gkrellm not see them? Is there a configuration setting that
> gkrellm reads which works for w83627hf but not for abituguru.
> 

I became inspired by your mail and have taken a look. I haven't done (I
haven't even started) a full libsensors implementation yet, but if you
compile gkrellm with the attached patch it should work fine with the
uguru and other new drivers which properly represent themselves as
platform devices.

Regards,

Hans


-------------- next part --------------
A non-text attachment was scrubbed...
Name: gkrellm-2.2.9-hwmon.patch
Type: text/x-patch
Size: 1444 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060705/5db6049d/attachment.bin 

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

Thanks. I will give it a try and post when I am home.

On 7/5/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
>
>
>
> Sunil Kumar wrote:
> > Hans,
> >
> > A related question: gkrellm2 (2.2.9) refuses to see my sensors as
> reported
> > by abit driver, but its able to see bogus sensors reported by w83627hf
> if I
> > modprobe it.  sensors and ksensors, both see the values correctly.
> >
> > Why would gkrellm not see them? Is there a configuration setting that
> > gkrellm reads which works for w83627hf but not for abituguru.
> >
>
> I became inspired by your mail and have taken a look. I haven't done (I
> haven't even started) a full libsensors implementation yet, but if you
> compile gkrellm with the attached patch it should work fine with the
> uguru and other new drivers which properly represent themselves as
> platform devices.
>
> Regards,
>
> Hans
>
>
>
>
> _______________________________________________
> lm-sensors mailing list
> lm-sensors at lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060705/04d35a29/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

great!! it works like a charm. Thanks a lot. That saves me the headache of
having ksensors around...:-)

temps are reported as temp1, temp2, temp3 and fans as fan1, fan2 etc. I
think this will go away once gkrellm moves to libsensors.

On 7/5/06, Sunil Kumar <devsku at gmail.com> wrote:
>
> Thanks. I will give it a try and post when I am home.
>
> On 7/5/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
>
> >
>
> Sunil Kumar wrote:
> > Hans,
> >
> > A related question: gkrellm2 ( 2.2.9) refuses to see my sensors as
> reported
> > by abit driver, but its able to see bogus sensors reported by w83627hf
> if I
> > modprobe it.  sensors and ksensors, both see the values correctly.
> >
> > Why would gkrellm not see them? Is there a configuration setting that
> > gkrellm reads which works for w83627hf but not for abituguru.
> >
>
> I became inspired by your mail and have taken a look. I haven't done (I
> haven't even started) a full libsensors implementation yet, but if you
> compile gkrellm with the attached patch it should work fine with the
> uguru and other new drivers which properly represent themselves as
> platform devices.
>
> Regards,
>
> Hans
>
>
>
>
> _______________________________________________
>
> lm-sensors mailing list
> lm-sensors at lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060705/18ecebf6/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Stephen Cormier]

On Wednesday 05 July 2006 22:09, Sunil Kumar wrote:
> great!! it works like a charm. Thanks a lot. That saves me the headache of
> having ksensors around...:-)
>
> temps are reported as temp1, temp2, temp3 and fans as fan1, fan2 etc. I
> think this will go away once gkrellm moves to libsensors.

In the area where you check mark the sensor to be shown double click on the 
label eg. temp1 then you will be able to put in the name you want displayed 
in gkrellm, works great here as well. 

-- 
GPG Pubic Key: http://users.eastlink.ca/~stephencormier/publickey.asc
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060705/905a972a/attachment.bin 

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

Hi Hans,

I am seeing these messages in the dmesg:

abituguru: timeout exceeded waiting for ready state
abituguru: timeout exceeded waiting for ready state
abituguru: timeout exceeded waiting for read state (bank: 34, sensor: 15)
abituguru: CMD reg does not hold 0xAC after ready command
abituguru: timeout exceeded waiting for read state (bank: 38, sensor: 2)
abituguru: timeout exceeded waiting for ready state

I am not sure if they are related to a freeze I saw today. My machine wasn't
being used and when I came back, it was frozen. Can you please tell me what
are those messages about?

Thanks a lot,
Sunil
PS: This is with 1.1.8 with 2.6.17.3 kernel.


On 7/4/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
>
> Sunil Kumar wrote:
> > I actually found that thread after posting here. I am devsk there. So,
> > is there going to be standalone tar release of version 1.1.10? I have
> > AN8-SLI and 1.1.8 is working fine with patch to lm_sensors-2.10.0
> > package. I am also wondering if I really need to go to 1.1.10?
> >
>
> I'm not planning on doing a stand alone release of 1.1.10 as you say
> 1.1.8 works fine for you and so it does for most others. If the need
> arises I may do a stand alone release. But the whole idea is to have
> this driver in the kernel and for the kernel to be _the_ place where
> development happens. So things are actually going as planned, with 1.1.8
> being the last standalone release.
>
> Regards,
>
> Hans
>
>
>
> _______________________________________________
> lm-sensors mailing list
> lm-sensors at lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060708/6a661f2e/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Hans de Goede]

Sunil Kumar wrote:
> Hi Hans,
> 
> I am seeing these messages in the dmesg:
> 
> abituguru: timeout exceeded waiting for ready state
> abituguru: timeout exceeded waiting for ready state
> abituguru: timeout exceeded waiting for read state (bank: 34, sensor: 15)
> abituguru: CMD reg does not hold 0xAC after ready command
> abituguru: timeout exceeded waiting for read state (bank: 38, sensor: 2)
> abituguru: timeout exceeded waiting for ready state
> 

Hmm, were these messages close together in time, or now and then a message?

> I am not sure if they are related to a freeze I saw today. My machine
> wasn't
> being used and when I came back, it was frozen.

I seriously doubt the freeze and these messages are related. The uGuru
only uses 2 io ports todo all communication with the outside. So reading
the ports doesn't always get you the same register there is a sortoff
network protocol which you speak over the ports. These messages mean
that the uGuru failed to response within a reasonable time. Unfortunate
on some uGuru's this is quite normal. It seems the older the
Motherboard, the more often this happens (iow the buggier the uGuru
firmware).

Regards,

Hans

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

they always occurr in a bunch but they are scattered all over time,
sometimes disappearing for many hours and then returning.

On 7/9/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
>
>
>
> Sunil Kumar wrote:
> > Hi Hans,
> >
> > I am seeing these messages in the dmesg:
> >
> > abituguru: timeout exceeded waiting for ready state
> > abituguru: timeout exceeded waiting for ready state
> > abituguru: timeout exceeded waiting for read state (bank: 34, sensor:
> 15)
> > abituguru: CMD reg does not hold 0xAC after ready command
> > abituguru: timeout exceeded waiting for read state (bank: 38, sensor: 2)
> > abituguru: timeout exceeded waiting for ready state
> >
>
> Hmm, were these messages close together in time, or now and then a
> message?
>
> > I am not sure if they are related to a freeze I saw today. My machine
> > wasn't
> > being used and when I came back, it was frozen.
>
> I seriously doubt the freeze and these messages are related. The uGuru
> only uses 2 io ports todo all communication with the outside. So reading
> the ports doesn't always get you the same register there is a sortoff
> network protocol which you speak over the ports. These messages mean
> that the uGuru failed to response within a reasonable time. Unfortunate
> on some uGuru's this is quite normal. It seems the older the
> Motherboard, the more often this happens (iow the buggier the uGuru
> firmware).
>
> Regards,
>
> Hans
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060709/e2bd50d7/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

Ok, now another problem. Some of the times the module loads fine during
bootup but sensors reports that

$ sensors
Can't access procfs/sysfs file
Unable to find i2c bus information;
For 2.6 kernels, make sure you have mounted sysfs and libsensors
was compiled with sysfs support!
For older kernels, make sure you have done 'modprobe i2c-proc'!

If I rmmod abituguru and modprobe it again, it works. I will rebuild the
module and see if it helps.

The fallout of this is that gkrellm forgets about my sensors permanently. I
have to re-enable them and re-assign the labels etc....:(

Thanks,
-Sunil

On 7/9/06, Sunil Kumar < devsku at gmail.com> wrote:
>
> they always occurr in a bunch but they are scattered all over time,
> sometimes disappearing for many hours and then returning.
>
>
> On 7/9/06, Hans de Goede < j.w.r.degoede at hhs.nl> wrote:
> >
> >
> >
> > Sunil Kumar wrote:
> > > Hi Hans,
> > >
> > > I am seeing these messages in the dmesg:
> > >
> > > abituguru: timeout exceeded waiting for ready state
> > > abituguru: timeout exceeded waiting for ready state
> > > abituguru: timeout exceeded waiting for read state (bank: 34, sensor:
> > 15)
> > > abituguru: CMD reg does not hold 0xAC after ready command
> > > abituguru: timeout exceeded waiting for read state (bank: 38, sensor:
> > 2)
> > > abituguru: timeout exceeded waiting for ready state
> > >
> >
> > Hmm, were these messages close together in time, or now and then a
> > message?
> >
> > > I am not sure if they are related to a freeze I saw today. My machine
> > > wasn't
> > > being used and when I came back, it was frozen.
> >
> > I seriously doubt the freeze and these messages are related. The uGuru
> > only uses 2 io ports todo all communication with the outside. So reading
> >
> > the ports doesn't always get you the same register there is a sortoff
> > network protocol which you speak over the ports. These messages mean
> > that the uGuru failed to response within a reasonable time. Unfortunate
> > on some uGuru's this is quite normal. It seems the older the
> > Motherboard, the more often this happens (iow the buggier the uGuru
> > firmware).
> >
> > Regards,
> >
> > Hans
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060709/b8fd318d/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Hans de Goede]

Sunil Kumar wrote:
> Ok, now another problem. Some of the times the module loads fine during
> bootup but sensors reports that
> 
> $ sensors
> Can't access procfs/sysfs file
> Unable to find i2c bus information;
> For 2.6 kernels, make sure you have mounted sysfs and libsensors
> was compiled with sysfs support!
> For older kernels, make sure you have done 'modprobe i2c-proc'!
> 
> If I rmmod abituguru and modprobe it again, it works. I will rebuild the
> module and see if it helps.
> 
> The fallout of this is that gkrellm forgets about my sensors permanently. I
> have to re-enable them and re-assign the labels etc....:(
> 

Yes, this is because the module loads, but the driver part of it doesn't
(new driver model, where drivers can be registered without actually
handling any devices, so they can be bound to hotplug devices lateR).


The driver part probably fails to load because it happens to run into
those errors you report during the initialisation fase. Strange though
as during init it retries all things 3 times, which goes to show that
indeed your uGuru is unwilling to talk for a while, as seen before with
early uGurus, what motherboard do you have. Lemme gues one out of this
list? : AI7, KV8-MAX3, AN7


Regards,

Hans




> Thanks,
> -Sunil
> 
> On 7/9/06, Sunil Kumar < devsku at gmail.com> wrote:
>>
>> they always occurr in a bunch but they are scattered all over time,
>> sometimes disappearing for many hours and then returning.
>>
>>
>> On 7/9/06, Hans de Goede < j.w.r.degoede at hhs.nl> wrote:
>> >
>> >
>> >
>> > Sunil Kumar wrote:
>> > > Hi Hans,
>> > >
>> > > I am seeing these messages in the dmesg:
>> > >
>> > > abituguru: timeout exceeded waiting for ready state
>> > > abituguru: timeout exceeded waiting for ready state
>> > > abituguru: timeout exceeded waiting for read state (bank: 34, sensor:
>> > 15)
>> > > abituguru: CMD reg does not hold 0xAC after ready command
>> > > abituguru: timeout exceeded waiting for read state (bank: 38, sensor:
>> > 2)
>> > > abituguru: timeout exceeded waiting for ready state
>> > >
>> >
>> > Hmm, were these messages close together in time, or now and then a
>> > message?
>> >
>> > > I am not sure if they are related to a freeze I saw today. My machine
>> > > wasn't
>> > > being used and when I came back, it was frozen.
>> >
>> > I seriously doubt the freeze and these messages are related. The uGuru
>> > only uses 2 io ports todo all communication with the outside. So
>> reading
>> >
>> > the ports doesn't always get you the same register there is a sortoff
>> > network protocol which you speak over the ports. These messages mean
>> > that the uGuru failed to response within a reasonable time. Unfortunate
>> > on some uGuru's this is quite normal. It seems the older the
>> > Motherboard, the more often this happens (iow the buggier the uGuru
>> > firmware).
>> >
>> > Regards,
>> >
>> > Hans
>> >
>>
>>
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> lm-sensors mailing list
> lm-sensors at lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

it is AN8-SLI with BIOS 19. I have recompiled the module and haven't seen
the behaviour so far. I will post once I see the timeout messages and
failure during boot again.

-Sunil

On 7/9/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
>
>
>
> Sunil Kumar wrote:
> > Ok, now another problem. Some of the times the module loads fine during
> > bootup but sensors reports that
> >
> > $ sensors
> > Can't access procfs/sysfs file
> > Unable to find i2c bus information;
> > For 2.6 kernels, make sure you have mounted sysfs and libsensors
> > was compiled with sysfs support!
> > For older kernels, make sure you have done 'modprobe i2c-proc'!
> >
> > If I rmmod abituguru and modprobe it again, it works. I will rebuild the
> > module and see if it helps.
> >
> > The fallout of this is that gkrellm forgets about my sensors
> permanently. I
> > have to re-enable them and re-assign the labels etc....:(
> >
>
> Yes, this is because the module loads, but the driver part of it doesn't
> (new driver model, where drivers can be registered without actually
> handling any devices, so they can be bound to hotplug devices lateR).
>
>
> The driver part probably fails to load because it happens to run into
> those errors you report during the initialisation fase. Strange though
> as during init it retries all things 3 times, which goes to show that
> indeed your uGuru is unwilling to talk for a while, as seen before with
> early uGurus, what motherboard do you have. Lemme gues one out of this
> list? : AI7, KV8-MAX3, AN7
>
>
> Regards,
>
> Hans
>
>
>
>
> > Thanks,
> > -Sunil
> >
> > On 7/9/06, Sunil Kumar < devsku at gmail.com> wrote:
> >>
> >> they always occurr in a bunch but they are scattered all over time,
> >> sometimes disappearing for many hours and then returning.
> >>
> >>
> >> On 7/9/06, Hans de Goede < j.w.r.degoede at hhs.nl> wrote:
> >> >
> >> >
> >> >
> >> > Sunil Kumar wrote:
> >> > > Hi Hans,
> >> > >
> >> > > I am seeing these messages in the dmesg:
> >> > >
> >> > > abituguru: timeout exceeded waiting for ready state
> >> > > abituguru: timeout exceeded waiting for ready state
> >> > > abituguru: timeout exceeded waiting for read state (bank: 34,
> sensor:
> >> > 15)
> >> > > abituguru: CMD reg does not hold 0xAC after ready command
> >> > > abituguru: timeout exceeded waiting for read state (bank: 38,
> sensor:
> >> > 2)
> >> > > abituguru: timeout exceeded waiting for ready state
> >> > >
> >> >
> >> > Hmm, were these messages close together in time, or now and then a
> >> > message?
> >> >
> >> > > I am not sure if they are related to a freeze I saw today. My
> machine
> >> > > wasn't
> >> > > being used and when I came back, it was frozen.
> >> >
> >> > I seriously doubt the freeze and these messages are related. The
> uGuru
> >> > only uses 2 io ports todo all communication with the outside. So
> >> reading
> >> >
> >> > the ports doesn't always get you the same register there is a sortoff
> >> > network protocol which you speak over the ports. These messages mean
> >> > that the uGuru failed to response within a reasonable time.
> Unfortunate
> >> > on some uGuru's this is quite normal. It seems the older the
> >> > Motherboard, the more often this happens (iow the buggier the uGuru
> >> > firmware).
> >> >
> >> > Regards,
> >> >
> >> > Hans
> >> >
> >>
> >>
> >
> >
> > ------------------------------------------------------------------------
> >
> > _______________________________________________
> > lm-sensors mailing list
> > lm-sensors at lm-sensors.org
> > http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060709/89652033/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Hans de Goede]

Sunil Kumar wrote:
> it is AN8-SLI with BIOS 19.
Hmm, the uguru on that should have firmware version 2.3.x.x I wouldn't
expect much trouble with that, maybe my timeouts are just a bit too
close to the critical limit.

> I have recompiled the module and haven't seen
> the behaviour so far. I will post once I see the timeout messages and
> failure during boot again.
> 

I don't expect a recompile to fix it, but if it does thats great. what
you could try if it comes back is raising (try doubling for starters)
the following defines:
First the most likely candidate:
#define ABIT_UGURU_WAIT_TIMEOUT 250
No once all the "timeout exceeded waiting for xxxx state" messages are
gone I would expect the other one: "CMD reg does not hold 0xAC after
ready command" to also be gone, if its not try raisjng this define:
#define ABIT_UGURU_READY_TIMEOUT 50

Thanks & Regards,

Hans

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

Why I expect re-compile to work is that I might have done some kernel
rebuild with a newer option (I don't remember if I disabled HIGHPTE before
or after I built abituguru last time) and it is possible that the module
still loads fine but can't really get initialized the first time.

What is the unit for those timeouts you mentioned? In case I need to go
there.

With the recompiled module, I haven't seen it happen it so far after 4
reboots.

This is the advantage of having it in the kernel tree itself. When is it
expected to be in kernel mainline? 2.6.17 series? (I can't run mm for now.)

Thanks a lot,
-Sunil

On 7/9/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
>
>
>
> Sunil Kumar wrote:
> > it is AN8-SLI with BIOS 19.
> Hmm, the uguru on that should have firmware version 2.3.x.x I wouldn't
> expect much trouble with that, maybe my timeouts are just a bit too
> close to the critical limit.
>
> > I have recompiled the module and haven't seen
> > the behaviour so far. I will post once I see the timeout messages and
> > failure during boot again.
> >
>
> I don't expect a recompile to fix it, but if it does thats great. what
> you could try if it comes back is raising (try doubling for starters)
> the following defines:
> First the most likely candidate:
> #define ABIT_UGURU_WAIT_TIMEOUT 250
> No once all the "timeout exceeded waiting for xxxx state" messages are
> gone I would expect the other one: "CMD reg does not hold 0xAC after
> ready command" to also be gone, if its not try raisjng this define:
> #define ABIT_UGURU_READY_TIMEOUT 50
>
> Thanks & Regards,
>
> Hans
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060709/4faea60d/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Hans de Goede]

Sunil Kumar wrote:
> Why I expect re-compile to work is that I might have done some kernel 
> rebuild with a newer option (I don't remember if I disabled HIGHPTE 
> before or after I built abituguru last time) and it is possible that the 
> module still loads fine but can't really get initialized the first time.
> 

Ah I see.

> What is the unit for those timeouts you mentioned? In case I need to go 
> there.
> 
Number of attempted ISA reads while waiting for a certain response 
before giving up.

> This is the advantage of having it in the kernel tree itself. When is it 
> expected to be in kernel mainline? 2.6.17 series? (I can't run mm for now.)
>

Its in 2.2.18rc1

Regards,

Hans


p.s

Everyone I'm going on a short vacation till friday so don't expect any 
response /replies from me till saturday.

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

bad news: module re-compile is not the issue. The problem came back.

And this time I know why. This problem happens when I stop sensors, rmmod
the module, suspend2 to disk, resume from the swap image and modprobe the
module. After sometime, these messages start appearing in the log.

Is there anything that you want me to enable for debugging purposes, so that
we can capture more information when it happens next time? I can almost
always reproduce it with suspend resume cycle.

On 7/9/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
>
>
>
> Sunil Kumar wrote:
> > Why I expect re-compile to work is that I might have done some kernel
> > rebuild with a newer option (I don't remember if I disabled HIGHPTE
> > before or after I built abituguru last time) and it is possible that the
> > module still loads fine but can't really get initialized the first time.
> >
>
> Ah I see.
>
> > What is the unit for those timeouts you mentioned? In case I need to go
> > there.
> >
> Number of attempted ISA reads while waiting for a certain response
> before giving up.
>
> > This is the advantage of having it in the kernel tree itself. When is it
> > expected to be in kernel mainline? 2.6.17 series? (I can't run mm for
> now.)
> >
>
> Its in 2.2.18rc1
>
> Regards,
>
> Hans
>
>
> p.s
>
> Everyone I'm going on a short vacation till friday so don't expect any
> response /replies from me till saturday.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060710/6326e2f4/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Hans de Goede]

Sunil Kumar wrote:
> bad news: module re-compile is not the issue. The problem came back.
> 
> And this time I know why. This problem happens when I stop sensors, rmmod
> the module, suspend2 to disk, resume from the swap image and modprobe the
> module. After sometime, these messages start appearing in the log.
> 
> Is there anything that you want me to enable for debugging purposes, so
> that
> we can capture more information when it happens next time? I can almost
> always reproduce it with suspend resume cycle.
> 

Hmm,

After suspend/resume does it happen just once/twice, or does it happen
regulary after suspend / resume.

My current guess is that the uguru is no longer in what Olle and I have
named "ready" state after a suspend / resume. If that is the problem
then I need to find out howto add a suspend/resume handler to the driver
and on resume mark the uguru as not ready (1 line of code in the handler
:), then the driver will force it into ready state before trying to talk
to it.

Regards,

Hans

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

you mean the hardware itself is not in the right state? Or its in the right
state but not in the state that you expect to be the right state? I am
confused I guess because suspending to disk should be like a shutdown to the
hardware. suspend to RAM might be a different beast because of BIOS
involvement.

On 7/14/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
>
>
>
> Sunil Kumar wrote:
> > bad news: module re-compile is not the issue. The problem came back.
> >
> > And this time I know why. This problem happens when I stop sensors,
> rmmod
> > the module, suspend2 to disk, resume from the swap image and modprobe
> the
> > module. After sometime, these messages start appearing in the log.
> >
> > Is there anything that you want me to enable for debugging purposes, so
> > that
> > we can capture more information when it happens next time? I can almost
> > always reproduce it with suspend resume cycle.
> >
>
> Hmm,
>
> After suspend/resume does it happen just once/twice, or does it happen
> regulary after suspend / resume.
>
> My current guess is that the uguru is no longer in what Olle and I have
> named "ready" state after a suspend / resume. If that is the problem
> then I need to find out howto add a suspend/resume handler to the driver
> and on resume mark the uguru as not ready (1 line of code in the handler
> :), then the driver will force it into ready state before trying to talk
> to it.
>
> Regards,
>
> Hans
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060714/89dbb825/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Hans de Goede]

Sunil Kumar wrote:
> you mean the hardware itself is not in the right state? Or its in the right
> state but not in the state that you expect to be the right state? I am
> confused I guess because suspending to disk should be like a shutdown to
> the
> hardware. suspend to RAM might be a different beast because of BIOS
> involvement.
> 

The problem is that my driver currently does not take the option of
suspendin (in anyway) into account. When the driver loads it puts the
hardware in the "ready" state and whenever it does something with the
hardware (making it not ready) it puts it back in the ready state when
its done. So the driver expects the hardware to be in the ready state
when it next tries to read from the hardware. However due to the
suspend/resume cycly the hardware is most likely no longer in the ready
state. My driver should be able to recover from this but complains that
things did not go as expected. Hence my question:

>> After suspend/resume does it happen just once/twice, or does it happen
>> regulary after suspend / resume.

Because if it only happens once or twice after a single suspend/restore
then my theory is most likely correct and then I'll try to fix my
driver. If it happens many times after a single suspend/resume it might
very well be something else.

Regards,

Hans

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

ok, the  machine where it happened is not in front of me right now. So, I
can reliably answer your question only when I reach home.

On 7/14/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
>
>
>
> Sunil Kumar wrote:
> > you mean the hardware itself is not in the right state? Or its in the
> right
> > state but not in the state that you expect to be the right state? I am
> > confused I guess because suspending to disk should be like a shutdown to
> > the
> > hardware. suspend to RAM might be a different beast because of BIOS
> > involvement.
> >
>
> The problem is that my driver currently does not take the option of
> suspendin (in anyway) into account. When the driver loads it puts the
> hardware in the "ready" state and whenever it does something with the
> hardware (making it not ready) it puts it back in the ready state when
> its done. So the driver expects the hardware to be in the ready state
> when it next tries to read from the hardware. However due to the
> suspend/resume cycly the hardware is most likely no longer in the ready
> state. My driver should be able to recover from this but complains that
> things did not go as expected. Hence my question:
>
> >> After suspend/resume does it happen just once/twice, or does it happen
> >> regulary after suspend / resume.
>
> Because if it only happens once or twice after a single suspend/restore
> then my theory is most likely correct and then I'll try to fix my
> driver. If it happens many times after a single suspend/resume it might
> very well be something else.
>
> Regards,
>
> Hans
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060714/bffe4ddd/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

You are right. The messages happen to be much more sparse. After recent
resume, I have only seen one:

abituguru: timeout exceeded waiting for read state (bank: 33, sensor: 9)

Last time I saw after a resume:

Jul 10 21:25:10 localhost abituguru: timeout exceeded waiting for read state
(bank: 34, sensor: 12)
Jul 10 21:25:10 localhost abituguru: timeout exceeded waiting for ready
state
Jul 10 21:25:20 localhost abituguru: timeout exceeded waiting for read state
(bank: 34, sensor: 15)
Jul 10 21:25:20 localhost abituguru: timeout exceeded waiting for ready
state
Jul 10 21:27:10 localhost abituguru: timeout exceeded waiting for ready
state
Jul 10 21:27:10 localhost abituguru: timeout exceeded waiting for ready
state

-Sunil

On 7/14/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
>
>
>
> Sunil Kumar wrote:
> > you mean the hardware itself is not in the right state? Or its in the
> right
> > state but not in the state that you expect to be the right state? I am
> > confused I guess because suspending to disk should be like a shutdown to
> > the
> > hardware. suspend to RAM might be a different beast because of BIOS
> > involvement.
> >
>
> The problem is that my driver currently does not take the option of
> suspendin (in anyway) into account. When the driver loads it puts the
> hardware in the "ready" state and whenever it does something with the
> hardware (making it not ready) it puts it back in the ready state when
> its done. So the driver expects the hardware to be in the ready state
> when it next tries to read from the hardware. However due to the
> suspend/resume cycly the hardware is most likely no longer in the ready
> state. My driver should be able to recover from this but complains that
> things did not go as expected. Hence my question:
>
> >> After suspend/resume does it happen just once/twice, or does it happen
> >> regulary after suspend / resume.
>
> Because if it only happens once or twice after a single suspend/restore
> then my theory is most likely correct and then I'll try to fix my
> driver. If it happens many times after a single suspend/resume it might
> very well be something else.
>
> Regards,
>
> Hans
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060714/9d03d6b5/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Hans de Goede]

Sunil Kumar wrote:
> I used the file you provided with the 1.1.8 sources that I had from the 
> tar and I still get the same problem after resume from suspend:
> 
> abituguru: timeout exceeded waiting for read state (bank: 33, sensor: 8)
> abituguru: timeout exceeded waiting for ready state
> abituguru: timeout exceeded waiting for ready state
> abituguru: timeout exceeded waiting for ready state
> abituguru: timeout exceeded waiting for read state (bank: 34, sensor: 15)
> abituguru: timeout exceeded waiting for ready state
> 
> bank:34 and bank: 33 messages are separated by half hour, while 
> consecutive messages separate by few seconds, if that helps...:)
> 
> Also, note that I rmmod the module before I suspend and modprobe it 
> after I resume. With your code in place for suspend/resume, is that 
> still required? I want a safe suspend/resume, so I rmmod as many modules 
> I can before suspend.
>

That no longer should be nescesarry and might even be the cause of your 
problems, could you try just leaving it in the kernel during suspend?

> I am not sure if the messages are a problem but you are a better judge 
> than me on that one.
> 

They are harmless, but it would be nice to fix them never the less.

Regards,

Hans

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

leaving it loaded during suspend doesn't help with the messages, they still
popup. Do you think I should try and adjust the timeouts that you had
mentioned once?

On 7/18/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
>
>
>
> Sunil Kumar wrote:
> > I used the file you provided with the 1.1.8 sources that I had from the
> > tar and I still get the same problem after resume from suspend:
> >
> > abituguru: timeout exceeded waiting for read state (bank: 33, sensor: 8)
> > abituguru: timeout exceeded waiting for ready state
> > abituguru: timeout exceeded waiting for ready state
> > abituguru: timeout exceeded waiting for ready state
> > abituguru: timeout exceeded waiting for read state (bank: 34, sensor:
> 15)
> > abituguru: timeout exceeded waiting for ready state
> >
> > bank:34 and bank: 33 messages are separated by half hour, while
> > consecutive messages separate by few seconds, if that helps...:)
> >
> > Also, note that I rmmod the module before I suspend and modprobe it
> > after I resume. With your code in place for suspend/resume, is that
> > still required? I want a safe suspend/resume, so I rmmod as many modules
> > I can before suspend.
> >
>
> That no longer should be nescesarry and might even be the cause of your
> problems, could you try just leaving it in the kernel during suspend?
>
> > I am not sure if the messages are a problem but you are a better judge
> > than me on that one.
> >
>
> They are harmless, but it would be nice to fix them never the less.
>
> Regards,
>
> Hans
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060719/f258abe6/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

one thing I noticed is that now the occurrence of these messages has
increased a lot. They are just crowding my /var/log/messages like nothing
else. I am not sure what to do next.


On 7/19/06, Sunil Kumar <devsku at gmail.com> wrote:
>
> leaving it loaded during suspend doesn't help with the messages, they
> still popup. Do you think I should try and adjust the timeouts that you had
> mentioned once?
>
>
> On 7/18/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
> >
> >
> >
> > Sunil Kumar wrote:
> > > I used the file you provided with the 1.1.8 sources that I had from
> > the
> > > tar and I still get the same problem after resume from suspend:
> > >
> > > abituguru: timeout exceeded waiting for read state (bank: 33, sensor:
> > 8)
> > > abituguru: timeout exceeded waiting for ready state
> > > abituguru: timeout exceeded waiting for ready state
> > > abituguru: timeout exceeded waiting for ready state
> > > abituguru: timeout exceeded waiting for read state (bank: 34, sensor:
> > 15)
> > > abituguru: timeout exceeded waiting for ready state
> > >
> > > bank:34 and bank: 33 messages are separated by half hour, while
> > > consecutive messages separate by few seconds, if that helps...:)
> > >
> > > Also, note that I rmmod the module before I suspend and modprobe it
> > > after I resume. With your code in place for suspend/resume, is that
> > > still required? I want a safe suspend/resume, so I rmmod as many
> > modules
> > > I can before suspend.
> > >
> >
> > That no longer should be nescesarry and might even be the cause of your
> > problems, could you try just leaving it in the kernel during suspend?
> >
> > > I am not sure if the messages are a problem but you are a better judge
> >
> > > than me on that one.
> > >
> >
> > They are harmless, but it would be nice to fix them never the less.
> >
> > Regards,
> >
> > Hans
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060719/ca93e33d/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

ok, doubling both the timeouts you mentioned doesn't help. It doesn't help
because its not a wait in that while loop. Instead, its a tight loop which
now runs 500 iterations instead of 250 before and most of todays processors
will complete it in few micro seconds. I think we need a genuine sleep wait
there, don't we? what is the kernel routine to sleep for a few msecs?

-Sunil

On 7/19/06, Sunil Kumar <devsku at gmail.com> wrote:
>
> one thing I noticed is that now the occurrence of these messages has
> increased a lot. They are just crowding my /var/log/messages like nothing
> else. I am not sure what to do next.
>
>
>
> On 7/19/06, Sunil Kumar <devsku at gmail.com> wrote:
> >
> > leaving it loaded during suspend doesn't help with the messages, they
> > still popup. Do you think I should try and adjust the timeouts that you had
> > mentioned once?
> >
> >
> > On 7/18/06, Hans de Goede <j.w.r.degoede at hhs.nl > wrote:
> > >
> > >
> > >
> > > Sunil Kumar wrote:
> > > > I used the file you provided with the 1.1.8 sources that I had from
> > > the
> > > > tar and I still get the same problem after resume from suspend:
> > > >
> > > > abituguru: timeout exceeded waiting for read state (bank: 33,
> > > sensor: 8)
> > > > abituguru: timeout exceeded waiting for ready state
> > > > abituguru: timeout exceeded waiting for ready state
> > > > abituguru: timeout exceeded waiting for ready state
> > > > abituguru: timeout exceeded waiting for read state (bank: 34,
> > > sensor: 15)
> > > > abituguru: timeout exceeded waiting for ready state
> > > >
> > > > bank:34 and bank: 33 messages are separated by half hour, while
> > > > consecutive messages separate by few seconds, if that helps...:)
> > > >
> > > > Also, note that I rmmod the module before I suspend and modprobe it
> > > > after I resume. With your code in place for suspend/resume, is that
> > > > still required? I want a safe suspend/resume, so I rmmod as many
> > > modules
> > > > I can before suspend.
> > > >
> > >
> > > That no longer should be nescesarry and might even be the cause of
> > > your
> > > problems, could you try just leaving it in the kernel during suspend?
> > >
> > > > I am not sure if the messages are a problem but you are a better
> > > judge
> > > > than me on that one.
> > > >
> > >
> > > They are harmless, but it would be nice to fix them never the less.
> > >
> > > Regards,
> > >
> > > Hans
> > >
> > >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060719/53399022/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

Ok, this patch seems to have made the messages go away for good (even with
suspend/resume cycle). Because this patch helps fix it, I suspect that its a
race between setting the ready state and doing a read because the while loop
executes really fast and the hardware is not really that fast to respond
when you expect it in ABIT_UGURU_STATUS_READ state in the abituguru_read
call.

$ diff -u abituguru.c.timeouts abituguru.c
--- abituguru.c.timeouts        2006-07-19 15:46:02.000000000 -0700
+++ abituguru.c 2006-07-19 21:03:39.000000000 -0700
@@ -30,6 +30,7 @@
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
 #include <asm/io.h>
+#include <linux/delay.h>

 /* Banks */
 #define ABIT_UGURU_ALARM_BANK                  0x20 /* 1x 3 bytes */
@@ -74,7 +75,7 @@
 /* Normally all expected status in abituguru_ready, are reported after the
    first read, but sometimes not and we need to poll, 5 polls was not
enough
    50 sofar is. */
-#define ABIT_UGURU_READY_TIMEOUT               50
+#define ABIT_UGURU_READY_TIMEOUT               30
 /* Maximum 3 retries on timedout reads/writes, delay 200 ms before retrying
*/
 #define ABIT_UGURU_MAX_RETRIES                 3
 #define ABIT_UGURU_RETRY_DELAY                 (HZ/5)
@@ -251,6 +252,7 @@
        /* Cmd port MUST be read now and should contain 0xAC */
        while (inb_p(data->addr + ABIT_UGURU_CMD) != 0xAC) {
                timeout--;
+               msleep(1);
                if (timeout = 0) {
                        ABIT_UGURU_DEBUG(1,
                           "CMD reg does not hold 0xAC after ready
command\n");
@@ -263,6 +265,7 @@
        timeout = ABIT_UGURU_READY_TIMEOUT;
        while (inb_p(data->addr + ABIT_UGURU_DATA) !ABIT_UGURU_STATUS_INPUT) {
                timeout--;
+               msleep(1);
                if (timeout = 0) {
                        ABIT_UGURU_DEBUG(1,
                                "state != more input after ready
command\n");

Please see if its ok and include it in next release. You can ignore the
ABIT_UGURU_READY_TIMEOUT change if you want. I decreased it because I am
sleeping for 1 millisecond everytime in the loop and even lower numbers may
be enough. The original while loops were much much faster on my machine
(3800+ X2) than 30ms...:)

-Sunil

On 7/19/06, Sunil Kumar <devsku at gmail.com> wrote:
>
> ok, doubling both the timeouts you mentioned doesn't help. It doesn't help
> because its not a wait in that while loop. Instead, its a tight loop which
> now runs 500 iterations instead of 250 before and most of todays processors
> will complete it in few micro seconds. I think we need a genuine sleep wait
> there, don't we? what is the kernel routine to sleep for a few msecs?
>
> -Sunil
>
>
> On 7/19/06, Sunil Kumar <devsku at gmail.com> wrote:
> >
> > one thing I noticed is that now the occurrence of these messages has
> > increased a lot. They are just crowding my /var/log/messages like nothing
> > else. I am not sure what to do next.
> >
> >
> >
> > On 7/19/06, Sunil Kumar <devsku at gmail.com> wrote:
> > >
> > > leaving it loaded during suspend doesn't help with the messages, they
> > > still popup. Do you think I should try and adjust the timeouts that you had
> > > mentioned once?
> > >
> > >
> > > On 7/18/06, Hans de Goede < j.w.r.degoede at hhs.nl > wrote:
> > > >
> > > >
> > > >
> > > > Sunil Kumar wrote:
> > > > > I used the file you provided with the 1.1.8 sources that I had
> > > > from the
> > > > > tar and I still get the same problem after resume from suspend:
> > > > >
> > > > > abituguru: timeout exceeded waiting for read state (bank: 33,
> > > > sensor: 8)
> > > > > abituguru: timeout exceeded waiting for ready state
> > > > > abituguru: timeout exceeded waiting for ready state
> > > > > abituguru: timeout exceeded waiting for ready state
> > > > > abituguru: timeout exceeded waiting for read state (bank: 34,
> > > > sensor: 15)
> > > > > abituguru: timeout exceeded waiting for ready state
> > > > >
> > > > > bank:34 and bank: 33 messages are separated by half hour, while
> > > > > consecutive messages separate by few seconds, if that helps...:)
> > > > >
> > > > > Also, note that I rmmod the module before I suspend and modprobe
> > > > it
> > > > > after I resume. With your code in place for suspend/resume, is
> > > > that
> > > > > still required? I want a safe suspend/resume, so I rmmod as many
> > > > modules
> > > > > I can before suspend.
> > > > >
> > > >
> > > > That no longer should be nescesarry and might even be the cause of
> > > > your
> > > > problems, could you try just leaving it in the kernel during
> > > > suspend?
> > > >
> > > > > I am not sure if the messages are a problem but you are a better
> > > > judge
> > > > > than me on that one.
> > > > >
> > > >
> > > > They are harmless, but it would be nice to fix them never the less.
> > > >
> > > > Regards,
> > > >
> > > > Hans
> > > >
> > > >
> > >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060719/09bf3a4e/attachment-0001.html 

[lm-sensors] new abituguru driver in mm kernel

[Hans de Goede]

Sunil Kumar wrote:
> Ok, this patch seems to have made the messages go away for good (even with
> suspend/resume cycle). Because this patch helps fix it, I suspect that
> its a
> race between setting the ready state and doing a read because the while
> loop
> executes really fast and the hardware is not really that fast to respond
> when you expect it in ABIT_UGURU_STATUS_READ state in the abituguru_read
> call.
> 

Hi,

1) Thanks for all the testing and the patch
2) The while loop speed is not as fast as you think, since the while
   test condition contains a inb_p which will do 2 isa reads, thus the
   while loops speed is ISA bus bound, not CPU bound.
3) Normally I would object against the sleeping in the while since in
   the normal (no suspend problem) case, the loops are intented to
   execute pretty fast. In abituguru_update_device 38 read operations
   are done, if every read op is going to take milliseconds this will
   cause noticable stalls in sensors using applications.
4) However the 2 places where you've inserted the sleep are places where
   normally the code succeeds on the first read (so it never enters the
   while and thus never sleeps) the whole reason for the while was to
   try to fix the problem you're experiencing and you've fixed the fix,
   thanks!

So the fix is most definitly going in, thanks! Could you try the
attached version? I've also modified the suspend / resume code so that
it no longer unconditionally marks the uguru not_ready as that seemed to
make things worse, instead it now checks the uguru status and sets the
ready flag depending on the status.

Thanks,

Hans
-------------- next part --------------
A non-text attachment was scrubbed...
Name: abituguru.c
Type: text/x-csrc
Size: 50948 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060720/5c6b6421/attachment-0001.bin 

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

I had been getting the problem on a clean boot as well as after a
suspend/resume cycle. I have now tested it with both reboots and
suspend/resume cycles multiple times and it seems to work. I agree that the
worst case delay of 38*50~ 00 msec is big but I don't think we will hit
that.

anyway, I will give the attached a run and let you know.

Thanks,
Sunil


On 7/20/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
>
>
>
> Sunil Kumar wrote:
> > Ok, this patch seems to have made the messages go away for good (even
> with
> > suspend/resume cycle). Because this patch helps fix it, I suspect that
> > its a
> > race between setting the ready state and doing a read because the while
> > loop
> > executes really fast and the hardware is not really that fast to respond
> > when you expect it in ABIT_UGURU_STATUS_READ state in the abituguru_read
> > call.
> >
>
> Hi,
>
> 1) Thanks for all the testing and the patch
> 2) The while loop speed is not as fast as you think, since the while
>    test condition contains a inb_p which will do 2 isa reads, thus the
>    while loops speed is ISA bus bound, not CPU bound.
> 3) Normally I would object against the sleeping in the while since in
>    the normal (no suspend problem) case, the loops are intented to
>    execute pretty fast. In abituguru_update_device 38 read operations
>    are done, if every read op is going to take milliseconds this will
>    cause noticable stalls in sensors using applications.
> 4) However the 2 places where you've inserted the sleep are places where
>    normally the code succeeds on the first read (so it never enters the
>    while and thus never sleeps) the whole reason for the while was to
>    try to fix the problem you're experiencing and you've fixed the fix,
>    thanks!
>
> So the fix is most definitly going in, thanks! Could you try the
> attached version? I've also modified the suspend / resume code so that
> it no longer unconditionally marks the uguru not_ready as that seemed to
> make things worse, instead it now checks the uguru status and sets the
> ready flag depending on the status.
>
> Thanks,
>
> Hans
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060720/c3923013/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

hold on to this patch please, because I just saw two messages appear again
after two hours of uptime after resume. This hasn't happened in my previous
tests where machine was up even longer. I think there is more to it than
just sleeping a few milliseconds. It may very well be an issue with my
particular board.

On 7/20/06, Sunil Kumar <devsku at gmail.com> wrote:
>
> I had been getting the problem on a clean boot as well as after a
> suspend/resume cycle. I have now tested it with both reboots and
> suspend/resume cycles multiple times and it seems to work. I agree that the
> worst case delay of 38*50~ 00 msec is big but I don't think we will hit
> that.
>
> anyway, I will give the attached a run and let you know.
>
> Thanks,
> Sunil
>
>
>
> On 7/20/06, Hans de Goede < j.w.r.degoede at hhs.nl> wrote:
> >
> >
> >
> > Sunil Kumar wrote:
> > > Ok, this patch seems to have made the messages go away for good (even
> > with
> > > suspend/resume cycle). Because this patch helps fix it, I suspect that
> > > its a
> > > race between setting the ready state and doing a read because the
> > while
> > > loop
> > > executes really fast and the hardware is not really that fast to
> > respond
> > > when you expect it in ABIT_UGURU_STATUS_READ state in the
> > abituguru_read
> > > call.
> > >
> >
> > Hi,
> >
> > 1) Thanks for all the testing and the patch
> > 2) The while loop speed is not as fast as you think, since the while
> >    test condition contains a inb_p which will do 2 isa reads, thus the
> >    while loops speed is ISA bus bound, not CPU bound.
> > 3) Normally I would object against the sleeping in the while since in
> >    the normal (no suspend problem) case, the loops are intented to
> >    execute pretty fast. In abituguru_update_device 38 read operations
> >    are done, if every read op is going to take milliseconds this will
> >    cause noticable stalls in sensors using applications.
> > 4) However the 2 places where you've inserted the sleep are places where
> >
> >    normally the code succeeds on the first read (so it never enters the
> >    while and thus never sleeps) the whole reason for the while was to
> >    try to fix the problem you're experiencing and you've fixed the fix,
> >    thanks!
> >
> > So the fix is most definitly going in, thanks! Could you try the
> > attached version? I've also modified the suspend / resume code so that
> > it no longer unconditionally marks the uguru not_ready as that seemed to
> >
> > make things worse, instead it now checks the uguru status and sets the
> > ready flag depending on the status.
> >
> > Thanks,
> >
> > Hans
> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060720/4d07214e/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

sent too soon. I wanted to mention that the frequency of messages has
reduced by manyfolds after this patch.

On 7/20/06, Sunil Kumar <devsku at gmail.com> wrote:
>
> hold on to this patch please, because I just saw two messages appear again
> after two hours of uptime after resume. This hasn't happened in my previous
> tests where machine was up even longer. I think there is more to it than
> just sleeping a few milliseconds. It may very well be an issue with my
> particular board.
>
>
> On 7/20/06, Sunil Kumar <devsku at gmail.com> wrote:
> >
> > I had been getting the problem on a clean boot as well as after a
> > suspend/resume cycle. I have now tested it with both reboots and
> > suspend/resume cycles multiple times and it seems to work. I agree that the
> > worst case delay of 38*50~ 00 msec is big but I don't think we will hit
> > that.
> >
> > anyway, I will give the attached a run and let you know.
> >
> > Thanks,
> > Sunil
> >
> >
> >
> > On 7/20/06, Hans de Goede < j.w.r.degoede at hhs.nl> wrote:
> > >
> > >
> > >
> > > Sunil Kumar wrote:
> > > > Ok, this patch seems to have made the messages go away for good
> > > (even with
> > > > suspend/resume cycle). Because this patch helps fix it, I suspect
> > > that
> > > > its a
> > > > race between setting the ready state and doing a read because the
> > > while
> > > > loop
> > > > executes really fast and the hardware is not really that fast to
> > > respond
> > > > when you expect it in ABIT_UGURU_STATUS_READ state in the
> > > abituguru_read
> > > > call.
> > > >
> > >
> > > Hi,
> > >
> > > 1) Thanks for all the testing and the patch
> > > 2) The while loop speed is not as fast as you think, since the while
> > >    test condition contains a inb_p which will do 2 isa reads, thus the
> > >    while loops speed is ISA bus bound, not CPU bound.
> > > 3) Normally I would object against the sleeping in the while since in
> > >    the normal (no suspend problem) case, the loops are intented to
> > >    execute pretty fast. In abituguru_update_device 38 read operations
> > >    are done, if every read op is going to take milliseconds this will
> > >    cause noticable stalls in sensors using applications.
> > > 4) However the 2 places where you've inserted the sleep are places
> > > where
> > >    normally the code succeeds on the first read (so it never enters
> > > the
> > >    while and thus never sleeps) the whole reason for the while was to
> > >    try to fix the problem you're experiencing and you've fixed the
> > > fix,
> > >    thanks!
> > >
> > > So the fix is most definitly going in, thanks! Could you try the
> > > attached version? I've also modified the suspend / resume code so that
> > > it no longer unconditionally marks the uguru not_ready as that seemed
> > > to
> > > make things worse, instead it now checks the uguru status and sets the
> > > ready flag depending on the status.
> > >
> > > Thanks,
> > >
> > > Hans
> > >
> > >
> > >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060720/29abaf1b/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Hans de Goede]

Sunil Kumar wrote:
> sent too soon. I wanted to mention that the frequency of messages has
> reduced by manyfolds after this patch.
> 

I've sightly reduced the ABIT_UGURU_READY_TIMEOUT because we are
sleeping now. You decreased it from 50 to 30, I went with 25. Can you
try the driver for a couple of days with this set back to 50 and then
see if it still happens?

Thanks & Regards,

Hans

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

Few things I noted:

1. HZ%0 means that msleep(1) sleeps for  8 ms because msleep does
schedule_timeout for msecs_to_jiffies(m) + 1, and hence sleeps for 2
jiffies. That's longer than I desired. Only way out would be make HZ\x1000
because I can't sleep finer than 4ms with HZ%0. So, msleep(0) which sleeps
for one jiffy, may be adequate.

2. The problem that comes after applying the patch is from this part of the
code in routine abituguru_read():

        /* And read the data */
        for (i = 0; i < count; i++) {
                if (abituguru_wait(data, ABIT_UGURU_STATUS_READ)) {
                        ABIT_UGURU_DEBUG(1, "timeout exceeded waiting for "
                                "read state (bank: %d, sensor: %d)\n",
                                (int)bank_addr, (int)sensor_addr);
                        break;
                }
                buf[i] = inb(data->addr + ABIT_UGURU_CMD);
        }

I just printed value of i and found that it fails for i=1 i.e. it read fine
for i=0, which means that consecutive reads don't have enough wait in
between. This is not solvable in any way apart from doing a msleep(0) for
every read, which may be as large as 10ms for people with HZ\x100, and as
small as 1ms (which is probably ok) for HZ\x1000.

Is it possible to change this loop to something like:

        /* And read the data */
        for (i = 0; i < count; i++) {
                done=1;
                while (done)
                {
                        if (abituguru_wait(data, ABIT_UGURU_STATUS_READ))
                                msleep(0);
                        done=0;
                }
                if (abituguru_wait(data, ABIT_UGURU_STATUS_READ)) {
                        ABIT_UGURU_DEBUG(1, "timeout exceeded waiting for "
                                "read state (bank: %d, sensor: %d)\n",
                                (int)bank_addr, (int)sensor_addr);
                        break;
                }
                buf[i] = inb(data->addr + ABIT_UGURU_CMD);
        }

(declare done at the top)

i.e. sleep for 1 jiffy if abituguru_wait() fails, at most once. It has no
impact for boards which return from abituguru_wait in the desired state the
first time. For some, it might mean longer delays.

I don't want to put msleep(0) in abituguru_wait() because that function is
used in many places.

OR

Just remove the message from the for loop ...:-) i.e.

        /* And read the data */
        for (i = 0; i < count; i++) {
                if (abituguru_wait(data, ABIT_UGURU_STATUS_READ)) {
                   break;
                }
                buf[i] = inb(data->addr + ABIT_UGURU_CMD);
        }

Basically, I don't have an elegant solution to this problem...:-(

-Sunil


On 7/20/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
>
>
>
> Sunil Kumar wrote:
> > sent too soon. I wanted to mention that the frequency of messages has
> > reduced by manyfolds after this patch.
> >
>
> I've sightly reduced the ABIT_UGURU_READY_TIMEOUT because we are
> sleeping now. You decreased it from 50 to 30, I went with 25. Can you
> try the driver for a couple of days with this set back to 50 and then
> see if it still happens?
>
> Thanks & Regards,
>
> Hans
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060721/ae22e0d6/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

Hans, I have tested this patch against abituguru.c that you sent me last
time for last 4 days, and found that it works fine (tested both with HZ 250
and 1000). It basically sleep for 2 jiffies if the timeout in abituguru_wait
gets over. Since all other parameters remain the same (I reverted 25 to
original 50), in systems where this driver was working fine, it will keep
working the same way with the same speed. It is zero impact for working
existing systems.

--- abituguru.c.ORG     2006-07-24 20:21:39.000000000 -0700
+++ abituguru.c 2006-07-23 01:05:46.000000000 -0700
@@ -74,7 +74,7 @@
 #define ABIT_UGURU_WAIT_TIMEOUT                        250
 /* Normally all expected status in abituguru_ready, are reported after the
    first read, but sometimes not and we need to poll. */
-#define ABIT_UGURU_READY_TIMEOUT               25
+#define ABIT_UGURU_READY_TIMEOUT               50
 /* Maximum 3 retries on timedout reads/writes, delay 200 ms before retrying
*/
 #define ABIT_UGURU_MAX_RETRIES                 3
 #define ABIT_UGURU_RETRY_DELAY                 (HZ/5)
@@ -221,11 +221,17 @@
 static int abituguru_wait(struct abituguru_data *data, u8 state)
 {
        int timeout = ABIT_UGURU_WAIT_TIMEOUT;
+       int done = 0;

        while (inb_p(data->addr + ABIT_UGURU_DATA) != state) {
                timeout--;
-               if (timeout = 0)
+               if (timeout = 0 && done)
                        return -EBUSY;
+               if (!done && timeout = 1)
+               {
+                       msleep(1);
+                       done = 1;
+               }
        }
        return 0;
 }
@@ -334,8 +340,8 @@
        for (i = 0; i < count; i++) {
                if (abituguru_wait(data, ABIT_UGURU_STATUS_READ)) {
                        ABIT_UGURU_DEBUG(1, "timeout exceeded waiting for "
-                               "read state (bank: %d, sensor: %d)\n",
-                               (int)bank_addr, (int)sensor_addr);
+                               "read state (bank: %d, sensor: %d i: %d)\n",
+                               (int)bank_addr, (int)sensor_addr, i);
                        break;
                }
                buf[i] = inb(data->addr + ABIT_UGURU_CMD);

Please consider it for inclusion.

Thanks,
Sunil


On 7/21/06, Sunil Kumar <devsku at gmail.com> wrote:
>
> Few things I noted:
>
> 1. HZ%0 means that msleep(1) sleeps for  8 ms because msleep does
> schedule_timeout for msecs_to_jiffies(m) + 1, and hence sleeps for 2
> jiffies. That's longer than I desired. Only way out would be make HZ\x1000
> because I can't sleep finer than 4ms with HZ%0. So, msleep(0) which sleeps
> for one jiffy, may be adequate.
>
> 2. The problem that comes after applying the patch is from this part of
> the code in routine abituguru_read():
>
>         /* And read the data */
>         for (i = 0; i < count; i++) {
>                 if (abituguru_wait(data, ABIT_UGURU_STATUS_READ)) {
>                         ABIT_UGURU_DEBUG(1, "timeout exceeded waiting for
> "
>                                 "read state (bank: %d, sensor: %d)\n",
>                                 (int)bank_addr, (int)sensor_addr);
>                         break;
>                 }
>                 buf[i] = inb(data->addr + ABIT_UGURU_CMD);
>         }
>
> I just printed value of i and found that it fails for i=1 i.e. it read
> fine for i=0, which means that consecutive reads don't have enough wait in
> between. This is not solvable in any way apart from doing a msleep(0) for
> every read, which may be as large as 10ms for people with HZ\x100, and as
> small as 1ms (which is probably ok) for HZ\x1000.
>
> Is it possible to change this loop to something like:
>
>         /* And read the data */
>         for (i = 0; i < count; i++) {
>                 done=1;
>                 while (done)
>                 {
>                         if (abituguru_wait(data, ABIT_UGURU_STATUS_READ))
>                                 msleep(0);
>                         done=0;
>                 }
>                 if (abituguru_wait(data, ABIT_UGURU_STATUS_READ)) {
>                         ABIT_UGURU_DEBUG(1, "timeout exceeded waiting for
> "
>                                 "read state (bank: %d, sensor: %d)\n",
>                                 (int)bank_addr, (int)sensor_addr);
>                         break;
>                 }
>                 buf[i] = inb(data->addr + ABIT_UGURU_CMD);
>         }
>
> (declare done at the top)
>
> i.e. sleep for 1 jiffy if abituguru_wait() fails, at most once. It has no
> impact for boards which return from abituguru_wait in the desired state the
> first time. For some, it might mean longer delays.
>
> I don't want to put msleep(0) in abituguru_wait() because that function is
> used in many places.
>
> OR
>
> Just remove the message from the for loop ...:-) i.e.
>
>         /* And read the data */
>         for (i = 0; i < count; i++) {
>                 if (abituguru_wait(data, ABIT_UGURU_STATUS_READ)) {
>                    break;
>                 }
>                 buf[i] = inb(data->addr + ABIT_UGURU_CMD);
>         }
>
> Basically, I don't have an elegant solution to this problem...:-(
>
> -Sunil
>
>
> On 7/20/06, Hans de Goede < j.w.r.degoede at hhs.nl> wrote:
>
> >
> >
> > Sunil Kumar wrote:
> > > sent too soon. I wanted to mention that the frequency of messages has
> > > reduced by manyfolds after this patch.
> > >
> >
> > I've sightly reduced the ABIT_UGURU_READY_TIMEOUT because we are
> > sleeping now. You decreased it from 50 to 30, I went with 25. Can you
> > try the driver for a couple of days with this set back to 50 and then
> > see if it still happens?
> >
> > Thanks & Regards,
> >
> > Hans
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060724/ff6a5d9b/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Hans de Goede]

Sunil Kumar wrote:
> Hans, I have tested this patch against abituguru.c that you sent me last
> time for last 4 days, and found that it works fine (tested both with HZ 250
> and 1000). It basically sleep for 2 jiffies if the timeout in
> abituguru_wait
> gets over. Since all other parameters remain the same (I reverted 25 to
> original 50), in systems where this driver was working fine, it will keep
> working the same way with the same speed. It is zero impact for working
> existing systems.
> 

Thanks,

Thats an excellent way to fix it! But I see no need for the done flag,
just checking timeout should suffice, or am I missing something? Anywasy
the attached version has your patch but with the done flag removed.

I was thinking a bit about this myself too and I decided to make those
read timeouts retryable errors instead of throwing an error the first
time things fail. I've added this to this version too. With the
combination of these 2 these type of errors should really be history.

Please give the attached version a spin for a day or two, I assume it
will work fine, but better safe then sorry.

If it turns out ok then I'll submit this version for merging.

Thanks! & Regards,

Hans




> --- abituguru.c.ORG     2006-07-24 20:21:39.000000000 -0700
> +++ abituguru.c 2006-07-23 01:05:46.000000000 -0700
> @@ -74,7 +74,7 @@
> #define ABIT_UGURU_WAIT_TIMEOUT                        250
> /* Normally all expected status in abituguru_ready, are reported after the
>    first read, but sometimes not and we need to poll. */
> -#define ABIT_UGURU_READY_TIMEOUT               25
> +#define ABIT_UGURU_READY_TIMEOUT               50
> /* Maximum 3 retries on timedout reads/writes, delay 200 ms before retrying
> */
> #define ABIT_UGURU_MAX_RETRIES                 3
> #define ABIT_UGURU_RETRY_DELAY                 (HZ/5)
> @@ -221,11 +221,17 @@
> static int abituguru_wait(struct abituguru_data *data, u8 state)
> {
>        int timeout = ABIT_UGURU_WAIT_TIMEOUT;
> +       int done = 0;
> 
>        while (inb_p(data->addr + ABIT_UGURU_DATA) != state) {
>                timeout--;
> -               if (timeout = 0)
> +               if (timeout = 0 && done)
>                        return -EBUSY;
> +               if (!done && timeout = 1)
> +               {
> +                       msleep(1);
> +                       done = 1;
> +               }
>        }
>        return 0;
> }
> @@ -334,8 +340,8 @@
>        for (i = 0; i < count; i++) {
>                if (abituguru_wait(data, ABIT_UGURU_STATUS_READ)) {
>                        ABIT_UGURU_DEBUG(1, "timeout exceeded waiting for "
> -                               "read state (bank: %d, sensor: %d)\n",
> -                               (int)bank_addr, (int)sensor_addr);
> +                               "read state (bank: %d, sensor: %d i:
> %d)\n",
> +                               (int)bank_addr, (int)sensor_addr, i);
>                        break;
>                }
>                buf[i] = inb(data->addr + ABIT_UGURU_CMD);
> 
> Please consider it for inclusion.
> 
> Thanks,
> Sunil
> 
> 
> On 7/21/06, Sunil Kumar <devsku at gmail.com> wrote:
>>
>> Few things I noted:
>>
>> 1. HZ%0 means that msleep(1) sleeps for  8 ms because msleep does
>> schedule_timeout for msecs_to_jiffies(m) + 1, and hence sleeps for 2
>> jiffies. That's longer than I desired. Only way out would be make HZ\x1000
>> because I can't sleep finer than 4ms with HZ%0. So, msleep(0) which
>> sleeps
>> for one jiffy, may be adequate.
>>
>> 2. The problem that comes after applying the patch is from this part of
>> the code in routine abituguru_read():
>>
>>         /* And read the data */
>>         for (i = 0; i < count; i++) {
>>                 if (abituguru_wait(data, ABIT_UGURU_STATUS_READ)) {
>>                         ABIT_UGURU_DEBUG(1, "timeout exceeded waiting for
>> "
>>                                 "read state (bank: %d, sensor: %d)\n",
>>                                 (int)bank_addr, (int)sensor_addr);
>>                         break;
>>                 }
>>                 buf[i] = inb(data->addr + ABIT_UGURU_CMD);
>>         }
>>
>> I just printed value of i and found that it fails for i=1 i.e. it read
>> fine for i=0, which means that consecutive reads don't have enough
>> wait in
>> between. This is not solvable in any way apart from doing a msleep(0) for
>> every read, which may be as large as 10ms for people with HZ\x100, and as
>> small as 1ms (which is probably ok) for HZ\x1000.
>>
>> Is it possible to change this loop to something like:
>>
>>         /* And read the data */
>>         for (i = 0; i < count; i++) {
>>                 done=1;
>>                 while (done)
>>                 {
>>                         if (abituguru_wait(data, ABIT_UGURU_STATUS_READ))
>>                                 msleep(0);
>>                         done=0;
>>                 }
>>                 if (abituguru_wait(data, ABIT_UGURU_STATUS_READ)) {
>>                         ABIT_UGURU_DEBUG(1, "timeout exceeded waiting for
>> "
>>                                 "read state (bank: %d, sensor: %d)\n",
>>                                 (int)bank_addr, (int)sensor_addr);
>>                         break;
>>                 }
>>                 buf[i] = inb(data->addr + ABIT_UGURU_CMD);
>>         }
>>
>> (declare done at the top)
>>
>> i.e. sleep for 1 jiffy if abituguru_wait() fails, at most once. It has no
>> impact for boards which return from abituguru_wait in the desired
>> state the
>> first time. For some, it might mean longer delays.
>>
>> I don't want to put msleep(0) in abituguru_wait() because that
>> function is
>> used in many places.
>>
>> OR
>>
>> Just remove the message from the for loop ...:-) i.e.
>>
>>         /* And read the data */
>>         for (i = 0; i < count; i++) {
>>                 if (abituguru_wait(data, ABIT_UGURU_STATUS_READ)) {
>>                    break;
>>                 }
>>                 buf[i] = inb(data->addr + ABIT_UGURU_CMD);
>>         }
>>
>> Basically, I don't have an elegant solution to this problem...:-(
>>
>> -Sunil
>>
>>
>> On 7/20/06, Hans de Goede < j.w.r.degoede at hhs.nl> wrote:
>>
>> >
>> >
>> > Sunil Kumar wrote:
>> > > sent too soon. I wanted to mention that the frequency of messages has
>> > > reduced by manyfolds after this patch.
>> > >
>> >
>> > I've sightly reduced the ABIT_UGURU_READY_TIMEOUT because we are
>> > sleeping now. You decreased it from 50 to 30, I went with 25. Can you
>> > try the driver for a couple of days with this set back to 50 and then
>> > see if it still happens?
>> >
>> > Thanks & Regards,
>> >
>> > Hans
>> >
>>
>>
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: abituguru.c
Type: text/x-csrc
Size: 51115 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060726/912479aa/attachment-0001.bin 

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

yeah, 'done' was remnant of another debugging session and lamely left there.

Hans, I have done some very rudimentary performance analysis of this driver
and I have a feeling that some tuning may be in order. All I did was to look
at gkrellm with abituguru loaded and unloaded. gkrellm updates sensors every
5 seconds (which is kinda high I think) and I see that the cpu usage is 0-2%
if module is unloaded. If module is loaded the cpu usage is as high as 4%
every 5 seconds (more than half of it red). System is quiet and nothing else
is being done on it.

I suspect its to do with the while loop in abitugru_wait(). What I did was
to make the following change on top of what you sent and cpu usage became 2%
every 5 seconds. So, it remains between 0-2%.

I don't think there is any harm in sleeping because most sensor user
programs are working at seconds level and we are talking about ms sleep. And
in most cases first sleep for 1 jiffy will be enough. But this probably
needs testing by our volunteers to see if there are any ill-effects,
although it works perfectly here.

--- /tmp/abituguru.c    2006-07-26 11:16:32.000000000 -0700
+++ ./abituguru.c       2006-07-26 11:14:04.000000000 -0700
@@ -72,9 +72,10 @@
    the CPU should be the bottleneck. Note that 250 sometimes is still not
    enough (only reported on AN7 mb) this is handled by a higher layer. */
 #define ABIT_UGURU_WAIT_TIMEOUT                        250
+#define ABIT_UGURU_WAIT_TIMEOUT_SLEEP          200
 /* Normally all expected status in abituguru_ready, are reported after the
    first read, but sometimes not and we need to poll. */
-#define ABIT_UGURU_READY_TIMEOUT               50
+#define ABIT_UGURU_READY_TIMEOUT               5
 /* Maximum 3 retries on timedout reads/writes, delay 200 ms before retrying
*/
 #define ABIT_UGURU_MAX_RETRIES                 3
 #define ABIT_UGURU_RETRY_DELAY                 (HZ/5)
@@ -226,10 +227,11 @@
                timeout--;
                if (timeout = 0)
                        return -EBUSY;
-               /* sleep a bit before our last try, to give the uGuru one
-                  last chance to respond. */
-               if (timeout = 1)
-                       msleep(1);
+               /* start sleeping a bit after
ABIT_UGURU_WAIT_TIMEOUT-ABIT_UGURU_WAIT_TIMEOUT_SLEEP
+                * iterations to give the uGuru max chance to respond.
+                */
+               if (timeout <= ABIT_UGURU_WAIT_TIMEOUT_SLEEP)
+                       msleep(0);
        }
        return 0;
 }

Thanks,
Sunil


On 7/26/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
>
>
>
> Sunil Kumar wrote:
> > Hans, I have tested this patch against abituguru.c that you sent me last
> > time for last 4 days, and found that it works fine (tested both with HZ
> 250
> > and 1000). It basically sleep for 2 jiffies if the timeout in
> > abituguru_wait
> > gets over. Since all other parameters remain the same (I reverted 25 to
> > original 50), in systems where this driver was working fine, it will
> keep
> > working the same way with the same speed. It is zero impact for working
> > existing systems.
> >
>
> Thanks,
>
> Thats an excellent way to fix it! But I see no need for the done flag,
> just checking timeout should suffice, or am I missing something? Anywasy
> the attached version has your patch but with the done flag removed.
>
> I was thinking a bit about this myself too and I decided to make those
> read timeouts retryable errors instead of throwing an error the first
> time things fail. I've added this to this version too. With the
> combination of these 2 these type of errors should really be history.
>
> Please give the attached version a spin for a day or two, I assume it
> will work fine, but better safe then sorry.
>
> If it turns out ok then I'll submit this version for merging.
>
> Thanks! & Regards,
>
> Hans
>
>
>
>
> > --- abituguru.c.ORG     2006-07-24 20:21:39.000000000 -0700
> > +++ abituguru.c 2006-07-23 01:05:46.000000000 -0700
> > @@ -74,7 +74,7 @@
> > #define ABIT_UGURU_WAIT_TIMEOUT                        250
> > /* Normally all expected status in abituguru_ready, are reported after
> the
> >    first read, but sometimes not and we need to poll. */
> > -#define ABIT_UGURU_READY_TIMEOUT               25
> > +#define ABIT_UGURU_READY_TIMEOUT               50
> > /* Maximum 3 retries on timedout reads/writes, delay 200 ms before
> retrying
> > */
> > #define ABIT_UGURU_MAX_RETRIES                 3
> > #define ABIT_UGURU_RETRY_DELAY                 (HZ/5)
> > @@ -221,11 +221,17 @@
> > static int abituguru_wait(struct abituguru_data *data, u8 state)
> > {
> >        int timeout = ABIT_UGURU_WAIT_TIMEOUT;
> > +       int done = 0;
> >
> >        while (inb_p(data->addr + ABIT_UGURU_DATA) != state) {
> >                timeout--;
> > -               if (timeout = 0)
> > +               if (timeout = 0 && done)
> >                        return -EBUSY;
> > +               if (!done && timeout = 1)
> > +               {
> > +                       msleep(1);
> > +                       done = 1;
> > +               }
> >        }
> >        return 0;
> > }
> > @@ -334,8 +340,8 @@
> >        for (i = 0; i < count; i++) {
> >                if (abituguru_wait(data, ABIT_UGURU_STATUS_READ)) {
> >                        ABIT_UGURU_DEBUG(1, "timeout exceeded waiting for
> "
> > -                               "read state (bank: %d, sensor: %d)\n",
> > -                               (int)bank_addr, (int)sensor_addr);
> > +                               "read state (bank: %d, sensor: %d i:
> > %d)\n",
> > +                               (int)bank_addr, (int)sensor_addr, i);
> >                        break;
> >                }
> >                buf[i] = inb(data->addr + ABIT_UGURU_CMD);
> >
> > Please consider it for inclusion.
> >
> > Thanks,
> > Sunil
> >
> >
> > On 7/21/06, Sunil Kumar <devsku at gmail.com> wrote:
> >>
> >> Few things I noted:
> >>
> >> 1. HZ%0 means that msleep(1) sleeps for  8 ms because msleep does
> >> schedule_timeout for msecs_to_jiffies(m) + 1, and hence sleeps for 2
> >> jiffies. That's longer than I desired. Only way out would be make
> HZ\x1000
> >> because I can't sleep finer than 4ms with HZ%0. So, msleep(0) which
> >> sleeps
> >> for one jiffy, may be adequate.
> >>
> >> 2. The problem that comes after applying the patch is from this part of
> >> the code in routine abituguru_read():
> >>
> >>         /* And read the data */
> >>         for (i = 0; i < count; i++) {
> >>                 if (abituguru_wait(data, ABIT_UGURU_STATUS_READ)) {
> >>                         ABIT_UGURU_DEBUG(1, "timeout exceeded waiting
> for
> >> "
> >>                                 "read state (bank: %d, sensor: %d)\n",
> >>                                 (int)bank_addr, (int)sensor_addr);
> >>                         break;
> >>                 }
> >>                 buf[i] = inb(data->addr + ABIT_UGURU_CMD);
> >>         }
> >>
> >> I just printed value of i and found that it fails for i=1 i.e. it read
> >> fine for i=0, which means that consecutive reads don't have enough
> >> wait in
> >> between. This is not solvable in any way apart from doing a msleep(0)
> for
> >> every read, which may be as large as 10ms for people with HZ\x100, and
> as
> >> small as 1ms (which is probably ok) for HZ\x1000.
> >>
> >> Is it possible to change this loop to something like:
> >>
> >>         /* And read the data */
> >>         for (i = 0; i < count; i++) {
> >>                 done=1;
> >>                 while (done)
> >>                 {
> >>                         if (abituguru_wait(data,
> ABIT_UGURU_STATUS_READ))
> >>                                 msleep(0);
> >>                         done=0;
> >>                 }
> >>                 if (abituguru_wait(data, ABIT_UGURU_STATUS_READ)) {
> >>                         ABIT_UGURU_DEBUG(1, "timeout exceeded waiting
> for
> >> "
> >>                                 "read state (bank: %d, sensor: %d)\n",
> >>                                 (int)bank_addr, (int)sensor_addr);
> >>                         break;
> >>                 }
> >>                 buf[i] = inb(data->addr + ABIT_UGURU_CMD);
> >>         }
> >>
> >> (declare done at the top)
> >>
> >> i.e. sleep for 1 jiffy if abituguru_wait() fails, at most once. It has
> no
> >> impact for boards which return from abituguru_wait in the desired
> >> state the
> >> first time. For some, it might mean longer delays.
> >>
> >> I don't want to put msleep(0) in abituguru_wait() because that
> >> function is
> >> used in many places.
> >>
> >> OR
> >>
> >> Just remove the message from the for loop ...:-) i.e.
> >>
> >>         /* And read the data */
> >>         for (i = 0; i < count; i++) {
> >>                 if (abituguru_wait(data, ABIT_UGURU_STATUS_READ)) {
> >>                    break;
> >>                 }
> >>                 buf[i] = inb(data->addr + ABIT_UGURU_CMD);
> >>         }
> >>
> >> Basically, I don't have an elegant solution to this problem...:-(
> >>
> >> -Sunil
> >>
> >>
> >> On 7/20/06, Hans de Goede < j.w.r.degoede at hhs.nl> wrote:
> >>
> >> >
> >> >
> >> > Sunil Kumar wrote:
> >> > > sent too soon. I wanted to mention that the frequency of messages
> has
> >> > > reduced by manyfolds after this patch.
> >> > >
> >> >
> >> > I've sightly reduced the ABIT_UGURU_READY_TIMEOUT because we are
> >> > sleeping now. You decreased it from 50 to 30, I went with 25. Can you
> >> > try the driver for a couple of days with this set back to 50 and then
> >> > see if it still happens?
> >> >
> >> > Thanks & Regards,
> >> >
> >> > Hans
> >> >
> >>
> >>
> >
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060726/8bfa2219/attachment-0001.html 

[lm-sensors] new abituguru driver in mm kernel

[Hans de Goede]

Sunil Kumar wrote:
> Hans, I have done some very rudimentary performance analysis of this driver
> and I have a feeling that some tuning may be in order. All I did was to
> look
> at gkrellm with abituguru loaded and unloaded. gkrellm updates sensors
> every
> 5 seconds (which is kinda high I think) and I see that the cpu usage is
> 0-2%
> if module is unloaded. If module is loaded the cpu usage is as high as 4%
> every 5 seconds (more than half of it red). System is quiet and nothing
> else
> is being done on it.
> 
> I suspect its to do with the while loop in abitugru_wait(). What I did was
> to make the following change on top of what you sent and cpu usage
> became 2%
> every 5 seconds. So, it remains between 0-2%.
> 

Hmm, how did you measure this? According to top gkrellm on my system
never comes above the 0.7 percent, then again on my system the wait
function usually returns pretty fast.

> I don't think there is any harm in sleeping because most sensor user
> programs are working at seconds level and we are talking about ms sleep.
> And
> in most cases first sleep for 1 jiffy will be enough. But this probably
> needs testing by our volunteers to see if there are any ill-effects,
> although it works perfectly here.
> 

My worries are because abituguru_wait gets called 148 times for one
update! So on a system running at 100 HZ, that could get translated to
blocking the program trying to read a sensor for 1.5 seconds.

However on my system the uGuru usually responds within 50 polls in
abituguru wait with 1 in 10 waits only succeeding after 90 polls. So if
we can agree to make ABIT_UGURU_WAIT_TIMEOUT_SLEEP 150 instead of 200
then I think your patch can go in to help those with uGuru's which are
slower to respond like yours is.

The 1.5 seconds mentioned above assumes 1 sleep per wait, and with your
current code it could become much more sleeps. Thinking about this some
more, with your current code it could become 200 / 150 sleeps per wait!

I think the correct fix for this would be to just lower
ABIT_UGURU_WAIT_TIMEOUT from 250 to 100 now that we have the sleep
before final try code. That way we wil not sleep more then once per
wait() and we should still get the desired effect of lowering cpu usage.
Since your last version (sleep 1 ms before final try) worked always
without errors. I'm pretty sure we can just lower
ABIT_UGURU_WAIT_TIMEOUT to 100, those missed 150 reads won't make much
of a difference timing wise when compared with that msleep(1), so things
should still work as expected. The only differences being that wait()
will go to sleep sooner, resulting in the decreased cpu usage you want.

Could you give the last version I send you with ABIT_UGURU_WAIT_TIMEOUT
changed to 100 a try? I think that should do the trick.

Regards,

Hans

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

On 7/26/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
>
> Hmm, how did you measure this? According to top gkrellm on my system
> never comes above the 0.7 percent, then again on my system the wait
> function usually returns pretty fast.


by simply staring at the gkrellm chart where it plots cpu usage and shows a
number in %age for it as well. The jump from 2% to 4% is easily noticeable,
particulalrly when it happens every 5 second on a quiet system, but I think
a more detailed analysis should probably be done.

My worries are because abituguru_wait gets called 148 times for one


So many reads actually make the case for  tight loop weaker and for sleeping
stronger, because the loop actually will run 250*148~7000 times per update
without doing anything useful apart from seeing if its ok to read. I am sure
lesser CPUs will see more %age being used every 5 seconds. May be we can
have someone with 1ghz cpu to monitor gkrellm with and without abituguru and
report.

update! So on a system running at 100 HZ, that could get translated to
> blocking the program trying to read a sensor for 1.5 seconds.


This is the unfortunate part. People with 100 hz will suffer terrible delay
if the read state is not reached fast. But we are talking about a monitoring
program, not some essential kernel component which can fail something
critical. For 1000hz it is only 150ms. Note that it is advised to run linux
2.6 with HZ 1000. May be we can make the determination of how early to go to
sleep based upon the value of HZ, by adjusting the difference between
TIMEOUT and TIMEOUT_SLEEP dynamically based on HZ. Please also realize that
with every 1 jiffy sleep, the chances of reaching the next sleep reduce
tremendously (unless uguru has died and not responding at all...:)) But I
have a feeling that sleep in steps of 1 jiffy is in order if loop times out
50 or so iterations.

Also, someone using 100 hz will never like a long tight loop in anything
periodic like sensors because the whole point of choosing hz 100 was to give
as much cpu to useful work as possible (like on a server). But, as I said,
its an unfortunate conflicting requirement.

How about this:

--- /tmp/abituguru.c    2006-07-26 11:16:32.000000000 -0700
+++ ./abituguru.c       2006-07-26 17:41:05.000000000 -0700
@@ -72,9 +72,10 @@
    the CPU should be the bottleneck. Note that 250 sometimes is still not
    enough (only reported on AN7 mb) this is handled by a higher layer. */
 #define ABIT_UGURU_WAIT_TIMEOUT                        250
+#define ABIT_UGURU_WAIT_TIMEOUT_SLEEP          200
 /* Normally all expected status in abituguru_ready, are reported after the
    first read, but sometimes not and we need to poll. */
-#define ABIT_UGURU_READY_TIMEOUT               50
+#define ABIT_UGURU_READY_TIMEOUT               5
 /* Maximum 3 retries on timedout reads/writes, delay 200 ms before retrying
*/
 #define ABIT_UGURU_MAX_RETRIES                 3
 #define ABIT_UGURU_RETRY_DELAY                 (HZ/5)
@@ -221,15 +222,17 @@
 static int abituguru_wait(struct abituguru_data *data, u8 state)
 {
        int timeout = ABIT_UGURU_WAIT_TIMEOUT;
+       int start_sleep = (HZ < 1000) ? ABIT_UGURU_WAIT_TIMEOUT_SLEEP / 2 :
ABIT_UGURU_WAIT_TIMEOUT_SLEEP;

        while (inb_p(data->addr + ABIT_UGURU_DATA) != state) {
                timeout--;
                if (timeout = 0)
                        return -EBUSY;
-               /* sleep a bit before our last try, to give the uGuru one
-                  last chance to respond. */
-               if (timeout = 1)
-                       msleep(1);
+               /* start sleeping a bit after
ABIT_UGURU_WAIT_TIMEOUT-start_sleep
+                * iterations to give the uGuru some chance to respond.
+                */
+               if (timeout <= start_sleep && timeout >= 1)
+                       msleep(0);
        }
        return 0;
 }

So, it will go 150 iterations before it starts sleeping on 100/250hz. Note
also that this patch has reduced READY timeout to 5 as well. Even with 5, I
haven't hit ready state timeout so far.

-Sunil
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060726/f9a3aa52/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Hans de Goede]

Sunil Kumar wrote:
> On 7/26/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
>>
>> Hmm, how did you measure this? According to top gkrellm on my system
>> never comes above the 0.7 percent, then again on my system the wait
>> function usually returns pretty fast.
> 
> 
> by simply staring at the gkrellm chart where it plots cpu usage and shows a
> number in %age for it as well. The jump from 2% to 4% is easily noticeable,
> particulalrly when it happens every 5 second on a quiet system, but I think
> a more detailed analysis should probably be done.
> 
> 

Ah, just about as scientific as my way.

> My worries are because abituguru_wait gets called 148 times for one
> 
> 
> So many reads actually make the case for  tight loop weaker and for
> sleeping
> stronger, because the loop actually will run 250*148~7000 times per
> update
> without doing anything useful apart from seeing if its ok to read. I am
> sure
> lesser CPUs will see more %age being used every 5 seconds. May be we can
> have someone with 1ghz cpu to monitor gkrellm with and without abituguru
> and
> report.
> 

Erm, that is incorrect twice. First of all on average it will only do
40-50 iterations in the loop. The 250 is a worst case scenario, and
appearantly in reality not enough (with your motherboard). This is
exactly why I've suggested lowering ABIT_UGURU_WAIT_TIMEOUT to 100, so
that we make the worsecase scenario burn less CPU, without slowing down
the normal case. We can lower it now, since normally all waits will
succeed within that 100, and in the few exceptions we have the sleep
now, which should be more then enough a wait for the uguru to respond if
it was distracted.

Also the CPU usage will be CPU-speed independent. This loops execution
speed is throttled by the ISA bus which runs at 7Mhz fixed. So the
execution time (and thus %age) will be the same independent of CPU-speed.


> update! So on a system running at 100 HZ, that could get translated to
>> blocking the program trying to read a sensor for 1.5 seconds.
> 
> 
> This is the unfortunate part. People with 100 hz will suffer terrible delay
> if the read state is not reached fast. But we are talking about a
> monitoring
> program, not some essential kernel component which can fail something
> critical. For 1000hz it is only 150ms. Note that it is advised to run linux
> 2.6 with HZ 1000. May be we can make the determination of how early to
> go to
> sleep based upon the value of HZ, by adjusting the difference between
> TIMEOUT and TIMEOUT_SLEEP dynamically based on HZ. Please also realize that
> with every 1 jiffy sleep, the chances of reaching the next sleep reduce
> tremendously (unless uguru has died and not responding at all...:)) But I
> have a feeling that sleep in steps of 1 jiffy is in order if loop times out
> 50 or so iterations.
> 
> Also, someone using 100 hz will never like a long tight loop in anything
> periodic like sensors because the whole point of choosing hz 100 was to
> give
> as much cpu to useful work as possible (like on a server). But, as I said,
> its an unfortunate conflicting requirement.
> 

Thats because of the unfortunate bad uGuru interface design :|
Anyways as said before could you please give the last version I mailed
you with ABIT_UGURU_WAIT_TIMEOUT changed to 100 a try, I think that will
cure most of you CPU usage problem in a nice simple and clean way.

Regards,

Hans

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

actually the reason I am insisting on doing more than one sleep is that one
sleep actually didn't work for me. It worked for four days (the uptime was
four days but 'on' time was probably one day cumulative because those were
working days and I was suspending the system down at night and in the
morning before going to work) and I assumed that it was fine but then it
popped its head again in the /var/log/messages. My cpu concern is already
addressed by making the TIMEOUT as 100 as you suggested.

I think the middle ground will be to make this a configuration parameter for
the driver because this sleep is going to vary from board to board e.g. 100
works for you and sometimes 100+msleep(1) doesn't work for me. The default
could be 1 and max could be 50, and these are taken out of the TIMEOUT i.e.
if value is set to 50, the loop executes for 50 and then starts to sleep for
the rest 50.

A runtime parameter would be better but I am fine with a config parameter
too. does that work?

-Sunil

On 7/27/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
>
>
>
> Sunil Kumar wrote:
> > On 7/26/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
> >>
> >> Hmm, how did you measure this? According to top gkrellm on my system
> >> never comes above the 0.7 percent, then again on my system the wait
> >> function usually returns pretty fast.
> >
> >
> > by simply staring at the gkrellm chart where it plots cpu usage and
> shows a
> > number in %age for it as well. The jump from 2% to 4% is easily
> noticeable,
> > particulalrly when it happens every 5 second on a quiet system, but I
> think
> > a more detailed analysis should probably be done.
> >
> >
>
> Ah, just about as scientific as my way.
>
> > My worries are because abituguru_wait gets called 148 times for one
> >
> >
> > So many reads actually make the case for  tight loop weaker and for
> > sleeping
> > stronger, because the loop actually will run 250*148~7000 times per
> > update
> > without doing anything useful apart from seeing if its ok to read. I am
> > sure
> > lesser CPUs will see more %age being used every 5 seconds. May be we can
> > have someone with 1ghz cpu to monitor gkrellm with and without abituguru
> > and
> > report.
> >
>
> Erm, that is incorrect twice. First of all on average it will only do
> 40-50 iterations in the loop. The 250 is a worst case scenario, and
> appearantly in reality not enough (with your motherboard). This is
> exactly why I've suggested lowering ABIT_UGURU_WAIT_TIMEOUT to 100, so
> that we make the worsecase scenario burn less CPU, without slowing down
> the normal case. We can lower it now, since normally all waits will
> succeed within that 100, and in the few exceptions we have the sleep
> now, which should be more then enough a wait for the uguru to respond if
> it was distracted.
>
> Also the CPU usage will be CPU-speed independent. This loops execution
> speed is throttled by the ISA bus which runs at 7Mhz fixed. So the
> execution time (and thus %age) will be the same independent of CPU-speed.
>
>
> > update! So on a system running at 100 HZ, that could get translated to
> >> blocking the program trying to read a sensor for 1.5 seconds.
> >
> >
> > This is the unfortunate part. People with 100 hz will suffer terrible
> delay
> > if the read state is not reached fast. But we are talking about a
> > monitoring
> > program, not some essential kernel component which can fail something
> > critical. For 1000hz it is only 150ms. Note that it is advised to run
> linux
> > 2.6 with HZ 1000. May be we can make the determination of how early to
> > go to
> > sleep based upon the value of HZ, by adjusting the difference between
> > TIMEOUT and TIMEOUT_SLEEP dynamically based on HZ. Please also realize
> that
> > with every 1 jiffy sleep, the chances of reaching the next sleep reduce
> > tremendously (unless uguru has died and not responding at all...:)) But
> I
> > have a feeling that sleep in steps of 1 jiffy is in order if loop times
> out
> > 50 or so iterations.
> >
> > Also, someone using 100 hz will never like a long tight loop in anything
> > periodic like sensors because the whole point of choosing hz 100 was to
> > give
> > as much cpu to useful work as possible (like on a server). But, as I
> said,
> > its an unfortunate conflicting requirement.
> >
>
> Thats because of the unfortunate bad uGuru interface design :|
> Anyways as said before could you please give the last version I mailed
> you with ABIT_UGURU_WAIT_TIMEOUT changed to 100 a try, I think that will
> cure most of you CPU usage problem in a nice simple and clean way.
>
> Regards,
>
> Hans
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060727/9b13d215/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Hans de Goede]

Sunil Kumar wrote:
> actually the reason I am insisting on doing more than one sleep is that one
> sleep actually didn't work for me. It worked for four days (the uptime was
> four days but 'on' time was probably one day cumulative because those were
> working days and I was suspending the system down at night and in the
> morning before going to work) and I assumed that it was fine but then it
> popped its head again in the /var/log/messages. My cpu concern is already
> addressed by making the TIMEOUT as 100 as you suggested.
> 
> I think the middle ground will be to make this a configuration parameter
> for
> the driver because this sleep is going to vary from board to board e.g. 100
> works for you and sometimes 100+msleep(1) doesn't work for me. The default
> could be 1 and max could be 50, and these are taken out of the TIMEOUT i.e.
> if value is set to 50, the loop executes for 50 and then starts to sleep
> for
> the rest 50.
> 
> A runtime parameter would be better but I am fine with a config parameter
> too. does that work?
> 

Hmm,

I really don't want to add all kinda knobs a user needs to tune for
things to work. Did you see this message return despite of the sleep
with my latest version of the driver? Because with my new driver you
shouldn't see it as its treated as retryable (which means the driver
will silently return the old values and try again next update, it will
only start to scream on 3 (define) or more consecutive errors.)

Noticed you also decreased the ABIT_UGURU_READY_TIMEOUT from 50 to 5,
maybe that is the problem now. I agree 50 is to much, but maybe 5 isn't
enough? What was the exact error you saw?

Since the sleep is in what is in essence an error handling path, Its
fine by me to sleep say 3 times before giving up, it should be very rare
that the uguru then still isn't responding. Together with making the
wait for read statsu retryable and thus in this rare occasion returning
oldvalues to userspace instead of an error I think we should be fine.
Does this sound like a plan?

I'm glad atleast we agree on the CPU-usage being fixed by setting the
default TIMEOUT to 100.

Regards,

Hans

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

the timeout happened for the read state with i=2 (yeah, I keep that around
to know how far my uguru could go) i.e in the for loop, it went fine for 2
reads and got stuck at 3rd read, and didn't even make it to the ready state
part in the end of that routine. Although, there is another path to ready
state routine, I haven't seen ready state timeout for a long time now (I
think because of the msleep(1) in the while loops, which we put in right in
the beginning of these discussions). So, 5 probably is fine.

I haven't tried the TIMEOUT 100 as yet but it came back with 250+msleep(1)
even with the retry code in place so I would expect it to hit it at
100+msleep(1) for sure. (this time I left my machine on; one thing to test
at a time, I was trying to test suspend2 reliability by suspending it as
many times as I had the reason for). My uguru (its an AN8 SLI) is really bad
I think. Sometimes it goes hours without the message, while at other times
it comes back within an half hour. But the frequency is low now (with
250+msleep(1)) and I am already glad and thankful for that....:)

I think this is an ideal candidate for parameter because it varies from
board to board so much. That will make me (hopefully some others) really
happy. Rest is upto you.

-Sunil


On 7/27/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
>
>
>
> Sunil Kumar wrote:
> > actually the reason I am insisting on doing more than one sleep is that
> one
> > sleep actually didn't work for me. It worked for four days (the uptime
> was
> > four days but 'on' time was probably one day cumulative because those
> were
> > working days and I was suspending the system down at night and in the
> > morning before going to work) and I assumed that it was fine but then it
> > popped its head again in the /var/log/messages. My cpu concern is
> already
> > addressed by making the TIMEOUT as 100 as you suggested.
> >
> > I think the middle ground will be to make this a configuration parameter
> > for
> > the driver because this sleep is going to vary from board to board e.g.
> 100
> > works for you and sometimes 100+msleep(1) doesn't work for me. The
> default
> > could be 1 and max could be 50, and these are taken out of the TIMEOUT
> i.e.
> > if value is set to 50, the loop executes for 50 and then starts to sleep
> > for
> > the rest 50.
> >
> > A runtime parameter would be better but I am fine with a config
> parameter
> > too. does that work?
> >
>
> Hmm,
>
> I really don't want to add all kinda knobs a user needs to tune for
> things to work. Did you see this message return despite of the sleep
> with my latest version of the driver? Because with my new driver you
> shouldn't see it as its treated as retryable (which means the driver
> will silently return the old values and try again next update, it will
> only start to scream on 3 (define) or more consecutive errors.)
>
> Noticed you also decreased the ABIT_UGURU_READY_TIMEOUT from 50 to 5,
> maybe that is the problem now. I agree 50 is to much, but maybe 5 isn't
> enough? What was the exact error you saw?
>
> Since the sleep is in what is in essence an error handling path, Its
> fine by me to sleep say 3 times before giving up, it should be very rare
> that the uguru then still isn't responding. Together with making the
> wait for read statsu retryable and thus in this rare occasion returning
> oldvalues to userspace instead of an error I think we should be fine.
> Does this sound like a plan?
>
> I'm glad atleast we agree on the CPU-usage being fixed by setting the
> default TIMEOUT to 100.
>
> Regards,
>
> Hans
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060727/88445e03/attachment.html 

[lm-sensors] new abituguru driver in mm kernel

[Hans de Goede]

Sunil Kumar wrote:
> but how do you account for the HZ. 

I don't, the system you've been running on has a HZ of 1000 I assume? 
That means that they delays we have found are them minimal ones needed 
to mkae things work, sleeping longer with lower HZ is unfortunate but 
not harmfull. Lower HZ has been taken into account in that we try not to 
sleep much, because otherwise delays for the calling up would become 
unacceptable. But besides that I do not take HZ into account. Remember 
we are dealing with an error / exception path here. It doesn't have to 
be beautifull or very efficient it just has to work and not suck.

 > I will give the 1:3 a run.
 >
Thanks, In combination with a TIMEOUT of 100 I assume?

Regards,

Hans




With msleep(1), one system will sleep
> for 20ms while other will sleep only 2ms for the last try. We need to 
> either 1. make it msleep(20), so all systems sleep for 20ms per read OR 
> 2. have a conditional based on HZ to do msleep(1) for 100 and do 
> multiple msleep(1) for HZ 1000 OR 3. have it as a configured parameter.  
> 1st option means that 1000HZ folks will suffer delays which they could 
> have avoided because they can sleep finer, but without option 2, they 
> will just sleep for 2ms which may not be sufficient. 2nd and 3rd options 
> work but 3rd works better because its simple and makes a lot of sense 
> for the variety of hardware and BIOSes that you could be dealing with.
> 
> 

[lm-sensors] new abituguru driver in mm kernel

[Sunil Kumar]

Has anybody used this driver on x86_64?

Any unexpected surprises?

-Sunil


On 7/31/06, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
>
> Sunil Kumar wrote:
> > but how do you account for the HZ.
>
> I don't, the system you've been running on has a HZ of 1000 I assume?
> That means that they delays we have found are them minimal ones needed
> to mkae things work, sleeping longer with lower HZ is unfortunate but
> not harmfull. Lower HZ has been taken into account in that we try not to
> sleep much, because otherwise delays for the calling up would become
> unacceptable. But besides that I do not take HZ into account. Remember
> we are dealing with an error / exception path here. It doesn't have to
> be beautifull or very efficient it just has to work and not suck.
>
> > I will give the 1:3 a run.
> >
> Thanks, In combination with a TIMEOUT of 100 I assume?
>
> Regards,
>
> Hans
>
>
>
>
> With msleep(1), one system will sleep
> > for 20ms while other will sleep only 2ms for the last try. We need to
> > either 1. make it msleep(20), so all systems sleep for 20ms per read OR
> > 2. have a conditional based on HZ to do msleep(1) for 100 and do
> > multiple msleep(1) for HZ 1000 OR 3. have it as a configured parameter.
> > 1st option means that 1000HZ folks will suffer delays which they could
> > have avoided because they can sleep finer, but without option 2, they
> > will just sleep for 2ms which may not be sufficient. 2nd and 3rd options
> > work but 3rd works better because its simple and makes a lot of sense
> > for the variety of hardware and BIOSes that you could be dealing with.
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060825/77e43b2b/attachment-0001.html