| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZAC7L8NQYgBcBTCF@smile.fi.intel.com>
Date: Thu, 2 Mar 2023 17:05:19 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Matti Vaittinen <mazziesaccount@...il.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
Jonathan Cameron <jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Paul Gazzillo <paul@...zz.com>,
Dmitry Osipenko <dmitry.osipenko@...labora.com>,
Shreeya Patel <shreeya.patel@...labora.com>,
Zhigang Shi <Zhigang.Shi@...eon.com>,
linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org
Subject: Re: [PATCH v2 2/6] iio: light: Add gain-time-scale helpers
On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote:
> Some light sensors can adjust both the HW-gain and integration time.
> There are cases where adjusting the integration time has similar impact
> to the scale of the reported values as gain setting has.
>
> IIO users do typically expect to handle scale by a single writable 'scale'
> entry. Driver should then adjust the gain/time accordingly.
>
> It however is difficult for a driver to know whether it should change
> gain or integration time to meet the requested scale. Usually it is
> preferred to have longer integration time which usually improves
> accuracy, but there may be use-cases where long measurement times can be
> an issue. Thus it can be preferable to allow also changing the
> integration time - but mitigate the scale impact by also changing the gain
> underneath. Eg, if integration time change doubles the measured values,
> the driver can reduce the HW-gain to half.
>
> The theory of the computations of gain-time-scale is simple. However,
> some people (undersigned) got that implemented wrong for more than once.
>
> Add some gain-time-scale helpers in order to not dublicate errors in all
> drivers needing these computations.
...
> +/*
Is it intentionally _not_ a kernel doc?
> + * iio_gts_get_gain - Convert scale to total gain
> + * Internal helper for converting scale to total gain.
Otherwise this line should go after the fields, I remember kernel doc warnings
on the similar cases.
> + * @max: Maximum linearized scale. As an example, when scale is creted in
creted?
IIRC I already pointed out to the very same mistake in your code in the past
(sorry, if my memory doesn't serve me well).
> + * magnitude of NANOs and max scale is 64.1 - The linearized
> + * scale is 64 100 000 000.
> + * @scale: Linearized scale to compte the gain for.
> + *
> + * Return: (floored) gain corresponding to the scales. -EINVAL if scale
scales? (Plural?)
> + * is invalid.
> + */
Same remark to all of the comments.
> +{
> + int tmp = 1;
> +
> + if (scale > max || !scale)
> + return -EINVAL;
> +
> + if (U64_MAX - max < scale) {
> + /* Risk of overflow */
> + if (max - scale < scale)
> + return 1;
> + while (max - scale > scale * (u64) tmp)
Space is not required after casting.
> + tmp++;
> +
> + return tmp + 1;
Wondering why you can't simplify this to
max -= scale;
tmp++;
> + }
> +
> + while (max > scale * (u64) tmp)
> + tmp++;
> +
> + return tmp;
> +}
Missing blank line.
> +/*
> + * gain_get_scale_fraction - get the gain or time based on scale and known one
> + *
> + * Internal helper for computing unknown fraction of total gain.
> + * Compute either gain or time based on scale and either the gain or time
> + * depending on which one is known.
> + *
> + * @max: Maximum linearized scale. As an example, when scale is creted in
creted?
Is it mistakenly stored in your spellcheck database? Or is it simply
copy'n'paste typo?
> + * magnitude of NANOs and max scale is 64.1 - The linearized
> + * scale is 64 100 000 000.
> + * @scale: Linearized scale to compute the gain/time for.
> + * @known: Either integration time or gain depending on which one is known
> + * @unknown: Pointer to variable where the computed gain/time is stored
> + *
> + * Return: 0 on success
> + */
...
> +static const struct iio_itime_sel_mul *
> + iio_gts_find_itime_by_time(struct iio_gts *gts, int time)
Strange indentation.
Ditto for all these types of cases.
...
> + *lin_scale = (u64) scale_whole * (u64)scaler + (u64)(scale_nano
> + / (NANO / scaler));
Strange indentation. Split on the logical (math) parts better.
...
> +EXPORT_SYMBOL_GPL(iio_init_iio_gts);
I haven't noticed if you put these all exports into a proper namespace.
If no, please do.
...
> + sort(gains[i], gts->num_hwgain, sizeof(int), iio_gts_gain_cmp,
> + NULL);
One line is okay.
...
> + all_gains = kcalloc(gts->num_itime * gts->num_hwgain, sizeof(int),
Something from overflow.h is very suitable here.
> + GFP_KERNEL);
> + if (!all_gains)
> + return -ENOMEM;
...
> + memcpy(all_gains, gains[gts->num_itime - 1], gts->num_hwgain * sizeof(int));
Maybe it's better to have a temporary which will be calculated as array_size()
for allocation and reused here?
...
> + for (i = gts->num_itime - 2; i >= 0; i--)
Yeah, if you put this into temporary, like
i = gts->num_itime - 1;
this becomes
while (i--) {
Note, you missed {} for better reading.
Note, you may re-use that i (maybe renamed to something better in the memcpy()
above as well).
> + for (j = 0; j < gts->num_hwgain; j++) {
> + int candidate = gains[i][j];
> + int chk;
> +
> + if (candidate > all_gains[new_idx - 1]) {
> + all_gains[new_idx] = candidate;
> + new_idx++;
> +
> + continue;
> + }
> + for (chk = 0; chk < new_idx; chk++)
> + if (candidate <= all_gains[chk])
> + break;
> +
> + if (candidate == all_gains[chk])
> + continue;
> +
> + memmove(&all_gains[chk + 1], &all_gains[chk],
> + (new_idx - chk) * sizeof(int));
> + all_gains[chk] = candidate;
> + new_idx++;
> + }
...
> + gts->avail_all_scales_table = kcalloc(gts->num_avail_all_scales,
> + 2 * sizeof(int), GFP_KERNEL);
> + if (!gts->avail_all_scales_table)
> + ret = -ENOMEM;
> + else
> + for (i = 0; !ret && i < gts->num_avail_all_scales; i++)
Much easier to read if you move this...
> + ret = iio_gts_total_gain_to_scale(gts, all_gains[i],
> + >s->avail_all_scales_table[i * 2],
> + >s->avail_all_scales_table[i * 2 + 1]);
...here as
if (ret)
break;
> + kfree(all_gains);
> + if (ret && gts->avail_all_scales_table)
> + kfree(gts->avail_all_scales_table);
> +
> + return ret;
But Wouldn't be better to use goto labels?
...
> + while (i) {
Instead of doing standard
while (i--) {
> + /*
> + * It does not matter if i'th alloc was not succesfull as
> + * kfree(NULL) is safe.
> + */
You add this comment, ...
> + kfree(per_time_gains[i]);
> + kfree(per_time_scales[i]);
...an additional loop, ...
> +
> + i--;
...and a line of code.
> + }
Why?
> + for (i = gts->num_itime - 1; i >= 0; i--) {
i = gts->num_itime;
while (i--) {
> + int new = gts->itime_table[i].time_us;
> +
> + if (times[idx] < new) {
> + times[idx++] = new;
> + continue;
> + }
> +
> + for (j = 0; j <= idx; j++) {
> + if (times[j] > new) {
> + memmove(×[j + 1], ×[j], (idx - j) * sizeof(int));
> + times[j] = new;
> + idx++;
> + }
> + }
> + }
...
> +void iio_gts_purge_avail_time_table(struct iio_gts *gts)
> +{
> + if (gts->num_avail_time_tables) {
if (!...)
return;
> + kfree(gts->avail_time_tables);
> + gts->avail_time_tables = NULL;
> + gts->num_avail_time_tables = 0;
> + }
> +}
...
> + if (!diff) {
Why not positive conditional?
if (diff) {
...
} else {
...
}
> + diff = gain - gts->hwgain_table[i].gain;
> + best = i;
> + } else {
> + int tmp = gain - gts->hwgain_table[i].gain;
> +
> + if (tmp < diff) {
> + diff = tmp;
> + best = i;
> + }
> + }
...
> + ret = gain_get_scale_fraction(gts->max_scale, scale_linear, mul, gain);
> +
Redundant blank line.
> + if (ret || !iio_gts_valid_gain(gts, *gain))
Why error code is shadowed?
> + return -EINVAL;
> +
...
> + ret = iio_gts_get_scale_linear(gts, old_gain, itime_old->time_us,
> + &scale);
Single line if fine.
> + if (ret)
> + return ret;
> +
> + ret = gain_get_scale_fraction(gts->max_scale, scale, itime_new->mul,
> + new_gain);
Ditto.
> + if (ret)
> + return -EINVAL;
...
> +#ifndef __GAIN_TIME_SCALE_HELPER__
> +#define __GAIN_TIME_SCALE_HELPER__
__IIO_... ?
Missing types.h (at least, haven't checked for more).
Missing some forward declarations, at least for struct device.
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists