linux-kernel - Re: [PATCH 3/3] hwmon: spd5118: Avoid hardware access during suspend and resume

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5e223091-9748-46c5-8e99-a894655f8bc8@roeck-us.net>
Date: Mon, 12 Jan 2026 06:42:04 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Tinsae Tadesse <tinsaetadesse2015@...il.com>
Cc: linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] hwmon: spd5118: Avoid hardware access during suspend
 and resume

On 1/10/26 09:19, Tinsae Tadesse wrote:
> The SPD5118 hub may be inaccessible during suspend/resume.
> Avoid updating/writing hardware in PM callbacks by switching the regmap
> to cache-only mode during suspend and deferring synchronization
> until first access.
> 
> This prevents the below I2C errors and async resume failures.
> 
> spd5118 ...: Failed to write b = 0: -6
> spd5118 ...: PM: dpm_run_callback(): spd5118_resume returns -6
> spd5118 ...: PM: failed to resume async: error -6
> 
> Signed-off-by: Tinsae Tadesse <tinsaetadesse2015@...il.com>
> ---
>   drivers/hwmon/spd5118.c | 36 ++++++++++--------------------------
>   1 file changed, 10 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
> index 63f798991363..a8afde7f47b2 100644
> --- a/drivers/hwmon/spd5118.c
> +++ b/drivers/hwmon/spd5118.c
> @@ -494,25 +494,12 @@ static const struct regmap_config spd5118_regmap16_config = {
>   static int spd5118_suspend(struct device *dev)
>   {
>   	struct spd5118_data *data = dev_get_drvdata(dev);
> -	struct regmap *regmap = data->regmap;
> -	u32 regval;
> -	int err;
>   
>   	/*
> -	 * Make sure the configuration register in the regmap cache is current
> -	 * before bypassing it.
> +	 * The SPD5118 hub may be inaccessible; avoid hardware access.
>   	 */
> -	err = regmap_read(regmap, SPD5118_REG_TEMP_CONFIG, &regval);
> -	if (err < 0)
> -		return err;
> -

There was a reason for this code. If the register was never read,
the regmap cache will not contain the correct and expected values.

> -	regcache_cache_bypass(regmap, true);
> -	regmap_update_bits(regmap, SPD5118_REG_TEMP_CONFIG, SPD5118_TS_DISABLE,
> -			   SPD5118_TS_DISABLE);

The point of this code was to disable the sensors during suspend.
Leaving the sensors enabled defeats the purpose of the suspend function.

Something is seriously broken in the system configuration if the i2c
controller suspends early, before the devices connected to it had a chance
to suspend. Maybe that should be fixed instead ?

> -	regcache_cache_bypass(regmap, false);
> -
> -	regcache_cache_only(regmap, true);
> -	regcache_mark_dirty(regmap);
> +	regcache_cache_only(data->regmap, true);
> +	regcache_mark_dirty(data->regmap);

It seems you dislike local variables. That is not a reason for removing them.

Guenter

>   
>   	return 0;
>   }
> @@ -520,16 +507,13 @@ static int spd5118_suspend(struct device *dev)
>   static int spd5118_resume(struct device *dev)
>   {
>   	struct spd5118_data *data = dev_get_drvdata(dev);
> -	struct regmap *regmap = data->regmap;
> -	int ret;
> -
> -	regcache_cache_only(regmap, false);
> -	ret = regcache_sync(regmap);
> -	if(ret == -ENXIO || ret == -EIO) {
> -		dev_warn(dev, "SPD hub not responding on resume (%d), deferring init\n", ret);
> -		return 0;
> -	}
> -	return ret;
> +	
> +	/*
> +	 * Re-enable hardware access; sync is deferred until first read.
> +	 */
> +	regcache_cache_only(data->regmap, false);
> +	
> +	return 0;
>   }
>   
>   static DEFINE_SIMPLE_DEV_PM_OPS(spd5118_pm_ops, spd5118_suspend, spd5118_resume);