[PATCH V4 1/6] i2c: tegra: clean up macros

[PATCH V4 1/6] i2c: tegra: clean up macros

[Bitan Biswas]

Clean up macros by:
1) removing unused macros
2) replace constants by macro BIT()

Signed-off-by: Bitan Biswas <bbiswas@nvidia.com>
Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/i2c/busses/i2c-tegra.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 1dbba39..00692d8 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -54,20 +54,15 @@
 #define I2C_INT_STATUS				0x068
 #define I2C_INT_BUS_CLR_DONE			BIT(11)
 #define I2C_INT_PACKET_XFER_COMPLETE		BIT(7)
-#define I2C_INT_ALL_PACKETS_XFER_COMPLETE	BIT(6)
-#define I2C_INT_TX_FIFO_OVERFLOW		BIT(5)
-#define I2C_INT_RX_FIFO_UNDERFLOW		BIT(4)
 #define I2C_INT_NO_ACK				BIT(3)
 #define I2C_INT_ARBITRATION_LOST		BIT(2)
 #define I2C_INT_TX_FIFO_DATA_REQ		BIT(1)
 #define I2C_INT_RX_FIFO_DATA_REQ		BIT(0)
 #define I2C_CLK_DIVISOR				0x06c
 #define I2C_CLK_DIVISOR_STD_FAST_MODE_SHIFT	16
-#define I2C_CLK_MULTIPLIER_STD_FAST_MODE	8
 
 #define DVC_CTRL_REG1				0x000
 #define DVC_CTRL_REG1_INTR_EN			BIT(10)
-#define DVC_CTRL_REG2				0x004
 #define DVC_CTRL_REG3				0x008
 #define DVC_CTRL_REG3_SW_PROG			BIT(26)
 #define DVC_CTRL_REG3_I2C_DONE_INTR_EN		BIT(30)
@@ -75,24 +70,21 @@
 #define DVC_STATUS_I2C_DONE_INTR		BIT(30)
 
 #define I2C_ERR_NONE				0x00
-#define I2C_ERR_NO_ACK				0x01
-#define I2C_ERR_ARBITRATION_LOST		0x02
-#define I2C_ERR_UNKNOWN_INTERRUPT		0x04
+#define I2C_ERR_NO_ACK				BIT(0)
+#define I2C_ERR_ARBITRATION_LOST		BIT(1)
+#define I2C_ERR_UNKNOWN_INTERRUPT		BIT(2)
 
 #define PACKET_HEADER0_HEADER_SIZE_SHIFT	28
 #define PACKET_HEADER0_PACKET_ID_SHIFT		16
 #define PACKET_HEADER0_CONT_ID_SHIFT		12
 #define PACKET_HEADER0_PROTOCOL_I2C		BIT(4)
 
-#define I2C_HEADER_HIGHSPEED_MODE		BIT(22)
 #define I2C_HEADER_CONT_ON_NAK			BIT(21)
-#define I2C_HEADER_SEND_START_BYTE		BIT(20)
 #define I2C_HEADER_READ				BIT(19)
 #define I2C_HEADER_10BIT_ADDR			BIT(18)
 #define I2C_HEADER_IE_ENABLE			BIT(17)
 #define I2C_HEADER_REPEAT_START			BIT(16)
 #define I2C_HEADER_CONTINUE_XFER		BIT(15)
-#define I2C_HEADER_MASTER_ADDR_SHIFT		12
 #define I2C_HEADER_SLAVE_ADDR_SHIFT		1
 
 #define I2C_BUS_CLEAR_CNFG			0x084
@@ -106,8 +98,6 @@
 
 #define I2C_CONFIG_LOAD				0x08C
 #define I2C_MSTR_CONFIG_LOAD			BIT(0)
-#define I2C_SLV_CONFIG_LOAD			BIT(1)
-#define I2C_TIMEOUT_CONFIG_LOAD			BIT(2)
 
 #define I2C_CLKEN_OVERRIDE			0x090
 #define I2C_MST_CORE_CLKEN_OVR			BIT(0)
@@ -133,7 +123,6 @@
 #define I2C_STANDARD_MODE			100000
 #define I2C_FAST_MODE				400000
 #define I2C_FAST_PLUS_MODE			1000000
-#define I2C_HS_MODE				3500000
 
 /* Packet header size in bytes */
 #define I2C_PACKET_HEADER_SIZE			12
-- 
2.7.4

