[v6,3/5] thermal/drivers/sun8i: get calibration data from two nvmem cells
Commit Message
From: Mikhail Kalashnikov <iuncuim@gmail.com>
The A523 processor has calibration data in two nvmem cells. To be able to
add support, the ability to add data from two cells into one array must be
added.
Signed-off-by: Mikhail Kalashnikov <iuncuim@gmail.com>
---
drivers/thermal/sun8i_thermal.c | 81 +++++++++++++++++++++------------
1 file changed, 53 insertions(+), 28 deletions(-)
Comments
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Devres-managed memory allocated for `caldata` is leaked for the lifetime of the device if a secondary nvmem cell fails to be acquired.
--
commit 340e56882111780a9b5ca525d954f99c45894fc7
Author: Mikhail Kalashnikov <iuncuim@gmail.com>
thermal/drivers/sun8i: get calibration data from two nvmem cells
The commit updates the sun8i thermal driver to support retrieving calibration
data from multiple nvmem cells. It iterates over nvmem-cell-names and
concatenates the cell data into a single devm-managed buffer.
> diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
> index 1412bce630c0c..3606548801f02 100644
> --- a/drivers/thermal/sun8i_thermal.c
> +++ b/drivers/thermal/sun8i_thermal.c
> @@ -300,43 +300,68 @@ static int sun50i_h6_ths_calibrate(struct ths_device *tmdev,
>
> static int sun8i_ths_calibrate(struct ths_device *tmdev)
> {
[ ... ]
> + of_property_for_each_string(np, "nvmem-cell-names", prop, cellname) {
> + size_t len;
> + u8 *caldatapart;
> +
> + calcell = of_nvmem_cell_get(np, cellname);
> + if (IS_ERR(calcell)) {
> + if (PTR_ERR(calcell) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
[ ... ]
> + goto out;
[Severity: Medium]
Does this goto out cause a memory leak of the devres-managed caldata buffer?
If of_nvmem_cell_get() fails for a secondary cell (with an error other than
-EPROBE_DEFER), the function jumps to out returning 0, since ret is not
updated. Since probe succeeds, the devres memory allocated via
devm_krealloc() for the first cell's data is retained for the lifetime of
the device, skipping the explicit devm_kfree(dev, caldata) before the
out label.
> + }
> +
> + caldatapart = nvmem_cell_read(calcell, &len);
> + nvmem_cell_put(calcell);
> + calcell = NULL;
> + if (IS_ERR(caldatapart)) {
> + ret = PTR_ERR(caldatapart);
> + goto out;
> + }
> +
> + caldata = devm_krealloc(dev, caldata, callen + len, GFP_KERNEL);
> + if (!caldata) {
> + kfree(caldatapart);
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + memcpy(caldata + callen, caldatapart, len);
> + callen += len;
> + kfree(caldatapart);
> }
>
> - caldata = nvmem_cell_read(calcell, &callen);
> - if (IS_ERR(caldata)) {
> - ret = PTR_ERR(caldata);
> + if (!caldata)
> goto out;
> - }
>
> - tmdev->chip->calibrate(tmdev, caldata, callen);
> + tmdev->chip->calibrate(tmdev, (u16 *)caldata, callen);
>
> - kfree(caldata);
> + devm_kfree(dev, caldata);
> + caldata = NULL;
> out:
> - if (!IS_ERR(calcell))
> + if (!IS_ERR_OR_NULL(calcell))
> nvmem_cell_put(calcell);
> return ret;
> }
On Sun, Sep 13, 2026 at 10:56 PM Mikhail Kalashnikov <iuncuim@gmail.com> wrote:
>
> From: Mikhail Kalashnikov <iuncuim@gmail.com>
>
> The A523 processor has calibration data in two nvmem cells. To be able to
> add support, the ability to add data from two cells into one array must be
> added.
>
> Signed-off-by: Mikhail Kalashnikov <iuncuim@gmail.com>
I'd say that Sashiko's comment on "leaking" memory until the device is
removed is not too much of a concern. Sure it could be released sooner
since it's not used, but it's not a lot of memory.
Reviewed-by: Chen-Yu Tsai <wens@kernel.org>
> ---
> drivers/thermal/sun8i_thermal.c | 81 +++++++++++++++++++++------------
> 1 file changed, 53 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
> index 1412bce63..360654880 100644
> --- a/drivers/thermal/sun8i_thermal.c
> +++ b/drivers/thermal/sun8i_thermal.c
> @@ -300,43 +300,68 @@ static int sun50i_h6_ths_calibrate(struct ths_device *tmdev,
>
> static int sun8i_ths_calibrate(struct ths_device *tmdev)
> {
> - struct nvmem_cell *calcell;
> + struct nvmem_cell *calcell = NULL;
> struct device *dev = tmdev->dev;
> - u16 *caldata;
> - size_t callen;
> + struct device_node *np = dev_of_node(dev);
> + struct property *prop;
> + const char *cellname;
> + u8 *caldata = NULL;
> + size_t callen = 0;
> int ret = 0;
>
> - calcell = nvmem_cell_get(dev, "calibration");
> - if (IS_ERR(calcell)) {
> - if (PTR_ERR(calcell) == -EPROBE_DEFER)
> - return -EPROBE_DEFER;
> - /*
> - * Even if the external calibration data stored in sid is
> - * not accessible, the THS hardware can still work, although
> - * the data won't be so accurate.
> - *
> - * The default value of calibration register is 0x800 for
> - * every sensor, and the calibration value is usually 0x7xx
> - * or 0x8xx, so they won't be away from the default value
> - * for a lot.
> - *
> - * So here we do not return error if the calibration data is
> - * not available, except the probe needs deferring.
> - */
> - goto out;
> + of_property_for_each_string(np, "nvmem-cell-names", prop, cellname) {
> + size_t len;
> + u8 *caldatapart;
> +
> + calcell = of_nvmem_cell_get(np, cellname);
> + if (IS_ERR(calcell)) {
> + if (PTR_ERR(calcell) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> + /*
> + * Even if the external calibration data stored in sid is
> + * not accessible, the THS hardware can still work, although
> + * the data won't be so accurate.
> + *
> + * The default value of calibration register is 0x800 for
> + * every sensor, and the calibration value is usually 0x7xx
> + * or 0x8xx, so they won't be away from the default value
> + * for a lot.
> + *
> + * So here we do not return error if the calibration data is
> + * not available, except the probe needs deferring.
> + */
> + goto out;
> + }
> +
> + caldatapart = nvmem_cell_read(calcell, &len);
> + nvmem_cell_put(calcell);
> + calcell = NULL;
> + if (IS_ERR(caldatapart)) {
> + ret = PTR_ERR(caldatapart);
> + goto out;
> + }
> +
> + caldata = devm_krealloc(dev, caldata, callen + len, GFP_KERNEL);
> + if (!caldata) {
> + kfree(caldatapart);
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + memcpy(caldata + callen, caldatapart, len);
> + callen += len;
> + kfree(caldatapart);
> }
>
> - caldata = nvmem_cell_read(calcell, &callen);
> - if (IS_ERR(caldata)) {
> - ret = PTR_ERR(caldata);
> + if (!caldata)
> goto out;
> - }
>
> - tmdev->chip->calibrate(tmdev, caldata, callen);
> + tmdev->chip->calibrate(tmdev, (u16 *)caldata, callen);
>
> - kfree(caldata);
> + devm_kfree(dev, caldata);
> + caldata = NULL;
> out:
> - if (!IS_ERR(calcell))
> + if (!IS_ERR_OR_NULL(calcell))
> nvmem_cell_put(calcell);
> return ret;
> }
> --
> 2.55.0
>
@@ -300,43 +300,68 @@ static int sun50i_h6_ths_calibrate(struct ths_device *tmdev,
static int sun8i_ths_calibrate(struct ths_device *tmdev)
{
- struct nvmem_cell *calcell;
+ struct nvmem_cell *calcell = NULL;
struct device *dev = tmdev->dev;
- u16 *caldata;
- size_t callen;
+ struct device_node *np = dev_of_node(dev);
+ struct property *prop;
+ const char *cellname;
+ u8 *caldata = NULL;
+ size_t callen = 0;
int ret = 0;
- calcell = nvmem_cell_get(dev, "calibration");
- if (IS_ERR(calcell)) {
- if (PTR_ERR(calcell) == -EPROBE_DEFER)
- return -EPROBE_DEFER;
- /*
- * Even if the external calibration data stored in sid is
- * not accessible, the THS hardware can still work, although
- * the data won't be so accurate.
- *
- * The default value of calibration register is 0x800 for
- * every sensor, and the calibration value is usually 0x7xx
- * or 0x8xx, so they won't be away from the default value
- * for a lot.
- *
- * So here we do not return error if the calibration data is
- * not available, except the probe needs deferring.
- */
- goto out;
+ of_property_for_each_string(np, "nvmem-cell-names", prop, cellname) {
+ size_t len;
+ u8 *caldatapart;
+
+ calcell = of_nvmem_cell_get(np, cellname);
+ if (IS_ERR(calcell)) {
+ if (PTR_ERR(calcell) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+ /*
+ * Even if the external calibration data stored in sid is
+ * not accessible, the THS hardware can still work, although
+ * the data won't be so accurate.
+ *
+ * The default value of calibration register is 0x800 for
+ * every sensor, and the calibration value is usually 0x7xx
+ * or 0x8xx, so they won't be away from the default value
+ * for a lot.
+ *
+ * So here we do not return error if the calibration data is
+ * not available, except the probe needs deferring.
+ */
+ goto out;
+ }
+
+ caldatapart = nvmem_cell_read(calcell, &len);
+ nvmem_cell_put(calcell);
+ calcell = NULL;
+ if (IS_ERR(caldatapart)) {
+ ret = PTR_ERR(caldatapart);
+ goto out;
+ }
+
+ caldata = devm_krealloc(dev, caldata, callen + len, GFP_KERNEL);
+ if (!caldata) {
+ kfree(caldatapart);
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ memcpy(caldata + callen, caldatapart, len);
+ callen += len;
+ kfree(caldatapart);
}
- caldata = nvmem_cell_read(calcell, &callen);
- if (IS_ERR(caldata)) {
- ret = PTR_ERR(caldata);
+ if (!caldata)
goto out;
- }
- tmdev->chip->calibrate(tmdev, caldata, callen);
+ tmdev->chip->calibrate(tmdev, (u16 *)caldata, callen);
- kfree(caldata);
+ devm_kfree(dev, caldata);
+ caldata = NULL;
out:
- if (!IS_ERR(calcell))
+ if (!IS_ERR_OR_NULL(calcell))
nvmem_cell_put(calcell);
return ret;
}