Re: CVS commit: src

[Alexander Nasonov <alnsn%yandex.ru@localhost>]

Jukka Ruohonen wrote:
> +If the backend operates with a simple boolean switch
> +without knowing the clock frequencies, the
> +.Fa cfs_freq
> +field should be set to
> +.Dv CPUFREQ_STATE_ENABLED
> +or
> +.Dv CPUFREQ_STATE_DISABLED .

Elements must go in descendant order but since values of
CPUFREQ_STATE_DISABLED and CPUFREQ_STATE_ENABLED are unspecified,
the manual should explicitly state in which order these two states
should go.


> +     /*
> +      * Sanity check the values and verify descending order.
> +      */
> +     for (count = i = 0; i < cf->cf_state_count; i++) {
> +
> +             CTASSERT(CPUFREQ_STATE_ENABLED != 0);
> +             CTASSERT(CPUFREQ_STATE_DISABLED != 0);
> +
> +             if (cf->cf_state[i].cfs_freq == 0)
> +                     continue;

Every time you skip cf->cf_state element, you have a gap in the
corresponding cf_state element in cf_backend. It's zeroed (because
you initialized cf_backend with kmem_zalloc) but I doubt is was
your intention.

The documentation is clear about cf_state elements, why do you need
to skip entries at all and make your life harder? Just return error
or add KASSERT if a caller doesn't follow the docs.

> +             for (j = k = 0; j < i; j++) {
> +
> +                     if (cf->cf_state[i].cfs_freq >=
> +                         cf->cf_state[j].cfs_freq) {
> +                             k = 1;
> +                             break;
> +                     }
> +             }

If you didn't skip elements, you could check only the previous element
rather than doing it in a loop

> +             if (k != 0)
> +                     continue;

... and return error rather than skipping an out-of-order element.

> +             cf_backend->cf_state[i].cfs_index = count;
> +             cf_backend->cf_state[i].cfs_freq = cf->cf_state[i].cfs_freq;
> +             cf_backend->cf_state[i].cfs_power = cf->cf_state[i].cfs_power;
                                     ^
Alternatively, you can change i here ^ to count to avoid gaps.

Hmm, cf_index assignment above makes me wonder that may be gaps are on
purpose but not documented?

Alex