[PATCH V4 2/6] i2c: tegra: remove unnecessary variable init

[Bitan Biswas]

Remove variable initializations in functions that
are followed by assignments before use

Signed-off-by: Bitan Biswas <bbiswas@nvidia.com>
Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/i2c/busses/i2c-tegra.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 00692d8..f7116b7 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -689,7 +689,7 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev, bool clk_reinit)
 	u32 val;
 	int err;
 	u32 clk_divisor, clk_multiplier;
-	u32 tsu_thd = 0;
+	u32 tsu_thd;
 	u8 tlow, thigh;
 
 	err = pm_runtime_get_sync(i2c_dev->dev);
@@ -1218,7 +1218,7 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 {
 	struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
 	int i;
-	int ret = 0;
+	int ret;
 
 	ret = pm_runtime_get_sync(i2c_dev->dev);
 	if (ret < 0) {
@@ -1489,7 +1489,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	void __iomem *base;
 	phys_addr_t base_phys;
 	int irq;
-	int ret = 0;
+	int ret;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	base_phys = res->start;
-- 
2.7.4

[PATCH V4 3/6] i2c: tegra: fix alignment and spacing violations

[Bitan Biswas]

Fix checkpatch.pl alignment and blank line check(s) in i2c-tegra.c

Signed-off-by: Bitan Biswas <bbiswas@nvidia.com>
Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/i2c/busses/i2c-tegra.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index f7116b7..2d381de 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -295,7 +295,7 @@ static u32 dvc_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg)
  * to the I2C block inside the DVC block
  */
 static unsigned long tegra_i2c_reg_addr(struct tegra_i2c_dev *i2c_dev,
-	unsigned long reg)
+					unsigned long reg)
 {
 	if (i2c_dev->is_dvc)
 		reg += (reg >= I2C_TX_FIFO) ? 0x10 : 0x40;
@@ -303,7 +303,7 @@ static unsigned long tegra_i2c_reg_addr(struct tegra_i2c_dev *i2c_dev,
 }
 
 static void i2c_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
-	unsigned long reg)
+		       unsigned long reg)
 {
 	writel(val, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
 
@@ -318,13 +318,13 @@ static u32 i2c_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg)
 }
 
 static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, void *data,
-	unsigned long reg, int len)
+			unsigned long reg, int len)
 {
 	writesl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
 }
 
 static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
-	unsigned long reg, int len)
+		       unsigned long reg, int len)
 {
 	readsl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
 }
@@ -669,10 +669,11 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
 		i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
 		if (in_interrupt())
 			err = readl_poll_timeout_atomic(addr, val, val == 0,
-					1000, I2C_CONFIG_LOAD_TIMEOUT);
+							1000,
+							I2C_CONFIG_LOAD_TIMEOUT);
 		else
-			err = readl_poll_timeout(addr, val, val == 0,
-					1000, I2C_CONFIG_LOAD_TIMEOUT);
+			err = readl_poll_timeout(addr, val, val == 0, 1000,
+						 I2C_CONFIG_LOAD_TIMEOUT);
 
 		if (err) {
 			dev_warn(i2c_dev->dev,
@@ -1013,7 +1014,8 @@ static int tegra_i2c_issue_bus_clear(struct i2c_adapter *adap)
 }
 
 static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
-	struct i2c_msg *msg, enum msg_end_type end_state)
+			      struct i2c_msg *msg,
+			      enum msg_end_type end_state)
 {
 	u32 packet_header;
 	u32 int_mask;
@@ -1150,9 +1152,8 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 		if (err)
 			return err;
 
-		time_left = wait_for_completion_timeout(
-						&i2c_dev->dma_complete,
-						msecs_to_jiffies(xfer_time));
+		time_left = wait_for_completion_timeout(&i2c_dev->dma_complete,
+							msecs_to_jiffies(xfer_time));
 		if (time_left == 0) {
 			dev_err(i2c_dev->dev, "DMA transfer timeout\n");
 			dmaengine_terminate_sync(i2c_dev->msg_read ?
@@ -1214,7 +1215,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 }
 
 static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
-	int num)
+			  int num)
 {
 	struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
 	int i;
@@ -1260,14 +1261,15 @@ static void tegra_i2c_parse_dt(struct tegra_i2c_dev *i2c_dev)
 {
 	struct device_node *np = i2c_dev->dev->of_node;
 	int ret;
+	bool multi_mode;
 
 	ret = of_property_read_u32(np, "clock-frequency",
-			&i2c_dev->bus_clk_rate);
+				   &i2c_dev->bus_clk_rate);
 	if (ret)
 		i2c_dev->bus_clk_rate = 100000; /* default clock rate */
 
-	i2c_dev->is_multimaster_mode = of_property_read_bool(np,
-			"multi-master");
+	multi_mode = of_property_read_bool(np, "multi-master");
+	i2c_dev->is_multimaster_mode = multi_mode;
 }
 
 static const struct i2c_algorithm tegra_i2c_algo = {
@@ -1611,7 +1613,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	}
 
 	ret = devm_request_irq(&pdev->dev, i2c_dev->irq,
-			tegra_i2c_isr, 0, dev_name(&pdev->dev), i2c_dev);
+			       tegra_i2c_isr, 0, dev_name(&pdev->dev), i2c_dev);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
 		goto release_dma;
