Re: aibs(4): ASUSTeK AI Booster (ACPI ATK0110) hardware monitor with limit support

[Constantine Aleksandrovich Murenin <C++%Cns.SU@localhost>]

2009/12/30 Jukka Ruohonen <jruohonen%iki.fi@localhost>:
> On Wed, Dec 30, 2009 at 03:49:35AM -0500, Constantine Aleksandrovich Murenin 
> wrote:
>>
>> Attached patch provides support for the hardware monitoring capabilities
>> that are present in many modern desktop motherboards from ASUS featuring
>> the ATK0110 ACPI device.
>
> Hi. Few small comments, all of them more or less trivial.
>
>> +struct aibs_sensor {
>> +     envsys_data_t   s;
>> +     int64_t         i;
>> +     int64_t         l;
>> +     int64_t         h;
>> +};
>
> The correct type for these would probably be uint64_t, given the relation to
> the type ACPI_INTEGER, which AFAIR is an unsigned 64-bit integer (from ACPI
> 2.0 onwards). Or maybe even plain ACPI_INTEGER?

Yes, this sounds good -- I'll try changing all of these to ACPI_INTEGER.


>> +     case ENVSYS_SVOLTS_DC:
>> +             name[0] = 'V';
>> +             break;
>> +     default:
>> +             return;
>> +     }
>> +
>> +     b.Length = ACPI_ALLOCATE_BUFFER;
>
> This could be ACPI_ALLOCATE_LOCAL_BUFFER.

ACPI_ALLOCATE_LOCAL_BUFFER is not documented in the latest
http://www.acpica.org/download/acpica-reference.pdf (Revision 1.26,
from October 2009).  Is there any principal difference between the
two?


>> +     s = AcpiEvaluateObjectTyped(sc->sc_node->ad_handle, name, NULL, &b,
>> +         ACPI_TYPE_PACKAGE);
>> +     if (ACPI_FAILURE(s)) {
>> +             aprint_error_dev(self, "%s not found\n", name);
>> +             return;
>> +     }
>
> This is just a personal preference, but I would use acpi_eval_struct() and
> verify the type manually, given that AcpiEvaluateObjectTyped() is just a
> wrapper around AcpiEvaluateObject(). Maybe we should have something like
> acpi_eval_struct_typed() though.
>
>> +     bp = b.Pointer;
>> +     o = bp->Package.Elements;
>> +     if (o[0].Type != ACPI_TYPE_INTEGER) {
>> +             aprint_error_dev(self, "%s[0]: invalid type\n", name);
>> +             AcpiOsFree(b.Pointer);
>
> Use the ACPI_FREE macro instead of calling AcpiOsFree() directly.

ACPICA Reference, Section 3.2.2.2, "ACPI Allocates Return Buffers",
explicitly suggests the use of AcpiOsFree().  Is it wrong?


>> +     as = malloc(sizeof(*as) * n, M_DEVBUF, M_NOWAIT | M_ZERO);
>
> I reckon that there is no reason to prefer malloc(9) instead of kmem(9).

I didn't use kmem(9) since that would require knowing the size of the
structure on kmem_free(9).


>> +     for (i = 0, o++; i < n; i++, o++) {
>
> Would it be possible to use the utility function acpi_foreach_package_object()
> here? It could clarify the loop, which I think is more readable in the
> original aiboost(4).

Actually, the original aiboost(4) also uses pointer arithmetics in a
for loop similar to mine. :-)

Thanks for pointing out this function, I'll take a further look if
it'll be more useful.  I'd prefer the driver to be as cross-portable
as possible, though, and acpica functions seem like a natural choice
for such a direction.


>> +             as[i].i = oi[0].Integer.Value;
>> +             strlcpy(as[i].s.desc, oi[1].String.Pointer,
>> +                 sizeof(as[i].s.desc));
>
> Is there a guarantee that "oi[1].String.Pointer" is not NULL?

I'd certainly expect so, since NULL is not a string, and oi[1].Type is
ACPI_TYPE_STRING.


>> +             aprint_verbose_dev(self, "%c%i: "
>> +                 "0x%08llx %20s %5lli / %5lli  0x%llx\n",
>> +                 name[0], i,
>> +                 as[i].i, as[i].s.desc, as[i].l, as[i].h,
>> +                 oi[4].Integer.Value);
>
> These should probably be %"PRId64" (or %"PRIx64" with uint64_t).

Sure, looks good...  Actually, what's the purpose of this?  Is
sizeof(unsigned long long) greater than sizeof(uint64_t) on amd64?  If
not, then why do you have to change llx to lx?  :/


>> +             sysmon_envsys_sensor_attach(sc->sc_sme, &as[i].s);
>> +     }
>
> The return value from sysmon_envsys_attach() could be validated, as there is
> no proof what is in "oi[1].String.Pointer" (i.e. could it be empty string or
> duplicate or something similar due buggy firmware?).

Yes, sure, overlooked that during the porting...


>> +static int
>> +aibs_detach(device_t self, int flags)
>> +{
>> +     struct aibs_softc       *sc = device_private(self);
>> +
>> +     sysmon_envsys_unregister(sc->sc_sme);
>> +     if (sc->sc_asens_volt != NULL)
>> +             free(sc->sc_asens_volt, M_DEVBUF);
>> +     if (sc->sc_asens_temp != NULL)
>> +             free(sc->sc_asens_temp, M_DEVBUF);
>> +     if (sc->sc_asens_fan != NULL)
>> +             free(sc->sc_asens_fan, M_DEVBUF);
>> +     return 0;
>> +}
>
> I wonder if the detach routine is needed at all?

As David pointed out, for drvctl -d. :-)

C.