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;
}
