On Mon, Mar 13, 2023 at 02:47:45PM +0200, Matti Vaittinen wrote:
> On 3/6/23 14:52, Andy Shevchenko wrote:
> > On Mon, Mar 06, 2023 at 11:17:15AM +0200, Matti Vaittinen wrote:
...
> > > +/*
> >
> > If it's deliberately not a kernel doc, why to bother to have it looking as one?
> > It's really a provocative to some people who will come with a patches to "fix"
> > this...
>
> I just liked the kernel-doc format. It's a standard way of explaining the
> parameters and returned value. Function however is intended to be internal
> and thus I don't see a need to make this "official kernel doc".
The problem as I pointed out with your approach it's unmaintainable. And
I even explained why I consider it this way.
> > > + * iio_gts_get_gain - Convert scale to total gain
> > > + *
> > > + * Internal helper for converting scale to total gain.
> > > + *
> > > + * @max: Maximum linearized scale. As an example, when scale is created
> > > + * in 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 scale. -EINVAL if scale
> > > + * is invalid.
> > > + */
...
> > > + if (scaler > NANO || !scaler)
> > > + return -EINVAL;
> >
> > Shouldn't be OVERFLOW for the first one?
>
> -EOVERFLOW? I guess it could be. Thanks.
Yes.
...
> > > + sort(gains[i], gts->num_hwgain, sizeof(int), iio_gts_gain_cmp,
> > > + NULL);
> >
> > One line reads better?
>
> I try mostly to keep the good old 80 chars as I often have 3 terminal
> windows fitted on my laptop screen. It works best with the short lines.
With it on one line
sort(gains[i], gts->num_hwgain, sizeof(int), iio_gts_gain_cmp, NULL);
You have N at the last column which quite likely suggests that it's NULL.
So, I don't think it's a big issue to put on a single line.
...
> > > + if (ret && gts->avail_all_scales_table)
> >
> > In one case you commented that free(NULL) is okay, in the other, you add
> > a duplicative check. Why?
>
> Sorry but what do you mean by dublicative check?
>
> Usually I avoid the kfree(NULL). That's why I commented on it in that
> another case where it was not explicitly disallowed. I'll change that for v4
> to avoid kfree(NULL) as you suggested.
So, and with it you put now a double check for NULL, do you think it's okay?
I don't.
> > > + kfree(gts->avail_all_scales_table);
...
> > > + per_time_gains = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
> >
> > sizeof(type) is error prone in comparison to sizeof(*var).
>
> Yes and no. In majority of cases where we see sizeof(*var) - the *var is no
> longer a pointer as having pointers to pointers is not _that_ common. When
> we see sizeof(type *) - we instantly know it is a size of a pointer and not
> a size of some other type.
>
> So yes, while having sizeof(*var) makes us tolerant to errors caused by
> variable type changes - it makes us prone to human reader errors. Also, if
> someone changes type of *var from pointer to some other type - then he/she
> is likely to in any case need to revise the array alloactions too.
>
> While I in general agree with you that the sizeof(variable) is better than
> sizeof(type) - I see that in cases like this the sizeof(type *) is clearer.
Still get a fundamental disagreement on this. I would insist, but I'm not
a maintainer, so you are lucky :-) if Jonathan will not force you to follow
my way.
...
> > > + per_time_scales = kcalloc(gts->num_itime, sizeof(int *), GFP_KERNEL);
Ditto.
...
> > > +err_free_out:
> > > + while (i) {
> > > + /*
> > > + * It does not matter if i'th alloc was not succesfull as
> > > + * kfree(NULL) is safe.
> > > + */
> >
> > Instead, can be just a free of the known allocated i:th member first followed
> > by traditional pattern. In that case comment will become redundant.
> >
>
> I replied to this in your comments regarding the v2. Sorry for splitting the
> discussion by sending v3 so soon.
You can have repeated it here :-)
Yes, two labels and one kfree() makes the comment go away. To me that comment
is more confusing than helping.
> > > + kfree(per_time_scales[i]);
> > > + kfree(per_time_gains[i]);
> > > +
> > > + i--;
> > > + }
...
> > > + for (i = gts->num_itime - 1; i >= 0; i--) {
> >
> > while (i--) {
> >
> > makes it easier to parse.
>
> This is also something I replied for v2. I think we have a fundamental
> disagreement on this one :/
Yes, and I will continue insisting on while (foo--).
That why I won't give you my tags :-)
...
> > > + if (!min)
> > > + min = gts->hwgain_table[i].gain;
> > > + else
> > > + min = min(min, gts->hwgain_table[i].gain);
> >
> > I was staring at this and have got no clue why it's not a dead code.
>
> Nor can I. It seems obvious to me that the one who wrote this had no idea
> what he was doing XD
>
> Well, I must have had some initial idea of using the minimum value to
> something - but I can't remember what would've been the use. Maybe I was
> initially thinking that I'll return the smallest value in-range if the gain
> given as a parameter was smaller than any of the supported ones.
>
> Thank you for reading this carefully and pointing it out! Well spotted!
Hint: run always `make W=1` when building kernel.
It will show defined but not used cases and combined with nowadays
default -Werror won't be compilable.
--
With Best Regards,
Andy Shevchenko