@@ -1704,6 +1706,7 @@ static const struct dev_pm_ops tegra_i2c_pm = {
 	SET_RUNTIME_PM_OPS(tegra_i2c_runtime_suspend, tegra_i2c_runtime_resume,
 			   NULL)
 };
+
 #define TEGRA_I2C_PM	(&tegra_i2c_pm)
 #else
 #define TEGRA_I2C_PM	NULL
-- 
2.7.4

[PATCH V4 4/6] i2c: tegra: add spinlock definition comment

[Bitan Biswas]

Fix checkpatch.pl CHECK as follows:
CHECK: spinlock_t definition without comment
+       spinlock_t xfer_lock;

Signed-off-by: Bitan Biswas <bbiswas@nvidia.com>
Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/i2c/busses/i2c-tegra.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 2d381de..bececa6 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -269,6 +269,7 @@ struct tegra_i2c_dev {
 	u32 bus_clk_rate;
 	u16 clk_divisor_non_hs_mode;
 	bool is_multimaster_mode;
+	/* xfer_lock: lock to serialize transfer submission and processing */
 	spinlock_t xfer_lock;
 	struct dma_chan *tx_dma_chan;
 	struct dma_chan *rx_dma_chan;
-- 
2.7.4

[PATCH V4 5/6] i2c: tegra: fix msleep warning

[Bitan Biswas]

Fix checkpatch.pl WARNING for delay of approximately 1msec
in flush i2c FIFO polling loop by using usleep_range(1000, 2000):
WARNING: msleep < 20ms can sleep for up to 20ms; see ...
Documentation/timers/timers-howto.txt
+               msleep(1);

Signed-off-by: Bitan Biswas <bbiswas@nvidia.com>
Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/i2c/busses/i2c-tegra.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index bececa6..4dfb4c1 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -476,7 +476,7 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
 			dev_warn(i2c_dev->dev, "timeout waiting for fifo flush\n");
 			return -ETIMEDOUT;
 		}
-		msleep(1);
+		usleep_range(1000, 2000);
 	}
 	return 0;
 }
-- 
2.7.4

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

[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(-)

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);
 		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);
 	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);
 		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);
 		}
 
 		if (!i2c_dev->msg_read && (status & I2C_INT_TX_FIFO_DATA_REQ)) {
@@ -876,7 +874,10 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 	if (status & I2C_INT_PACKET_XFER_COMPLETE) {
 		if (i2c_dev->is_curr_dma_xfer)
 			i2c_dev->msg_buf_remaining = 0;
-		BUG_ON(i2c_dev->msg_buf_remaining);
+		if (WARN_ON_ONCE(i2c_dev->msg_buf_remaining)) {
+			i2c_dev->msg_err |= I2C_ERR_UNKNOWN_INTERRUPT;
+			goto err;
+		}
 		complete(&i2c_dev->msg_complete);
 	}
 	goto done;
-- 
2.7.4

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

[Dmitry Osipenko]

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;

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

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

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

>  		}
>  
>  		if (!i2c_dev->msg_read && (status & I2C_INT_TX_FIFO_DATA_REQ)) {
> @@ -876,7 +874,10 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>  	if (status & I2C_INT_PACKET_XFER_COMPLETE) {
>  		if (i2c_dev->is_curr_dma_xfer)
>  			i2c_dev->msg_buf_remaining = 0;
> -		BUG_ON(i2c_dev->msg_buf_remaining);
> +		if (WARN_ON_ONCE(i2c_dev->msg_buf_remaining)) {
> +			i2c_dev->msg_err |= I2C_ERR_UNKNOWN_INTERRUPT;
> +			goto err;
> +		}
>  		complete(&i2c_dev->msg_complete);
>  	}
>  	goto done;
> 

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

[Dmitry Osipenko]

10.06.2019 21:12, Dmitry Osipenko пишет:
> 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;
> 
>>  		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;
> 
>>  	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;
> 
>>  		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) ||

