On Thu, 28 Nov 2024 18:50:24 +0200 Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote: > The error path in the gain_to_scaletables() uses goto for unwinding an > allocation on failure. This can be slightly simplified by using the > automated free when exiting the scope. > > Use __free(kfree) and drop the goto based error handling. > > Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx> > --- > This is derived from the: > https://lore.kernel.org/lkml/5efc30d832275778d1f48d7e2c75b1ecc63511d5.1732105157.git.mazziesaccount@xxxxxxxxx/ Hi Matti A few comments on specific parts of this below Thanks, Jonathan > +static int build_combined_table(struct iio_gts *gts, int **gains, size_t gain_bytes) > +{ > + int ret, i, j, new_idx, time_idx; > + int *all_gains __free(kfree) = kcalloc(gts->num_itime, gain_bytes, > + GFP_KERNEL); > + > if (!all_gains) > return -ENOMEM; > > @@ -232,10 +238,9 @@ static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales) > > gts->avail_all_scales_table = kcalloc(new_idx, 2 * sizeof(int), > GFP_KERNEL); I'm not particularly keen in a partial application of __free magic. Perhaps you can use a local variable for this and a no_free_ptr() to assign it after we know there can't be an error that requires it to be freed. > - if (!gts->avail_all_scales_table) { > - ret = -ENOMEM; > - goto free_out; > - } > + if (!gts->avail_all_scales_table) > + return -ENOMEM; > + > gts->num_avail_all_scales = new_idx; > > for (i = 0; i < gts->num_avail_all_scales; i++) { > @@ -246,14 +251,25 @@ static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales) > if (ret) { > kfree(gts->avail_all_scales_table); > gts->num_avail_all_scales = 0; > - goto free_out; > + return ret; > } > } > > -free_out: > - kfree(all_gains); > + return 0; > +} > > - return ret; > +static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales) > +{ > + int ret; > + size_t gain_bytes; > + > + ret = fill_and_sort_scaletables(gts, gains, scales); > + if (ret) > + return ret; > + > + gain_bytes = array_size(gts->num_hwgain, sizeof(int)); array_size is documented as being for 2D arrays, not an array of a multi byte type. We should not use it for this purpose. I'd be tempted to not worry about overflow, but if you do want to be sure then copy what kcalloc does and use a check_mul_overflow() https://elixir.bootlin.com/linux/v6.12.1/source/include/linux/slab.h#L919 You don't have to tidy that up in this patch though. > + > + return build_combined_table(gts, gains, gain_bytes); > } > > +/** > + * iio_gts_build_avail_time_table - build table of available integration times > + * @gts: Gain time scale descriptor > + * > + * Build the table which can represent the available times to be returned > + * to users using the read_avail-callback. > + * > + * NOTE: Space allocated for the tables must be freed using > + * iio_gts_purge_avail_time_table() when the tables are no longer needed. > + * > + * Return: 0 on success. > + */ > +static int iio_gts_build_avail_time_table(struct iio_gts *gts) Hmm. I guess this wrapper exists because perhaps you aren't comfortable yet with the __free() handling mid function. It does not harm so I'm fine having this. > +{ > + if (!gts->num_itime) > + return 0; > + > + return __iio_gts_build_avail_time_table(gts); > +} > + > /** > * iio_gts_purge_avail_time_table - free-up the available integration time table > * @gts: Gain time scale descriptor
