linux-kernel - Re: [PATCH] watchdog: da9062: da9063: use unlocked xfer function in restart

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 for Android: free password hash cracker in your pocket

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

Message-ID: <efe041a2-8d5b-f06a-73b8-3151b0a33004@roeck-us.net>
Date:   Fri, 16 Dec 2022 02:46:44 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Primoz Fiser <primoz.fiser@...ik.com>,
        Support Opensource <support.opensource@...semi.com>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     upstream@...ts.phytec.de
Subject: Re: [PATCH] watchdog: da9062: da9063: use unlocked xfer function in
 restart

On 12/16/22 00:36, Primoz Fiser wrote:
> Machine resets via da9062/da9063 PMICs are challenging since one needs
> to use special i2c atomic transfers due to the fact interrupts are
> disabled in such late system stages. This is the reason both PMICs don't
> use regmap and have instead opted for i2c_smbus_write_byte_data() in
> restart handlers.
> 
> However extensive testing revealed that even using atomic safe function
> is not enough and occasional resets fail with error message "Failed to
> shutdown (err =  -11)". This is due to the fact that function
> i2c_smbus_write_byte_data() in turn calls __i2c_lock_bus_helper()
> which might fail with -EAGAIN when bus lock is already taken and cannot
> be released anymore.
> 
> Thus replace i2c_smbus_write_byte_data() with unlocked flavor of
> i2c_smbus_xfer() function to avoid above dead-lock scenario. At this
> system stage we don't care about proper locking anymore and only want
> proper machine reset to be carried out.
> 
> Signed-off-by: Primoz Fiser <primoz.fiser@...ik.com>

Reviewed-by: Guenter Roeck <linux@...ck-us.net>

> ---
>   drivers/watchdog/da9062_wdt.c | 15 ++++++++++++---
>   drivers/watchdog/da9063_wdt.c | 15 ++++++++++++---
>   2 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
> index f02cbd530538..1ec33b4bd2f2 100644
> --- a/drivers/watchdog/da9062_wdt.c
> +++ b/drivers/watchdog/da9062_wdt.c
> @@ -155,11 +155,20 @@ static int da9062_wdt_restart(struct watchdog_device *wdd, unsigned long action,
>   {
>   	struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
>   	struct i2c_client *client = to_i2c_client(wdt->hw->dev);
> +	union i2c_smbus_data msg;
>   	int ret;
>   
> -	/* Don't use regmap because it is not atomic safe */
> -	ret = i2c_smbus_write_byte_data(client, DA9062AA_CONTROL_F,
> -					DA9062AA_SHUTDOWN_MASK);
> +	/*
> +	 * Don't use regmap because it is not atomic safe. Additionally, use
> +	 * unlocked flavor of i2c_smbus_xfer to avoid scenario where i2c bus
> +	 * might be previously locked by some process unable to release the
> +	 * lock due to interrupts already being disabled at this late stage.
> +	 */
> +	msg.byte = DA9062AA_SHUTDOWN_MASK;
> +	ret = __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> +			I2C_SMBUS_WRITE, DA9062AA_CONTROL_F,
> +			I2C_SMBUS_BYTE_DATA, &msg);
> +
>   	if (ret < 0)
>   		dev_alert(wdt->hw->dev, "Failed to shutdown (err = %d)\n",
>   			  ret);
> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> index 09a4af4c58fc..684667469b10 100644
> --- a/drivers/watchdog/da9063_wdt.c
> +++ b/drivers/watchdog/da9063_wdt.c
> @@ -174,11 +174,20 @@ static int da9063_wdt_restart(struct watchdog_device *wdd, unsigned long action,
>   {
>   	struct da9063 *da9063 = watchdog_get_drvdata(wdd);
>   	struct i2c_client *client = to_i2c_client(da9063->dev);
> +	union i2c_smbus_data msg;
>   	int ret;
>   
> -	/* Don't use regmap because it is not atomic safe */
> -	ret = i2c_smbus_write_byte_data(client, DA9063_REG_CONTROL_F,
> -					DA9063_SHUTDOWN);
> +	/*
> +	 * Don't use regmap because it is not atomic safe. Additionally, use
> +	 * unlocked flavor of i2c_smbus_xfer to avoid scenario where i2c bus
> +	 * might previously be locked by some process unable to release the
> +	 * lock due to interrupts already being disabled at this late stage.
> +	 */
> +	msg.byte = DA9063_SHUTDOWN;
> +	ret = __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> +			I2C_SMBUS_WRITE, DA9063_REG_CONTROL_F,
> +			I2C_SMBUS_BYTE_DATA, &msg);
> +
>   	if (ret < 0)
>   		dev_alert(da9063->dev, "Failed to shutdown (err = %d)\n",
>   			  ret);