All of lore.kernel.org
 help / color / mirror / Atom feed
From: sashiko-bot@kernel.org
To: "Felix Gu" <ustc.gu@gmail.com>
Cc: linux-sunxi@lists.linux.dev
Subject: Re: [PATCH 2/2] iio: adc: sun4i-gpadc-iio: clean up on thermal zone registration failure
Date: Sun, 06 Sep 2026 16:08:55 +0000	[thread overview]
Message-ID: <20260906160855.964E71F00A3A@smtp.kernel.org> (raw)
In-Reply-To: <20260906-gpadc-v1-2-92d3dc8ef355@gmail.com>

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] pm_runtime_disable() is called manually before devm-managed subsystems are unregistered, exposing a window where callbacks run with runtime PM disabled.
- [High] The driver ignores the return value of pm_runtime_get_sync(), which can lead to accessing hardware registers while the device is suspended or powered off.
--

commit 0cc8a4f7ab93d097709950676951a80ac9c05108
Author: Felix Gu <ustc.gu@gmail.com>

    iio: adc: sun4i-gpadc-iio: clean up on thermal zone registration failure
    
    If devm_thermal_of_zone_register() fails, probe returns without
    unregistering the IIO map array or disabling runtime PM.
    
    Jump to err_map to release them.

> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
> index 85382e435f2a7..ac5ba76b979e6 100644
> --- a/drivers/iio/adc/sun4i-gpadc-iio.c
> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
> @@ -638,10 +638,10 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>  		 * thermal because no thermal DT node is found.
>  		 */
>  		if (IS_ERR(info->tzd) && PTR_ERR(info->tzd) != -ENODEV) {
> -			dev_err(&pdev->dev,
> -				"could not register thermal sensor: %ld\n",
> -				PTR_ERR(info->tzd));
> -			return PTR_ERR(info->tzd);
> +			ret = dev_err_probe(&pdev->dev,
> +					    PTR_ERR(info->tzd),
> +					    "could not register thermal sensor\n");
> +			goto err_map;
>  		}
>  	}
>  
>  	ret = devm_iio_device_register(&pdev->dev, indio_dev);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "could not register the device\n");
>  		goto err_map;
>  	}
>  
>  	return 0;
>  
>  err_map:
>  	if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF))
>  		iio_map_array_unregister(indio_dev);
>  
>  	pm_runtime_disable(&pdev->dev);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This is a pre-existing issue, but does explicitly calling pm_runtime_disable()
here create a window where the devres-managed thermal zone and IIO devices are
still registered while runtime PM is disabled?

Since devm_thermal_of_zone_register() and devm_iio_device_register() are used,
devres will not unregister them until after the probe error path returns.

In the interim, if the thermal core's polling thread invokes .get_temp() or
userspace reads IIO sysfs, could this access hardware while clocks are
disabled, triggering a synchronous external abort?

A similar pre-existing issue appears in sun4i_gpadc_remove():

drivers/iio/adc/sun4i-gpadc-iio.c:sun4i_gpadc_remove() {
	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
	struct sun4i_gpadc_iio *info = iio_priv(indio_dev);

	pm_runtime_disable(&pdev->dev);
...
}

Is pm_runtime_disable() being called here before devres tears down the
registered IIO and thermal zones?

There is another pre-existing issue where return values for
pm_runtime_get_sync() are not checked before accessing hardware. Can this lead
to accessing hardware registers while the device is suspended or powered off?

If pm_runtime_get_sync() fails (e.g., due to runtime PM being disabled during
the teardown race from err_map or remove, or a genuine PM failure), it returns
a negative error code but regmap accesses proceed anyway:

drivers/iio/adc/sun4i-gpadc-iio.c:sun4i_prepare_for_irq() {
	...
	pm_runtime_get_sync(indio_dev->dev.parent);

	reinit_completion(&info->completion);

	ret = regmap_write(info->regmap, SUN4I_GPADC_INT_FIFOC,
	...
}

And also in sun4i_gpadc_temp_read():

drivers/iio/adc/sun4i-gpadc-iio.c:sun4i_gpadc_temp_read() {
	struct sun4i_gpadc_iio *info = iio_priv(indio_dev);

	if (info->no_irq) {
		pm_runtime_get_sync(indio_dev->dev.parent);

		regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
	...
}

Should the return value of pm_runtime_get_sync() be checked to avoid a bus
fault or kernel panic when reading or writing to unpowered MMIO registers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906-gpadc-v1-0-92d3dc8ef355@gmail.com?part=2

  reply	other threads:[~2026-09-06 16:08 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-06 15:58 [PATCH 0/2] iio: adc: sun4i-gpadc-iio: two fixes Felix Gu
2026-09-06 15:58 ` [PATCH 1/2] iio: adc: sun4i-gpadc-iio: drop underflowing pm_runtime_put() calls Felix Gu
2026-09-06 16:08   ` sashiko-bot
2026-09-06 15:58 ` [PATCH 2/2] iio: adc: sun4i-gpadc-iio: clean up on thermal zone registration failure Felix Gu
2026-09-06 16:08   ` sashiko-bot [this message]
2026-09-06 17:36 ` [PATCH 0/2] iio: adc: sun4i-gpadc-iio: two fixes Jonathan Cameron

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260906160855.964E71F00A3A@smtp.kernel.org \
    --to=sashiko-bot@kernel.org \
    --cc=linux-sunxi@lists.linux.dev \
    --cc=sashiko-reviews@lists.linux.dev \
    --cc=ustc.gu@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.