[v6,6/8] i2c: wmt: fix a bug when thread blocked

During each byte access, the host performs clock stretching.
In this case, the thread may be interrupted by preemption,
resulting in a long stretching time.

However, some touchpad can only tolerate host clock stretching
of no more than 200 ms. We reduce the impact of this through
a retransmission mechanism.

Signed-off-by: Hans Hu <hanshu-oc@zhaoxin.com>
---
 drivers/i2c/busses/i2c-viai2c-common.c | 37 +++++++++++++++++++++-----
 drivers/i2c/busses/i2c-viai2c-common.h |  2 ++
 2 files changed, 33 insertions(+), 6 deletions(-)

Hi Hans,

[...]

>  static int viai2c_wait_bus_ready(struct viai2c *i2c)
>  {
> @@ -25,12 +26,35 @@ static int viai2c_wait_bus_ready(struct viai2c *i2c)
>  static int viai2c_wait_status(struct viai2c *i2c)
>  {
>  	int ret = 0;
> -	unsigned long wait_result;
> +	unsigned long time_left;
> +	unsigned long delta_ms;
> +
> +	time_left = wait_for_completion_timeout(&i2c->complete,
> +						VIAI2C_TIMEOUT);
> +	if (!time_left) {
> +		dev_err(i2c->dev, "bus transfer timeout\n");
> +		return -EIO;
> +	}
>  
> -	wait_result = wait_for_completion_timeout(&i2c->complete,
> -						msecs_to_jiffies(500));
> -	if (!wait_result)
> -		return -ETIMEDOUT;

this change is unrelated. Why are you changing the timeout here
from 500 to 1000?

> +	/*
> +	 * During each byte access, the host performs clock stretching.
> +	 * In this case, the thread may be interrupted by preemption,
> +	 * resulting in a long stretching time.
> +	 * However, some touchpad can only tolerate host clock stretching
> +	 * of no more than 200 ms. We reduce the impact of this through
> +	 * a retransmission mechanism.
> +	 */

is the hardware sending the stretching on its own?

> +	local_irq_disable();
> +	i2c->to = ktime_get();
> +	delta_ms = ktime_to_ms(ktime_sub(i2c->to, i2c->ti));
> +	if (delta_ms > VIAI2C_STRETCHING_TIMEOUT) {
> +		local_irq_enable();
> +		dev_warn(i2c->dev, "thread blocked more than %ldms\n",
> +				delta_ms);
> +		return -EAGAIN;
> +	}
> +	i2c->ti = i2c->to;
> +	local_irq_enable();
>  
>  	if (i2c->cmd_status & VIAI2C_ISR_NACK_ADDR)
>  		ret = -EIO;
> @@ -184,6 +208,7 @@ int viai2c_xfer(struct i2c_adapter *adap,
>  	int ret = 0;
>  	struct viai2c *i2c = i2c_get_adapdata(adap);
>  
> +	i2c->to = i2c->ti = ktime_get();
>  	for (i = 0; ret >= 0 && i < num; i++) {
>  		pmsg = &msgs[i];
>  		if (!(pmsg->flags & I2C_M_NOSTART)) {
> diff --git a/drivers/i2c/busses/i2c-viai2c-common.h b/drivers/i2c/busses/i2c-viai2c-common.h
> index f171f81e4d0f..73a88398d763 100644
> --- a/drivers/i2c/busses/i2c-viai2c-common.h
> +++ b/drivers/i2c/busses/i2c-viai2c-common.h
> @@ -58,6 +58,8 @@ struct viai2c {
>  	u16			tcr;
>  	int			irq;
>  	u16			cmd_status;
> +	ktime_t			ti;
> +	ktime_t			to;

don't these need some arbitration?

Andi

On 2024/1/4 03:39, Andi Shyti wrote:
> Hi Hans,
>
> [...]
>
>>   static int viai2c_wait_bus_ready(struct viai2c *i2c)
>>   {
>> @@ -25,12 +26,35 @@ static int viai2c_wait_bus_ready(struct viai2c *i2c)
>>   static int viai2c_wait_status(struct viai2c *i2c)
>>   {
>>   	int ret = 0;
>> -	unsigned long wait_result;
>> +	unsigned long time_left;
>> +	unsigned long delta_ms;
>> +
>> +	time_left = wait_for_completion_timeout(&i2c->complete,
>> +						VIAI2C_TIMEOUT);
>> +	if (!time_left) {
>> +		dev_err(i2c->dev, "bus transfer timeout\n");
>> +		return -EIO;
>> +	}
>>   
>> -	wait_result = wait_for_completion_timeout(&i2c->complete,
>> -						msecs_to_jiffies(500));
>> -	if (!wait_result)
>> -		return -ETIMEDOUT;
> this change is unrelated. Why are you changing the timeout here
> from 500 to 1000?


it does unrelated, will drop it.


>> +	/*
>> +	 * During each byte access, the host performs clock stretching.
>> +	 * In this case, the thread may be interrupted by preemption,
>> +	 * resulting in a long stretching time.
>> +	 * However, some touchpad can only tolerate host clock stretching
>> +	 * of no more than 200 ms. We reduce the impact of this through
>> +	 * a retransmission mechanism.
>> +	 */
> is the hardware sending the stretching on its own?
>

Yes, controller will handle it.


>> +	local_irq_disable();
>> +	i2c->to = ktime_get();
>> +	delta_ms = ktime_to_ms(ktime_sub(i2c->to, i2c->ti));
>> +	if (delta_ms > VIAI2C_STRETCHING_TIMEOUT) {
>> +		local_irq_enable();
>> +		dev_warn(i2c->dev, "thread blocked more than %ldms\n",
>> +				delta_ms);
>> +		return -EAGAIN;
>> +	}
>> +	i2c->ti = i2c->to;
>> +	local_irq_enable();
>>   
>>   	if (i2c->cmd_status & VIAI2C_ISR_NACK_ADDR)
>>   		ret = -EIO;
>> @@ -184,6 +208,7 @@ int viai2c_xfer(struct i2c_adapter *adap,
>>   	int ret = 0;
>>   	struct viai2c *i2c = i2c_get_adapdata(adap);
>>   
>> +	i2c->to = i2c->ti = ktime_get();
>>   	for (i = 0; ret >= 0 && i < num; i++) {
>>   		pmsg = &msgs[i];
>>   		if (!(pmsg->flags & I2C_M_NOSTART)) {
>> diff --git a/drivers/i2c/busses/i2c-viai2c-common.h b/drivers/i2c/busses/i2c-viai2c-common.h
>> index f171f81e4d0f..73a88398d763 100644
>> --- a/drivers/i2c/busses/i2c-viai2c-common.h
>> +++ b/drivers/i2c/busses/i2c-viai2c-common.h
>> @@ -58,6 +58,8 @@ struct viai2c {
>>   	u16			tcr;
>>   	int			irq;
>>   	u16			cmd_status;
>> +	ktime_t			ti;
>> +	ktime_t			to;
> don't these need some arbitration?
>
> Andi


I don't think it needs to be arbitration.
the controllers are independent of each other,
each access is locked using __i2c_lock_bus_helper().
Am I missing something?


Hans

Hi Hans,

...

> > > @@ -58,6 +58,8 @@ struct viai2c {
> > >   	u16			tcr;
> > >   	int			irq;
> > >   	u16			cmd_status;
> > > +	ktime_t			ti;
> > > +	ktime_t			to;
> > don't these need some arbitration?
> > 
> 
> I don't think it needs to be arbitration.
> the controllers are independent of each other,
> each access is locked using __i2c_lock_bus_helper().
> Am I missing something?

no, it's fine, it's me who missed that. Do you mind writing a
comment?

Thanks,
Andi