Also note that this check could be moved out to the beginning of
tegra_i2c_empty_rx_fifo().

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

[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

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

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

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

[Bitan Biswas]

On 6/10/19 2:00 PM, Dmitry Osipenko wrote:
> 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!
> 
I see that I am wrong and buf_remaining > 3 needs to be prevented at

https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L528

because of word_to_transfer is limited to rx_fifo_avail:

https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L515

I shall add the check for less than 3 in both RX and TX cases in a 
separate patch in this series.

>>
>>>
>>>>            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;
I shall add the WARN_ON_ONCE.


> 
>>>> -    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;
> 	}
> 
In the case "if (status & I2C_INT_PACKET_XFER_COMPLETE) {" , we do not 
have any tegra_i2c_empty_rx_fifo call today. In this current driver I do 
not see any code that checks for the buffer space and prevents RX FIFO 
from being drained. The transfer complete when seen must have already 
consumed all bytes of msg_buf_remaining in the call at the line

https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L860

So we can put the "WARN_ON_ONCE(i2c_dev->msg_buf_remaining) with msg_err 
assignment and goto err" to confirm if some corner case is not handled.

Planning to share updated patch.

-Thanks,
  Bitan

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

[Dmitry Osipenko]

11.06.2019 10:38, Bitan Biswas пишет:
> 
> 
> On 6/10/19 2:00 PM, Dmitry Osipenko wrote:
>> 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!
>>
> I see that I am wrong and buf_remaining > 3 needs to be prevented at
> 
> https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L528
> 
> 
> because of word_to_transfer is limited to rx_fifo_avail:
> 
> https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L515
> 
> 
> I shall add the check for less than 3 in both RX and TX cases in a
> separate patch in this series.

When word_to_transfer is more than rx_fifo_avail, then the rx_fifo_avail
becomes zero and hence the nibbles won't be copied. Please take a closer
look, the current code is correct, but the buf_remaining > 3 is unneeded
because it can't ever happen.

The code is structured the way that it's difficult to follow, apparently
the person who added the BUG_ON check in the first place couldn't follow
it either. Maybe it's worth to invest some more effort into refactoring
at least that part of the code. At minimum a clarifying comments would
be helpful.

[snip]

>>>> 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;
>>     }
>>
> In the case "if (status & I2C_INT_PACKET_XFER_COMPLETE) {" , we do not
> have any tegra_i2c_empty_rx_fifo call today. In this current driver I do
> not see any code that checks for the buffer space and prevents RX FIFO
> from being drained. The transfer complete when seen must have already
> consumed all bytes of msg_buf_remaining in the call at the line
> 
> https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L860
> 
> 
> So we can put the "WARN_ON_ONCE(i2c_dev->msg_buf_remaining) with msg_err
> assignment and goto err" to confirm if some corner case is not handled.
> 
> Planning to share updated patch.

There are two possible error conditions:

1) Underflow: the XFER_COMPLETE happens before message is fully sent.

2) Overflow: message is fully sent, but there is no XFER_COMPLETE and
then hardware asks to transfer more.

We are addressing the second case here, while you seems are confusing it
with the first case.

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

[Bitan Biswas]

On 6/11/19 4:34 AM, Dmitry Osipenko wrote:
> 11.06.2019 10:38, Bitan Biswas пишет:
>>
>>
>> On 6/10/19 2:00 PM, Dmitry Osipenko wrote:
>>> 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!
>>>
>> I see that I am wrong and buf_remaining > 3 needs to be prevented at
>>
>> https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L528
>>
>>
>> because of word_to_transfer is limited to rx_fifo_avail:
>>
>> https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L515
>>
>>
>> I shall add the check for less than 3 in both RX and TX cases in a
>> separate patch in this series.
> 
> When word_to_transfer is more than rx_fifo_avail, then the rx_fifo_avail
> becomes zero and hence the nibbles won't be copied. Please take a closer
> look, the current code is correct, but the buf_remaining > 3 is unneeded
> because it can't ever happen.
> 
> The code is structured the way that it's difficult to follow, apparently
> the person who added the BUG_ON check in the first place couldn't follow
> it either. Maybe it's worth to invest some more effort into refactoring
> at least that part of the code. At minimum a clarifying comments would
> be helpful.
> 
I shall try to add some comments near the BUG_ON check.

