drivers/iio/adc/meson_saradc.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
meson_sar_adc_temp_sensor_init() allocates a buffer with
nvmem_cell_read(), but the old code leaked it if
syscon_regmap_lookup_by_phandle() failed.
Switch buf to __free(kfree) so all return paths release it.
Fixes: d6f2eac64403 ("iio: adc: meson: no devm for nvmem_cell_get")
Signed-off-by: Felix Gu <ustc.gu@gmail.com>
---
drivers/iio/adc/meson_saradc.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
index 23991a3612bd..9708ddcc4919 100644
--- a/drivers/iio/adc/meson_saradc.c
+++ b/drivers/iio/adc/meson_saradc.c
@@ -786,7 +786,7 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev,
static int meson_sar_adc_temp_sensor_init(struct iio_dev *indio_dev)
{
struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
- u8 *buf, trimming_bits, trimming_mask, upper_adc_val;
+ u8 trimming_bits, trimming_mask, upper_adc_val;
struct device *dev = indio_dev->dev.parent;
struct nvmem_cell *temperature_calib;
size_t read_len;
@@ -807,14 +807,12 @@ static int meson_sar_adc_temp_sensor_init(struct iio_dev *indio_dev)
}
read_len = MESON_SAR_ADC_EFUSE_BYTES;
- buf = nvmem_cell_read(temperature_calib, &read_len);
+ u8 *buf __free(kfree) = nvmem_cell_read(temperature_calib, &read_len);
nvmem_cell_put(temperature_calib);
if (IS_ERR(buf))
return dev_err_probe(dev, PTR_ERR(buf), "failed to read temperature_calib cell\n");
- if (read_len != MESON_SAR_ADC_EFUSE_BYTES) {
- kfree(buf);
+ if (read_len != MESON_SAR_ADC_EFUSE_BYTES)
return dev_err_probe(dev, -EINVAL, "invalid read size of temperature_calib cell\n");
- }
priv->tsc_regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "amlogic,hhi-sysctrl");
if (IS_ERR(priv->tsc_regmap))
@@ -835,8 +833,6 @@ static int meson_sar_adc_temp_sensor_init(struct iio_dev *indio_dev)
priv->temperature_sensor_adc_val |= upper_adc_val << BITS_PER_BYTE;
priv->temperature_sensor_adc_val >>= trimming_bits;
- kfree(buf);
-
return 0;
}
---
base-commit: 7080e32d3f09d8688c4a87d81bdcc71f7f606b16
change-id: 20260425-meson_saradc-54abc52d9b31
Best regards,
--
Felix Gu <ustc.gu@gmail.com>On Sun, Apr 26, 2026 at 12:07:24AM +0800, Felix Gu wrote: > meson_sar_adc_temp_sensor_init() allocates a buffer with > nvmem_cell_read(), but the old code leaked it if > syscon_regmap_lookup_by_phandle() failed. > Switch buf to __free(kfree) so all return paths release it. __free() is defined in cleanup.h which is missing in the driver. Please add the include to it. -- With Best Regards, Andy Shevchenko
On Sat, Apr 25, 2026 at 9:07 AM Felix Gu <ustc.gu@gmail.com> wrote:
>
> meson_sar_adc_temp_sensor_init() allocates a buffer with
> nvmem_cell_read(), but the old code leaked it if
> syscon_regmap_lookup_by_phandle() failed.
>
> Switch buf to __free(kfree) so all return paths release it.
>
> Fixes: d6f2eac64403 ("iio: adc: meson: no devm for nvmem_cell_get")
> Signed-off-by: Felix Gu <ustc.gu@gmail.com>
> ---
> drivers/iio/adc/meson_saradc.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> index 23991a3612bd..9708ddcc4919 100644
> --- a/drivers/iio/adc/meson_saradc.c
> +++ b/drivers/iio/adc/meson_saradc.c
> @@ -786,7 +786,7 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev,
> static int meson_sar_adc_temp_sensor_init(struct iio_dev *indio_dev)
> {
> struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
> - u8 *buf, trimming_bits, trimming_mask, upper_adc_val;
> + u8 trimming_bits, trimming_mask, upper_adc_val;
> struct device *dev = indio_dev->dev.parent;
> struct nvmem_cell *temperature_calib;
> size_t read_len;
> @@ -807,14 +807,12 @@ static int meson_sar_adc_temp_sensor_init(struct iio_dev *indio_dev)
> }
>
> read_len = MESON_SAR_ADC_EFUSE_BYTES;
> - buf = nvmem_cell_read(temperature_calib, &read_len);
> + u8 *buf __free(kfree) = nvmem_cell_read(temperature_calib, &read_len);
> nvmem_cell_put(temperature_calib);
> if (IS_ERR(buf))
> return dev_err_probe(dev, PTR_ERR(buf), "failed to read temperature_calib cell\n");
> - if (read_len != MESON_SAR_ADC_EFUSE_BYTES) {
> - kfree(buf);
I don't like this. Just add the missing kfree.
> + if (read_len != MESON_SAR_ADC_EFUSE_BYTES)
> return dev_err_probe(dev, -EINVAL, "invalid read size of temperature_calib cell\n");
> - }
>
> priv->tsc_regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "amlogic,hhi-sysctrl");
> if (IS_ERR(priv->tsc_regmap))
> @@ -835,8 +833,6 @@ static int meson_sar_adc_temp_sensor_init(struct iio_dev *indio_dev)
> priv->temperature_sensor_adc_val |= upper_adc_val << BITS_PER_BYTE;
> priv->temperature_sensor_adc_val >>= trimming_bits;
>
> - kfree(buf);
> -
> return 0;
> }
>
>
> ---
> base-commit: 7080e32d3f09d8688c4a87d81bdcc71f7f606b16
> change-id: 20260425-meson_saradc-54abc52d9b31
>
> Best regards,
> --
> Felix Gu <ustc.gu@gmail.com>
>
On Sat, 25 Apr 2026 12:20:55 -0700
Rosen Penev <rosenp@gmail.com> wrote:
> On Sat, Apr 25, 2026 at 9:07 AM Felix Gu <ustc.gu@gmail.com> wrote:
> >
> > meson_sar_adc_temp_sensor_init() allocates a buffer with
> > nvmem_cell_read(), but the old code leaked it if
> > syscon_regmap_lookup_by_phandle() failed.
> >
> > Switch buf to __free(kfree) so all return paths release it.
> >
> > Fixes: d6f2eac64403 ("iio: adc: meson: no devm for nvmem_cell_get")
> > Signed-off-by: Felix Gu <ustc.gu@gmail.com>
> > ---
> > drivers/iio/adc/meson_saradc.c | 10 +++-------
> > 1 file changed, 3 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> > index 23991a3612bd..9708ddcc4919 100644
> > --- a/drivers/iio/adc/meson_saradc.c
> > +++ b/drivers/iio/adc/meson_saradc.c
> > @@ -786,7 +786,7 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev,
> > static int meson_sar_adc_temp_sensor_init(struct iio_dev *indio_dev)
> > {
> > struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
> > - u8 *buf, trimming_bits, trimming_mask, upper_adc_val;
> > + u8 trimming_bits, trimming_mask, upper_adc_val;
> > struct device *dev = indio_dev->dev.parent;
> > struct nvmem_cell *temperature_calib;
> > size_t read_len;
> > @@ -807,14 +807,12 @@ static int meson_sar_adc_temp_sensor_init(struct iio_dev *indio_dev)
> > }
> >
> > read_len = MESON_SAR_ADC_EFUSE_BYTES;
> > - buf = nvmem_cell_read(temperature_calib, &read_len);
> > + u8 *buf __free(kfree) = nvmem_cell_read(temperature_calib, &read_len);
> > nvmem_cell_put(temperature_calib);
> > if (IS_ERR(buf))
> > return dev_err_probe(dev, PTR_ERR(buf), "failed to read temperature_calib cell\n");
> > - if (read_len != MESON_SAR_ADC_EFUSE_BYTES) {
> > - kfree(buf);
> I don't like this. Just add the missing kfree.
That is the minimal fix, so we should probably do that first
even if we then circle back to consider if __free() magic is worth using here.
J
> > + if (read_len != MESON_SAR_ADC_EFUSE_BYTES)
> > return dev_err_probe(dev, -EINVAL, "invalid read size of temperature_calib cell\n");
> > - }
> >
> > priv->tsc_regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "amlogic,hhi-sysctrl");
> > if (IS_ERR(priv->tsc_regmap))
> > @@ -835,8 +833,6 @@ static int meson_sar_adc_temp_sensor_init(struct iio_dev *indio_dev)
> > priv->temperature_sensor_adc_val |= upper_adc_val << BITS_PER_BYTE;
> > priv->temperature_sensor_adc_val >>= trimming_bits;
> >
> > - kfree(buf);
> > -
> > return 0;
> > }
> >
> >
> > ---
> > base-commit: 7080e32d3f09d8688c4a87d81bdcc71f7f606b16
> > change-id: 20260425-meson_saradc-54abc52d9b31
> >
> > Best regards,
> > --
> > Felix Gu <ustc.gu@gmail.com>
> >
On Sun, Apr 26, 2026 at 6:41 PM Jonathan Cameron <jic23@kernel.org> wrote: > > That is the minimal fix, so we should probably do that first > even if we then circle back to consider if __free() magic is worth using here. > > J Hi Jonathan, You prefer a minimal fix here? Best regards, Felix
On Sun, 26 Apr 2026 23:26:33 +0800 Felix Gu <ustc.gu@gmail.com> wrote: > On Sun, Apr 26, 2026 at 6:41 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > That is the minimal fix, so we should probably do that first > > even if we then circle back to consider if __free() magic is worth using here. > > > > J > > Hi Jonathan, > You prefer a minimal fix here? Yes please. Then if you like we can carry on discussion about whether __free() is a good change here as a follow up. That can include the fact that it would have avoided this bug ever being introduced. Thanks, Jonathan > > Best regards, > Felix
On 4/25/26 11:07 AM, Felix Gu wrote:
> meson_sar_adc_temp_sensor_init() allocates a buffer with
> nvmem_cell_read(), but the old code leaked it if
> syscon_regmap_lookup_by_phandle() failed.
>
> Switch buf to __free(kfree) so all return paths release it.
>
> Fixes: d6f2eac64403 ("iio: adc: meson: no devm for nvmem_cell_get")
> Signed-off-by: Felix Gu <ustc.gu@gmail.com>
> ---
> drivers/iio/adc/meson_saradc.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> index 23991a3612bd..9708ddcc4919 100644
> --- a/drivers/iio/adc/meson_saradc.c
> +++ b/drivers/iio/adc/meson_saradc.c
> @@ -786,7 +786,7 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev,
> static int meson_sar_adc_temp_sensor_init(struct iio_dev *indio_dev)
> {
Nice to see one of these cleanup.h patches that is actually fixing a bug.
Should `#include <linux/cleanup.h>` though rather that relying on it being
included through another header.
With that fixed...
Reviewed-by: David Lechner <dlechner@baylibre.com>
On Sat, 25 Apr 2026 11:17:18 -0500
David Lechner <dlechner@baylibre.com> wrote:
> On 4/25/26 11:07 AM, Felix Gu wrote:
> > meson_sar_adc_temp_sensor_init() allocates a buffer with
> > nvmem_cell_read(), but the old code leaked it if
> > syscon_regmap_lookup_by_phandle() failed.
> >
> > Switch buf to __free(kfree) so all return paths release it.
> >
> > Fixes: d6f2eac64403 ("iio: adc: meson: no devm for nvmem_cell_get")
> > Signed-off-by: Felix Gu <ustc.gu@gmail.com>
> > ---
> > drivers/iio/adc/meson_saradc.c | 10 +++-------
> > 1 file changed, 3 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> > index 23991a3612bd..9708ddcc4919 100644
> > --- a/drivers/iio/adc/meson_saradc.c
> > +++ b/drivers/iio/adc/meson_saradc.c
> > @@ -786,7 +786,7 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev,
> > static int meson_sar_adc_temp_sensor_init(struct iio_dev *indio_dev)
> > {
>
> Nice to see one of these cleanup.h patches that is actually fixing a bug.
It's not fixing anything as far as I can see.
The syscon_regmap_lookup_by_handle() is earlier in the function and
nvmem_cell_read() isn't called if that fails.
So this is just a code simplification and so small benefit if anything.
Maybe there is an old version that does things in a different order?
>
> Should `#include <linux/cleanup.h>` though rather that relying on it being
> included through another header.
>
> With that fixed...
>
> Reviewed-by: David Lechner <dlechner@baylibre.com>
>
On 4/25/26 1:44 PM, Jonathan Cameron wrote:
> On Sat, 25 Apr 2026 11:17:18 -0500
> David Lechner <dlechner@baylibre.com> wrote:
>
>> On 4/25/26 11:07 AM, Felix Gu wrote:
>>> meson_sar_adc_temp_sensor_init() allocates a buffer with
>>> nvmem_cell_read(), but the old code leaked it if
>>> syscon_regmap_lookup_by_phandle() failed.
>>>
>>> Switch buf to __free(kfree) so all return paths release it.
>>>
>>> Fixes: d6f2eac64403 ("iio: adc: meson: no devm for nvmem_cell_get")
>>> Signed-off-by: Felix Gu <ustc.gu@gmail.com>
>>> ---
>>> drivers/iio/adc/meson_saradc.c | 10 +++-------
>>> 1 file changed, 3 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
>>> index 23991a3612bd..9708ddcc4919 100644
>>> --- a/drivers/iio/adc/meson_saradc.c
>>> +++ b/drivers/iio/adc/meson_saradc.c
>>> @@ -786,7 +786,7 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev,
>>> static int meson_sar_adc_temp_sensor_init(struct iio_dev *indio_dev)
>>> {
>>
>> Nice to see one of these cleanup.h patches that is actually fixing a bug.
>
> It's not fixing anything as far as I can see.
> The syscon_regmap_lookup_by_handle() is earlier in the function and
> nvmem_cell_read() isn't called if that fails.
>
> So this is just a code simplification and so small benefit if anything.
>
> Maybe there is an old version that does things in a different order?
I just updated iio/testing today and I see return without kfree().
priv->tsc_regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "amlogic,hhi-sysctrl");
if (IS_ERR(priv->tsc_regmap))
return dev_err_probe(dev, PTR_ERR(priv->tsc_regmap),
"failed to get amlogic,hhi-sysctrl regmap\n");
>
>
>>
>> Should `#include <linux/cleanup.h>` though rather that relying on it being
>> included through another header.
>>
>> With that fixed...
>>
>> Reviewed-by: David Lechner <dlechner@baylibre.com>
>>
>
On Sun, Apr 26, 2026 at 7:16 AM David Lechner <dlechner@baylibre.com> wrote: > I just updated iio/testing today and I see return without kfree(). > > priv->tsc_regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "amlogic,hhi-sysctrl"); > if (IS_ERR(priv->tsc_regmap)) > return dev_err_probe(dev, PTR_ERR(priv->tsc_regmap), > "failed to get amlogic,hhi-sysctrl regmap\n"); > >> Should `#include <linux/cleanup.h>` though rather that relying on it being > >> included through another header. > >> > >> With that fixed... > >> > >> Reviewed-by: David Lechner <dlechner@baylibre.com> > >> > > > Hi Jonathan, This code was merged into linux/master yesterday, perhaps you haven't updated the code base. Best regards, Felix
On Sun, 26 Apr 2026 11:54:47 +0800 Felix Gu <ustc.gu@gmail.com> wrote: > On Sun, Apr 26, 2026 at 7:16 AM David Lechner <dlechner@baylibre.com> wrote: > > I just updated iio/testing today and I see return without kfree(). > > > > priv->tsc_regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "amlogic,hhi-sysctrl"); > > if (IS_ERR(priv->tsc_regmap)) > > return dev_err_probe(dev, PTR_ERR(priv->tsc_regmap), > > "failed to get amlogic,hhi-sysctrl regmap\n"); > > >> Should `#include <linux/cleanup.h>` though rather that relying on it being > > >> included through another header. > > >> > > >> With that fixed... > > >> > > >> Reviewed-by: David Lechner <dlechner@baylibre.com> > > >> > > > > > > Hi Jonathan, > This code was merged into linux/master yesterday, perhaps you haven't > updated the code base. Ah. My bad. I tend to be lazy and use elixir to check stuff like this and had completely forgotten I merged the patch that broke this :( J > > Best regards, > Felix >
© 2016 - 2026 Red Hat, Inc.