On Mon, Sep 15, 2014 at 04:57:24PM +0200, S??bastien Marie wrote:
> ping ?
>
> Tihs patch is very conservative: it just allow to switch fan OFF if
> state is unknown.
I finally read through the relevant parts of the spec. The problem
here is that it looks like we are implementing thermal zone hysteresis
incorrectly. I think you noticed this in your original diff, but IMO
we've gone down a bit of a wrong path here with these later diffs.
I'll take a look at the hysteresis code tomorrow and see if I can come
up with something better.
Thanks.
-ml
PS the relevant part of the spec is 11.1.2.3 "Resetting Cooling Temperatures
to implement Hysteresis"
>
> Thanks.
> --
> S??bastien Marie
>
> On Wed, Aug 27, 2014 at 02:51:20PM +0200, S??bastien Marie wrote:
> > Hi Jonathan,
> >
> > First, thanks for your feedback and for your patch.
> >
> > On Wed, Aug 27, 2014 at 02:42:43AM +1000, Jonathan Gray wrote:
> > > Rather than calling acpi_setfan() again would it make
> > > sense to remove the cached state value and move
> > > the state reading to just before the method is called?
> > >
> > > That said I'm not familiar with the acpitz code and
> > > haven't gone off to look at the spec.
> >
> > At first reading I am quite disappointed to remove caching.
> >
> > So I have:
> > - checked in others acpi termal-zone implementation to see if caching
> > is using or not for active cooling.
> > - try to measure some values on my host.
> >
> >
> > Others implementations checked:
> > - netbsd : caching used (sys/dev/acpi/acpi_tz.c, l.368)
> > - freebsd : caching used (head/sys/dev/acpica/acpi_thermal.c, l.564)
> > - linux : caching seems not be used (but not 100% sure)
> >
> > (please note I am not expert in theses kernels implementations, so I
> > could be wrong).
> >
> >
> > Some measures:
> >
> > I first try to measure the cost of acpi_setfan in term of time:
> > something between 9998432ns et 14574920ns (0.0099 and 0.015 s). It
> > seems not being a heavy operation (too my eyes).
> >
> >
> > Secondly, the number of call of acpi_setfan, with and without caching.
> >
> > The test kernel is build with caching enable. The without-caching
> > counter is incremented every time acpi_setfan would be call without
> > caching, and with-caching counter incremented only when acpi_setfan is
> > called (the patch below was applied: an unknown state would result
> > acpi_setfan calling).
> >
> > During 1h09 :
> > - without caching: 3388 calls
> > - with caching: 742 calls
> >
> > The cache is used at 78% of time.
> >
> >
> > So, even if your patch resolve the initial problem (the fan don't keep
> > running whereas state is unknown), I would prefer to keep the cache in
> > place. But if you think the impact of not use this cache is negligible,
> > it is ok for me.
> >
> > I join a very conservative patch which just allow calling
> > acpi_setfan(sc,i,"_OFF") if cache is unknown.
> >
> > Thanks.
> > --
> > S??bastien Marie
> >
> >
> > Index: dev/acpi/acpitz.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/acpi/acpitz.c,v
> > retrieving revision 1.47
> > diff -u -p -r1.47 acpitz.c
> > --- dev/acpi/acpitz.c 12 Jul 2014 02:44:49 -0000 1.47
> > +++ dev/acpi/acpitz.c 27 Aug 2014 12:38:36 -0000
> > @@ -424,7 +424,7 @@ acpitz_refresh(void *arg)
> > acpitz_setfan(sc, i, "_ON_");
> > } else if (sc->sc_ac[i] != -1) {
> > /* turn off fan i */
> > - if (sc->sc_ac_stat[i] > 0)
> > + if (sc->sc_ac_stat[i] == -1 || sc->sc_ac_stat[i] > 0)
> > acpitz_setfan(sc, i, "_OFF");
> > }
> > }
>
