Re: [PATCH v4 3/9] rtc: abx80x: add mutex protection for register writes

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

Pre-existing issues:
- [High] Incomplete locking leaves configuration key sequences vulnerable to disruption by concurrent read operations.
--

--- Patch [3]: [PATCH v4 3/9] rtc: abx80x: add mutex protection for register writes ---
commit 45525a43d1a439e6b8a50fcba7c541c85b598f5c
Author: Antoni Pokusinski <[email protected]>

rtc: abx80x: add mutex protection for register writes

The ABX80X RTC driver performs multi-step register operations such as
NVMEM transfers or register writes preceded by a configuration key write.
This patch adds a mutex to serialize register writes and protect these
sequences against race conditions.

> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index fdf9664ebb6f0..0c8d3416fd5a9 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c
[ ... ]
> @@ -234,6 +237,8 @@ static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  	buf[ABX8XX_REG_YR] = bin2bcd(tm->tm_year - 100);
>  	buf[ABX8XX_REG_WD] = tm->tm_wday;
>  
> +	guard(mutex)(&priv->lock);
> +

[Severity: High]
This is a pre-existing issue, but since this patch introduces the mutex to
protect multi-step sequences, should the read paths also acquire the lock?

According to the ABX80X hardware specification, the configuration key is
automatically cleared by any subsequent register read or write.

If a sysfs attribute write like oscillator_store() acquires the lock and
writes the configuration key, but another process concurrently calls an
unprotected read path like abx80x_rtc_read_time() or abx80x_read_alarm():

abx80x_rtc_read_time()
    err = i2c_smbus_read_i2c_block_data(client, ABX8XX_REG_HTH, ...);

Could this interleaved I2C read silently clear the configuration key before
the protected write completes, causing the hardware configuration update to
fail?

>  	err = i2c_smbus_write_i2c_block_data(client, ABX8XX_REG_HTH,
>  					     sizeof(buf), buf);
>  	if (err < 0) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3