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
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:
>> - 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.
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)) {
-regards,
Bitan
