Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <efe041a2-8d5b-f06a-73b8-3151b0a33004@roeck-us.net>
Date:   Fri, 16 Dec 2022 02:46:44 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH] watchdog: da9062: da9063: use unlocked xfer function in
 restart
Content-Language: en-US
To:     Primoz Fiser <primoz.fiser@norik.com>,
        Support Opensource <support.opensource@diasemi.com>,
        Wim Van Sebroeck <wim@linux-watchdog.org>,
        linux-watchdog@vger.kernel.org, linux-kernel@vger.kernel.org
Cc:     upstream@lists.phytec.de
References: <20221216083645.2574077-1-primoz.fiser@norik.com>
From:   Guenter Roeck <linux@roeck-us.net>
In-Reply-To: <20221216083645.2574077-1-primoz.fiser@norik.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@norik.com>

Reviewed-by: Guenter Roeck <linux@roeck-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);