Messages in this thread | | | Date | Fri, 16 Dec 2022 02:46:44 -0800 | Subject | Re: [PATCH] watchdog: da9062: da9063: use unlocked xfer function in restart | From | Guenter Roeck <> |
| |
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);
| |