Re: [PATCH] iio: Fix the sorting functionality in iio_gts_build_avail_time_table

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 15 Mar 2024 15:49:10 -0500
Chenyuan Yang <chenyuan0y@gmail.com> wrote:

> Hi Matti,
> 
> Thanks for your reply!
> 
> > I think the suggested-by tag is a bit of an overkill :) I don't feel
> > like taking the credit - you spotted the problem and fixed it!  
> 
> You did help me figure out the real issue here and how to fix it :)
> 
> > Do you think you could fix the removal of the duplicates too?  
> 
> Sure, I can help to implement the deduplication logic.
> Here is a potential patch for it based on your help.
> Besides, I changed the stop condition in the inner loop to `j < idx`
> since the current last index should be `idx - 1`.

Matti, I didn't follow why duplicates are a problem?

Sure the code is less efficient, but do we end up with a wrong
answer as a result (I've not checked logic, but I'd expect either
first or last of repeating values to be used depending on the alg).

I'm not convinced adding to complexity of the code is worthwhile if
its not a functional problem.  I would expect a unit test to verify
that duplicates aren't a problem though (given you have unit tests!)

Jonathan

> ---
> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
> index 7653261d2dc2..32f0635ffc18 100644
> --- a/drivers/iio/industrialio-gts-helper.c
> +++ b/drivers/iio/industrialio-gts-helper.c
> @@ -375,17 +375,20 @@ static int iio_gts_build_avail_time_table(struct iio_gts *gts)
>  	for (i = gts->num_itime - 1; i >= 0; i--) {
>  		int new = gts->itime_table[i].time_us;
>  
> -		if (times[idx] < new) {
> +		if (idx == 0 || times[idx - 1] < new) {
>  			times[idx++] = new;
>  			continue;
>  		}
>  
> -		for (j = 0; j <= idx; j++) {
> +		for (j = 0; j < idx; j++) {
> +			if (times[j] == new)
> +				break;
>  			if (times[j] > new) {
>  				memmove(&times[j + 1], &times[j],
>  					(idx - j) * sizeof(int));
>  				times[j] = new;
>  				idx++;
> +				break;
>  			}
>  		}
>  	}
>