> [snip]
> 
>>>>> 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;
>>>      }
>>>
>> In the case "if (status & I2C_INT_PACKET_XFER_COMPLETE) {" , we do not
>> have any tegra_i2c_empty_rx_fifo call today. In this current driver I do
>> not see any code that checks for the buffer space and prevents RX FIFO
>> from being drained. The transfer complete when seen must have already
>> consumed all bytes of msg_buf_remaining in the call at the line
>>
>> https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L860
>>
>>
>> So we can put the "WARN_ON_ONCE(i2c_dev->msg_buf_remaining) with msg_err
>> assignment and goto err" to confirm if some corner case is not handled.
>>
>> Planning to share updated patch.
> 
> There are two possible error conditions:
> 
> 1) Underflow: the XFER_COMPLETE happens before message is fully sent.
> 
> 2) Overflow: message is fully sent, but there is no XFER_COMPLETE and
> then hardware asks to transfer more.
> 
> We are addressing the second case here, while you seems are confusing it
> with the first case.
> 
Is the Overflow case pointed above corresponding to when 
msg_buf_remaining is zero? If no, what indicates that message is fully 
sent? I see that if msg_buf_remaining is already zero, the call 
tegra_i2c_empty_rx_fifo will not do any copy of the bytes from FIFO to buf.

One more point that is not clear to me is are the above suggestions you 
made is corresponding to replacing below line in linux-next ?

https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L888

Can you please also review the newly added patch "V5 6/7 "that was newly 
posted? I think it is needed.


-regards,
  Bitan

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

[Dmitry Osipenko]

11.06.2019 21:22, Bitan Biswas пишет:
> 
> 
> On 6/11/19 4:34 AM, Dmitry Osipenko wrote:
>> 11.06.2019 10:38, Bitan Biswas пишет:
>>>
>>>
>>> On 6/10/19 2:00 PM, Dmitry Osipenko wrote:
>>>> 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!
>>>>
>>> I see that I am wrong and buf_remaining > 3 needs to be prevented at
>>>
>>> https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L528
>>>
>>>
>>>
>>> because of word_to_transfer is limited to rx_fifo_avail:
>>>
>>> https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L515
>>>
>>>
>>>
>>> I shall add the check for less than 3 in both RX and TX cases in a
>>> separate patch in this series.
>>
>> When word_to_transfer is more than rx_fifo_avail, then the rx_fifo_avail
>> becomes zero and hence the nibbles won't be copied. Please take a closer
>> look, the current code is correct, but the buf_remaining > 3 is unneeded
>> because it can't ever happen.
>>
>> The code is structured the way that it's difficult to follow, apparently
>> the person who added the BUG_ON check in the first place couldn't follow
>> it either. Maybe it's worth to invest some more effort into refactoring
>> at least that part of the code. At minimum a clarifying comments would
>> be helpful.
>>
> I shall try to add some comments near the BUG_ON check.
> 
>> [snip]
>>
>>>>>> 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;
>>>>      }
>>>>
>>> In the case "if (status & I2C_INT_PACKET_XFER_COMPLETE) {" , we do not
>>> have any tegra_i2c_empty_rx_fifo call today. In this current driver I do
>>> not see any code that checks for the buffer space and prevents RX FIFO
>>> from being drained. The transfer complete when seen must have already
>>> consumed all bytes of msg_buf_remaining in the call at the line
>>>
>>> https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L860
>>>
>>>
>>>
>>> So we can put the "WARN_ON_ONCE(i2c_dev->msg_buf_remaining) with msg_err
>>> assignment and goto err" to confirm if some corner case is not handled.
>>>
>>> Planning to share updated patch.
>>
>> There are two possible error conditions:
>>
>> 1) Underflow: the XFER_COMPLETE happens before message is fully sent.
>>
>> 2) Overflow: message is fully sent, but there is no XFER_COMPLETE and
>> then hardware asks to transfer more.
>>
>> We are addressing the second case here, while you seems are confusing it
>> with the first case.
>>
> Is the Overflow case pointed above corresponding to when
> msg_buf_remaining is zero?

