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

[Dmitry Osipenko]

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@xxxxxxxxxx>
>>> ---
>>> Â 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;
	}