Re: [PATCH V4 6/6] i2c: tegra: remove BUG, BUG_ON

[Dmitry Osipenko <digetx@gmail.com>]

10.06.2019 22:41, Bitan Biswas пишет:
> 
> 
> On 6/10/19 11:12 AM, Dmitry Osipenko wrote:
>> 10.06.2019 20:08, Bitan Biswas пишет:
>>> Remove redundant BUG_ON calls or replace with WARN_ON_ONCE
>>> as needed. Remove BUG() and make Rx and Tx case handling
>>> similar.
>>>
>>> Signed-off-by: Bitan Biswas <bbiswas@nvidia.com>
>>> ---
>>>   drivers/i2c/busses/i2c-tegra.c | 11 ++++++-----
>>>   1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> Looks that this is still not correct. What if it transfer-complete flag
>> is set and buffer is full on RX? In this case the transfer will succeed
>> while it was a failure.
>>
>>> diff --git a/drivers/i2c/busses/i2c-tegra.c
>>> b/drivers/i2c/busses/i2c-tegra.c
>>> index 4dfb4c1..30619d6 100644
>>> --- a/drivers/i2c/busses/i2c-tegra.c
>>> +++ b/drivers/i2c/busses/i2c-tegra.c
>>> @@ -515,7 +515,6 @@ static int tegra_i2c_empty_rx_fifo(struct
>>> tegra_i2c_dev *i2c_dev)
>>>        * prevent overwriting past the end of buf
>>>        */
>>>       if (rx_fifo_avail > 0 && buf_remaining > 0) {
>>> -        BUG_ON(buf_remaining > 3);
>>
>> Actually error should be returned here since out-of-bounds memory
>> accesses must be avoided, hence:
>>
>>     if (WARN_ON_ONCE(buf_remaining > 3))
>>         return -EINVAL;
> buf_remaining will be less than equal to 3 because of the expression
> earlier
> https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L520
> 

Ah yes, indeed!

> 
>>
>>>           val = i2c_readl(i2c_dev, I2C_RX_FIFO);
>>>           val = cpu_to_le32(val);
>>>           memcpy(buf, &val, buf_remaining);
>>> @@ -523,7 +522,6 @@ static int tegra_i2c_empty_rx_fifo(struct
>>> tegra_i2c_dev *i2c_dev)
>>>           rx_fifo_avail--;
>>>       }
>>>   -    BUG_ON(rx_fifo_avail > 0 && buf_remaining > 0);
>>
>> Better not to ignore this as well:
>>
>>     if (WARN_ON_ONCE(rx_fifo_avail > 0 &&
>>              buf_remaining > 0))
>>         return -EINVAL;
>>
> Please check below line.
> https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L532
> 
> 
> It ensures that buf_remaining will be 0 and we never hit the BUG_ON as
> follows:

[1] Okay, but it doesn't ensure about rx_fifo_avail. So it could be:

	if (WARN_ON_ONCE(rx_fifo_avail))
		return -EINVAL;

>>> -    BUG_ON(rx_fifo_avail > 0 && buf_remaining > 0);
> 
>>>       i2c_dev->msg_buf_remaining = buf_remaining;
>>>       i2c_dev->msg_buf = buf;
>>>   @@ -581,7 +579,6 @@ static int tegra_i2c_fill_tx_fifo(struct
>>> tegra_i2c_dev *i2c_dev)
>>>        * boundary and fault.
>>>        */
>>>       if (tx_fifo_avail > 0 && buf_remaining > 0) {
>>> -        BUG_ON(buf_remaining > 3);
>>
>> And here, cause this will corrupt stack:
>>
>>         if (WARN_ON_ONCE(buf_remaining > 3))
>>             return -EINVAL;
>>
> Please check the line
> https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L576
> 
> 
> It ensures buf_remaining will be less or equal to 3.

Okay, agree here.

>>>           memcpy(&val, buf, buf_remaining);
>>>           val = le32_to_cpu(val);
>>>   @@ -850,7 +847,8 @@ static irqreturn_t tegra_i2c_isr(int irq, void
>>> *dev_id)
>>>               if (i2c_dev->msg_buf_remaining)
>>>                   tegra_i2c_empty_rx_fifo(i2c_dev);
>>>               else
>>> -                BUG();
>>> +                tegra_i2c_mask_irq(i2c_dev,
>>> +                           I2C_INT_RX_FIFO_DATA_REQ);
>>
>> Then here:
>>
>>     if (WARN_ON_ONCE(!i2c_dev->msg_buf_remaining) ||
>>         tegra_i2c_empty_rx_fifo(i2c_dev)) {
>>         i2c_dev->msg_err |= I2C_ERR_UNKNOWN_INTERRUPT;
>>         goto err;
>>     }
>>
> Can you please elaborate why the condition needs to be as follows
> instead of " if (WARN_ON_ONCE(i2c_dev->msg_buf_remaining)) " ?
> 
>>     if (WARN_ON_ONCE(!i2c_dev->msg_buf_remaining) ||
>>         tegra_i2c_empty_rx_fifo(i2c_dev)) {

Because this is a "receive" transfer and hence it is a error condition
if the data-message was already fully received and then there is another
request from hardware to receive more data. So
"!i2c_dev->msg_buf_remaining" is the error condition here because there
is no more space in the buffer.

Looking at this again, seems checking for "if
(WARN_ON_ONCE(rx_fifo_avail))" in the above hunk [1] will be already
enough since a not fully drained RX FIFO means that there is no enough
space in the buffer. Then it could be:

        if (tegra_i2c_empty_rx_fifo(i2c_dev)) {
                i2c_dev->msg_err |= I2C_ERR_UNKNOWN_INTERRUPT;
                goto err;
	}