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

[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?

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

This could be ACPI_ALLOCATE_LOCAL_BUFFER.

> +     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.

> +     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).

> +     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).

> +             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?

> +             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).

> +             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?).

> +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?

- Jukka.