Re: [PATCH v3 2/6] iio: light: Add gain-time-scale helpers

[Matti Vaittinen]

On 3/13/23 15:25, Andy Shevchenko wrote:
> 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.

Yes. You told me that it asks for people to turn it to kernel doc. If 
that happens, apply the patch and it is kernel doc. I don't see how 
unmaintainable it is. I think this is just creating a problem we may 
never face - and if we do, we can solve it by applying the 'problem' then.

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

Trusting suggestions like this in a kernel code would be a big problem 
to me. I would ask myself - "do you feel lucky"?

Well, my favourite editor would wrap the line - so I would see the NULL 
at the next row. Not indented properly causing it to be harder to read 
than the code which is properly manually split and indented. It is much 
less of a problem for me to "waste" a row here and see the line properly 
split.

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

I don't see the double check. I see only one check just above the 
kfree()? Where is the other check?

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

In a code you are maintaining it is good to have it in your way as 
you're responsible for it. This is also why I insist on having things in 
a way I can read best for a code I plan to maintain - unless the 
subsystem maintainers see it hard to maintain for them. So, let's see if 
Jonathan has strong opinions on this one :)

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

Well, I am planning to keep reading this code when/if it is being 
patched. Hence I am so reluctant to change it to something that makes it 
harder for me to follow. Meanwhile, I understand that you don't want to 
tag something you don't agree with.

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

Ah. I thought I had that and sparse enabled. It seems I disabled all 
extra checks from my build scripts a while ago to speed-up compilation 
when I was bisecting...

> It will show defined but not used cases and combined with nowadays
> default -Werror won't be compilable.

Thanks for the review.

--Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~