Re: [PATCH] i2c: tegra: Wait for config load atomically while in ISR

From: Mikko Perttunen <cyndis@kapsi.fi>
To: Dmitry Osipenko <digetx@gmail.com>,
	Mikko Perttunen <mperttunen@nvidia.com>,
	ldewangan@nvidia.com, thierry.reding@gmail.com,
	jonathanh@nvidia.com, wsa@kernel.org
Cc: linux-i2c@vger.kernel.org, linux-tegra@vger.kernel.org,
	linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH] i2c: tegra: Wait for config load atomically while in ISR
Date: Mon, 11 Jan 2021 17:52:19 +0200	[thread overview]
Message-ID: <a180bd42-2cd9-87b6-1848-61058f198d63@kapsi.fi> (raw)
In-Reply-To: <b7f69fb9-2960-f994-51d8-afd86af26bba@gmail.com>

On 1/11/21 5:14 PM, Dmitry Osipenko wrote:
> 11.01.2021 16:55, Mikko Perttunen пишет:
>> Upon a communication error, the interrupt handler can call
>> tegra_i2c_disable_packet_mode. This causes a sleeping poll to happen
>> unless the current transaction was marked atomic. Since
>> tegra_i2c_disable_packet_mode is only called from the interrupt path,
>> make it use atomic waiting always.
>>
>> Fixes: ede2299f7101 ("i2c: tegra: Support atomic transfers")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>> ---
>> This fixes constant spew for me while HDMI is connected to a
>> powered off television.
>>
>>   drivers/i2c/busses/i2c-tegra.c | 16 ++++++++--------
>>   1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>> index 6f08c0c3238d..4a7ff1840565 100644
>> --- a/drivers/i2c/busses/i2c-tegra.c
>> +++ b/drivers/i2c/busses/i2c-tegra.c
>> @@ -528,12 +528,12 @@ static void tegra_i2c_vi_init(struct tegra_i2c_dev *i2c_dev)
>>   
>>   static int tegra_i2c_poll_register(struct tegra_i2c_dev *i2c_dev,
>>   				   u32 reg, u32 mask, u32 delay_us,
>> -				   u32 timeout_us)
>> +				   u32 timeout_us, bool force_atomic)
>>   {
>>   	void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg);
>>   	u32 val;
>>   
>> -	if (!i2c_dev->atomic_mode)
>> +	if (!i2c_dev->atomic_mode && !force_atomic)
>>   		return readl_relaxed_poll_timeout(addr, val, !(val & mask),
>>   						  delay_us, timeout_us);
>>   
>> @@ -560,7 +560,7 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
>>   	val |= mask;
>>   	i2c_writel(i2c_dev, val, offset);
>>   
>> -	err = tegra_i2c_poll_register(i2c_dev, offset, mask, 1000, 1000000);
>> +	err = tegra_i2c_poll_register(i2c_dev, offset, mask, 1000, 1000000, false);
>>   	if (err) {
>>   		dev_err(i2c_dev->dev, "failed to flush FIFO\n");
>>   		return err;
>> @@ -569,7 +569,7 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
>>   	return 0;
>>   }
>>   
>> -static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
>> +static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev, bool force_atomic)
>>   {
>>   	int err;
>>   
>> @@ -579,7 +579,7 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
>>   	i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
>>   
>>   	err = tegra_i2c_poll_register(i2c_dev, I2C_CONFIG_LOAD, 0xffffffff,
>> -				      1000, I2C_CONFIG_LOAD_TIMEOUT);
>> +				      1000, I2C_CONFIG_LOAD_TIMEOUT, force_atomic);
>>   	if (err) {
>>   		dev_err(i2c_dev->dev, "failed to load config\n");
>>   		return err;
>> @@ -684,7 +684,7 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>>   	if (i2c_dev->multimaster_mode && i2c_dev->hw->has_slcg_override_reg)
>>   		i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE);
>>   
>> -	err = tegra_i2c_wait_for_config_load(i2c_dev);
>> +	err = tegra_i2c_wait_for_config_load(i2c_dev, false);
>>   	if (err)
>>   		return err;
>>   
>> @@ -707,7 +707,7 @@ static int tegra_i2c_disable_packet_mode(struct tegra_i2c_dev *i2c_dev)
>>   	if (cnfg & I2C_CNFG_PACKET_MODE_EN)
>>   		i2c_writel(i2c_dev, cnfg & ~I2C_CNFG_PACKET_MODE_EN, I2C_CNFG);
>>   
>> -	return tegra_i2c_wait_for_config_load(i2c_dev);
>> +	return tegra_i2c_wait_for_config_load(i2c_dev, true);
>>   }
>>   
>>   static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev)
>> @@ -1090,7 +1090,7 @@ static int tegra_i2c_issue_bus_clear(struct i2c_adapter *adap)
>>   	      I2C_BC_TERMINATE;
>>   	i2c_writel(i2c_dev, val, I2C_BUS_CLEAR_CNFG);
>>   
>> -	err = tegra_i2c_wait_for_config_load(i2c_dev);
>> +	err = tegra_i2c_wait_for_config_load(i2c_dev, false);
>>   	if (err)
>>   		return err;
>>   
>>
> 
> What about a simpler variant:
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 6f08c0c3238d..0727383f4940 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -533,7 +533,7 @@ static int tegra_i2c_poll_register(struct
> tegra_i2c_dev *i2c_dev,
>   	void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg);
>   	u32 val;
> 
> -	if (!i2c_dev->atomic_mode)
> +	if (!i2c_dev->atomic_mode && !in_irq())
>   		return readl_relaxed_poll_timeout(addr, val, !(val & mask),
>   						  delay_us, timeout_us);
> 

Yep, I'll post a v2 with that.

Mikko