Re: [PATCH RFC] rtc: m41t80: Support SMBus adapters without I2C block transfers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] rtc: m41t80: raw i2c_transfer() in wdt_ping() and wdt_disable() fails on SMBus
--

commit b3f3c10491b061e74e177e35683431f79685d1f4
Author: Florian Fainelli <f.fainelli@gmail.com>

rtc: m41t80: Support SMBus adapters without I2C block transfers

This commit allows rtc-m41t80 to probe on SMBus adapters that only support
SMBus byte and word transactions (such as the SiByte SMBus controller with an
M41T81) by falling back to byte writes and emulated block reads, with a
seconds recheck loop when block reads are unavailable.

Link: https://lkml.rescloud.iu.edu/hypermail/linux/kernel/0805.1/1778.html
Link: https://lkml.rescloud.iu.edu/1811.1/07408.html

> diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
> index 1385cb43864ba..23d02b95a606b 100644
> --- a/drivers/rtc/rtc-m41t80.c
> +++ b/drivers/rtc/rtc-m41t80.c

[ ... ]

> @@ -906,9 +949,9 @@ static int m41t80_probe(struct i2c_client *client)
>  	struct m41t80_data *m41t80_data = NULL;
>  	bool wakeup_source = false;
>  
> -	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_I2C_BLOCK |
> +	if (!i2c_check_functionality(client->adapter,
>  				     I2C_FUNC_SMBUS_BYTE_DATA)) {
> -		dev_err(&adapter->dev, "doesn't support I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_I2C_BLOCK\n");
> +		dev_err(&adapter->dev, "doesn't support I2C_FUNC_SMBUS_BYTE_DATA\n");
>  		return -ENODEV;
>  	}

[Severity: Medium]
With m41t80_probe() now accepting SMBus-only adapters that only support
I2C_FUNC_SMBUS_BYTE_DATA and lack master_xfer (such as i2c-sibyte), what
happens when CONFIG_RTC_DRV_M41T80_WDT is enabled on an M41T80_FEATURE_HT
chip like the M41T81?

Later in m41t80_probe(), the watchdog miscdevice and reboot notifier are
still registered unconditionally for M41T80_FEATURE_HT devices:

drivers/rtc/rtc-m41t80.c:m41t80_probe() {
	...
#ifdef CONFIG_RTC_DRV_M41T80_WDT
	if (m41t80_data->features & M41T80_FEATURE_HT) {
		save_client = client;
		rc = misc_register(&wdt_dev);
		if (rc)
			return rc;
		rc = register_reboot_notifier(&wdt_notifier);
	...
}

However, wdt_ping() and wdt_disable() in drivers/rtc/rtc-m41t80.c still use
raw i2c_transfer() messages to access register 0x09 and ignore the return
value:

drivers/rtc/rtc-m41t80.c:wdt_ping() {
	...
	i2c_data[0] = 0x09;		/* watchdog register */
	...
	i2c_transfer(save_client->adapter, msgs1, 1);
}

drivers/rtc/rtc-m41t80.c:wdt_disable() {
	...
	i2c_data[0] = 0x09;
	i2c_transfer(save_client->adapter, msgs0, 2);

	i2c_data[0] = 0x09;
	i2c_data[1] = 0x00;
	i2c_transfer(save_client->adapter, msgs1, 1);
}

On an SMBus-only adapter where adapter->algo->master_xfer is NULL,
i2c_transfer() fails with -EOPNOTSUPP, so watchdog keepalives, ioctl
operations, and reboot/shutdown disable notifications via wdt_notify_sys() ->
wdt_disable() will silently fail to reach the hardware.

Should wdt_ping() and wdt_disable() be converted to use
i2c_smbus_write_byte_data() and i2c_smbus_read_byte_data() as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103225027.1248035-1-f.fainelli@gmail.com?part=1