Re: [PATCH 4/5] i2c: xiic: Switch from waitqueue to completion

[Marek Vasut <marex@denx.de>]

On 6/29/20 2:53 PM, Raviteja Narayanam wrote:

Hi,

[...]

>>>> @@ -703,23 +704,24 @@ static int xiic_xfer(struct i2c_adapter *adap,
>>>> struct i2c_msg *msgs, int num)
>>>>  	err = xiic_start_xfer(i2c, msgs, num);
>>>>  	if (err < 0) {
>>>>  		dev_err(adap->dev.parent, "Error xiic_start_xfer\n");
>>>> -		goto out;
>>>> +		return err;
>>>>  	}
>>>>
>>>> -	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
>>>> -		(i2c->state == STATE_DONE), HZ)) {
>>>> -		mutex_lock(&i2c->lock);
>>>> -		err = (i2c->state == STATE_DONE) ? num : -EIO;
>>>> -		goto out;
>>>> -	} else {
>>>> -		mutex_lock(&i2c->lock);
>>>> +	err = wait_for_completion_interruptible_timeout(&i2c->completion,
>>>> +							XIIC_I2C_TIMEOUT);
>>>
>>> This is causing random lock up of bus. Since it is "interruptible"
>>> API,  once we enter Ctrl+C, It is coming out of wait state, the message
>> pointers are made NULL and the ongoing transaction is not completed.
>>> Can use " wait_for_completion_timeout" API in place of this.
>>>
>>>> +	mutex_lock(&i2c->lock);
>>
>> So in case this is interrupted, we would have to somehow reset the controller ?
> 
> Yes, but the cleanup is not straight forward because we do not know the exact state
> Of controller and the I2C bus. That’s why preferring a non-interruptible API.

Ah, right, that makes sense, thanks.

>> What sort of lockups do you see exactly ? 
> 
> There is an i2ctransfer going on (let's say a read of 255 bytes), we get interrupt everytime the Rx FIFO is full.
> While the controller is receiving data, if there is a signal, this is exiting abruptly and there is no STOP condition
> on the bus. So, no master can communicate on that bus further. 
> 
> Can you share some sort of test case,
>> so I can replicate it ?
> 
> I have 3 scripts running with commands like below, and I am randomly giving Ctrl+C.

OK, thanks.