Yes!

 If no, what indicates that message is fully
> sent? I see that if msg_buf_remaining is already zero, the call
> tegra_i2c_empty_rx_fifo will not do any copy of the bytes from FIFO to buf.
> 
> One more point that is not clear to me is are the above suggestions you
> made is corresponding to replacing below line in linux-next ?
> 
> https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L888

That addresses the "underflow" case. I'm not suggesting to replace it at
all. I was talking about replacing this and nothing else:

https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L862

> Can you please also review the newly added patch "V5 6/7 "that was newly
> posted? I think it is needed.

Sure.

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

[Bitan Biswas]

On 6/12/19 6:33 AM, Dmitry Osipenko wrote:
> 11.06.2019 21:22, Bitan Biswas пишет:
>>
>>
>> On 6/11/19 4:34 AM, Dmitry Osipenko wrote:
>>> 11.06.2019 10:38, Bitan Biswas пишет:
>>>>
>>>>
>>>> On 6/10/19 2:00 PM, Dmitry Osipenko wrote:
>>>>> 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!
>>>>>
>>>> I see that I am wrong and buf_remaining > 3 needs to be prevented at
>>>>
>>>> https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L528
>>>>
>>>>
>>>>
>>>> because of word_to_transfer is limited to rx_fifo_avail:
>>>>
>>>> https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L515
>>>>
>>>>
>>>>
>>>> I shall add the check for less than 3 in both RX and TX cases in a
>>>> separate patch in this series.
>>>
>>> When word_to_transfer is more than rx_fifo_avail, then the rx_fifo_avail
>>> becomes zero and hence the nibbles won't be copied. Please take a closer
>>> look, the current code is correct, but the buf_remaining > 3 is unneeded
>>> because it can't ever happen.
>>>
>>> The code is structured the way that it's difficult to follow, apparently
>>> the person who added the BUG_ON check in the first place couldn't follow
>>> it either. Maybe it's worth to invest some more effort into refactoring
>>> at least that part of the code. At minimum a clarifying comments would
>>> be helpful.
>>>
>> I shall try to add some comments near the BUG_ON check.
>>
>>> [snip]
>>>
>>>>>>> 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;
>>>>>       }
>>>>>
>>>> In the case "if (status & I2C_INT_PACKET_XFER_COMPLETE) {" , we do not
>>>> have any tegra_i2c_empty_rx_fifo call today. In this current driver I do
>>>> not see any code that checks for the buffer space and prevents RX FIFO
>>>> from being drained. The transfer complete when seen must have already
>>>> consumed all bytes of msg_buf_remaining in the call at the line
>>>>
>>>> https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L860
>>>>
>>>>
>>>>
>>>> So we can put the "WARN_ON_ONCE(i2c_dev->msg_buf_remaining) with msg_err
>>>> assignment and goto err" to confirm if some corner case is not handled.
>>>>
>>>> Planning to share updated patch.
>>>
>>> There are two possible error conditions:
>>>
>>> 1) Underflow: the XFER_COMPLETE happens before message is fully sent.
>>>
>>> 2) Overflow: message is fully sent, but there is no XFER_COMPLETE and
>>> then hardware asks to transfer more.
>>>
>>> We are addressing the second case here, while you seems are confusing it
>>> with the first case.
>>>
>> Is the Overflow case pointed above corresponding to when
>> msg_buf_remaining is zero?
> 
> Yes!
> 
OK.

>   If no, what indicates that message is fully
>> sent? I see that if msg_buf_remaining is already zero, the call
>> tegra_i2c_empty_rx_fifo will not do any copy of the bytes from FIFO to buf.
>>
>> One more point that is not clear to me is are the above suggestions you
>> made is corresponding to replacing below line in linux-next ?
>>
>> https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L888
> 
> That addresses the "underflow" case. I'm not suggesting to replace it at
> all. I was talking about replacing this and nothing else:
> 
> https://elixir.bootlin.com/linux/v5.2-rc4/source/drivers/i2c/busses/i2c-tegra.c#L862
I would update the patch for the Overflow case around the line 862 pointed.


> 
>> Can you please also review the newly added patch "V5 6/7 "that was newly
>> posted? I think it is needed.
> 
> Sure.
> 
Thanks. Based on your review plan to abandon the patch "V5 6/7".

-regards,
  Bitan