Date:   Fri, 13 Dec 2019 17:36:37 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Thierry Reding <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Laxman Dewangan <ldewangan@...dia.com>,
        Wolfram Sang <wsa@...-dreams.de>
Cc:     linux-i2c@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-kernel@...r.kernel.org, Mikko Perttunen <cyndis@...si.fi>
Subject: Re: [PATCH v1 1/3] i2c: tegra: Support atomic transfers

13.12.2019 02:34, Dmitry Osipenko пишет:
> System shutdown may happen with interrupts being disabled and in this case
> I2C core rejects transfers if atomic transfer isn't supported by driver.
> 
> There were several occurrences where I found my Nexus 7 completely
> discharged despite of being turned off and then one day I spotted this in
> the log:
> 
>  reboot: Power down
>  ------------[ cut here ]------------
>  WARNING: CPU: 0 PID: 1 at drivers/i2c/i2c-core.h:40 i2c_transfer+0x95/0x9c
>  No atomic I2C transfer handler for 'i2c-1'
>  Modules linked in: tegra30_devfreq
>  CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 5.4.0-next-20191202-00120-gf7ecd80fb803-dirty #3195
>  Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
>  [<c010e4b5>] (unwind_backtrace) from [<c010a0fd>] (show_stack+0x11/0x14)
>  [<c010a0fd>] (show_stack) from [<c09995e5>] (dump_stack+0x85/0x94)
>  [<c09995e5>] (dump_stack) from [<c011f3d1>] (__warn+0xc1/0xc4)
>  [<c011f3d1>] (__warn) from [<c011f691>] (warn_slowpath_fmt+0x61/0x78)
>  [<c011f691>] (warn_slowpath_fmt) from [<c069a8dd>] (i2c_transfer+0x95/0x9c)
>  [<c069a8dd>] (i2c_transfer) from [<c05667f1>] (regmap_i2c_read+0x4d/0x6c)
>  [<c05667f1>] (regmap_i2c_read) from [<c0563601>] (_regmap_raw_read+0x99/0x1cc)
>  [<c0563601>] (_regmap_raw_read) from [<c0563757>] (_regmap_bus_read+0x23/0x38)
>  [<c0563757>] (_regmap_bus_read) from [<c056293d>] (_regmap_read+0x3d/0xfc)
>  [<c056293d>] (_regmap_read) from [<c0562d3b>] (_regmap_update_bits+0x87/0xc4)
>  [<c0562d3b>] (_regmap_update_bits) from [<c0563add>] (regmap_update_bits_base+0x39/0x50)
>  [<c0563add>] (regmap_update_bits_base) from [<c056fd39>] (max77620_pm_power_off+0x29/0x2c)
>  [<c056fd39>] (max77620_pm_power_off) from [<c013bbdd>] (__do_sys_reboot+0xe9/0x170)
>  [<c013bbdd>] (__do_sys_reboot) from [<c0101001>] (ret_fast_syscall+0x1/0x28)
>  Exception stack(0xde907fa8 to 0xde907ff0)
>  7fa0:                   00000000 00000000 fee1dead 28121969 4321fedc 00000000
>  7fc0: 00000000 00000000 00000000 00000058 00000000 00000000 00000000 00000000
>  7fe0: 0045adf0 bed9abb8 004444a0 b6c666d0
>  ---[ end trace bdd18f87595b1a5e ]---
> 
> The atomic transferring is implemented by enforcing PIO mode for the
> transfer and by polling interrupt status until transfer is completed or
> failed.
> 
> Now system shuts down properly every time.
> 
> Signed-off-by: Dmitry Osipenko <digetx@...il.com>
> ---
>  drivers/i2c/busses/i2c-tegra.c | 95 +++++++++++++++++++++++++++++++---
>  1 file changed, 88 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index a98bf31d0e5c..799c343571bc 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -17,6 +17,7 @@
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
>  #include <linux/kernel.h>
> +#include <linux/ktime.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
>  #include <linux/pinctrl/consumer.h>
> @@ -248,6 +249,7 @@ struct tegra_i2c_hw_feature {
>   * @dma_buf_size: DMA buffer size
>   * @is_curr_dma_xfer: indicates active DMA transfer
>   * @dma_complete: DMA completion notifier
> + * @is_curr_atomic_xfer: indicates active atomic transfer
>   */
>  struct tegra_i2c_dev {
>  	struct device *dev;
> @@ -279,6 +281,7 @@ struct tegra_i2c_dev {
>  	unsigned int dma_buf_size;
>  	bool is_curr_dma_xfer;
>  	struct completion dma_complete;
> +	bool is_curr_atomic_xfer;
>  };
>  
>  static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
> @@ -999,6 +1002,44 @@ static void tegra_i2c_config_fifo_trig(struct tegra_i2c_dev *i2c_dev,
>  	i2c_writel(i2c_dev, val, reg);
>  }
>  
> +static unsigned long
> +tegra_i2c_poll_completion_timeout(struct tegra_i2c_dev *i2c_dev,
> +				  unsigned int timeout_ms)
> +{
> +	ktime_t ktime = ktime_get();
> +	ktime_t ktimeout = ktime_add_ms(ktime, timeout_ms);
> +
> +	do {
> +		u32 status = i2c_readl(i2c_dev, I2C_INT_STATUS);
> +
> +		if (status) {
> +			tegra_i2c_isr(i2c_dev->irq, i2c_dev);
> +
> +			if (completion_done(&i2c_dev->msg_complete)) {
> +				s64 delta = ktime_ms_delta(ktimeout, ktime);
> +
> +				return msecs_to_jiffies(delta) ?: 1;
> +			}
> +		}
> +
> +		ktime = ktime_get();
> +
> +	} while (ktime_before(ktime, ktimeout));
> +
> +	return 0;
> +}
> +
> +static unsigned long
> +tegra_i2c_wait_msg_completion_timeout(struct tegra_i2c_dev *i2c_dev,
> +				      unsigned int timeout_ms)
> +{
> +	if (i2c_dev->is_curr_atomic_xfer)
> +		return tegra_i2c_poll_completion_timeout(i2c_dev, timeout_ms);
> +
> +	return wait_for_completion_timeout(&i2c_dev->msg_complete,
> +					   msecs_to_jiffies(timeout_ms));
> +}
> +
>  static int tegra_i2c_issue_bus_clear(struct i2c_adapter *adap)
>  {
>  	struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
> @@ -1020,8 +1061,7 @@ static int tegra_i2c_issue_bus_clear(struct i2c_adapter *adap)
>  	i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
>  	tegra_i2c_unmask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
>  
> -	time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
> -						msecs_to_jiffies(50));
> +	time_left = tegra_i2c_wait_msg_completion_timeout(i2c_dev, 50);

I realized that potentially there is a nasty problem here.. nothing
stops interrupt to occur after the timeout, thus I2C hardware could be
reset at the time of interrupt handling.

The solution will be to always keep interrupt disabled and only enable
it for the wait_for_completion_timeout(). I'll address it in the next
revision of this series.

[snip]

Powered by blists - more mailing lists