diff mbox series

[4/5] i2c: xiic: Switch from waitqueue to completion

Message ID 20200613150751.114595-4-marex@denx.de
State New
Headers show
Series [1/5] i2c: xiic: Fix broken locking on tx_msg | expand

Commit Message

Marek Vasut June 13, 2020, 3:07 p.m. UTC
There will never be threads queueing up in the xiic_xmit(), use
completion synchronization primitive to wait for the interrupt
handler thread to complete instead as it is much better fit and
there is no need to overload it for this purpose.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Cc: Wolfram Sang <wsa@kernel.org>
---
 drivers/i2c/busses/i2c-xiic.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

Comments

Raviteja Narayanam June 26, 2020, 12:13 p.m. UTC | #1
> -----Original Message-----
> From: linux-i2c-owner@vger.kernel.org <linux-i2c-owner@vger.kernel.org> On
> Behalf Of Marek Vasut
> Sent: Saturday, June 13, 2020 8:38 PM
> To: linux-i2c@vger.kernel.org
> Cc: Marek Vasut <marex@denx.de>; Michal Simek <michals@xilinx.com>;
> Shubhrajyoti Datta <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
> Subject: [PATCH 4/5] i2c: xiic: Switch from waitqueue to completion
> 
> There will never be threads queueing up in the xiic_xmit(), use completion
> synchronization primitive to wait for the interrupt handler thread to complete
> instead as it is much better fit and there is no need to overload it for this
> purpose.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Cc: Wolfram Sang <wsa@kernel.org>
> ---
>  drivers/i2c/busses/i2c-xiic.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index
> 87eca9d46afd9..e4c3427b2f3f5 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -23,7 +23,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
> -#include <linux/wait.h>
> +#include <linux/completion.h>
>  #include <linux/platform_data/i2c-xiic.h>  #include <linux/io.h>  #include
> <linux/slab.h> @@ -48,7 +48,7 @@ enum xiic_endian {
>   * struct xiic_i2c - Internal representation of the XIIC I2C bus
>   * @dev:	Pointer to device structure
>   * @base:	Memory base of the HW registers
> - * @wait:	Wait queue for callers
> + * @completion:	Completion for callers
>   * @adap:	Kernel adapter representation
>   * @tx_msg:	Messages from above to be sent
>   * @lock:	Mutual exclusion
> @@ -63,7 +63,7 @@ enum xiic_endian {
>  struct xiic_i2c {
>  	struct device		*dev;
>  	void __iomem		*base;
> -	wait_queue_head_t	wait;
> +	struct completion	completion;
>  	struct i2c_adapter	adap;
>  	struct i2c_msg		*tx_msg;
>  	struct mutex		lock;
> @@ -365,7 +365,7 @@ static void xiic_wakeup(struct xiic_i2c *i2c, int code)
>  	i2c->rx_msg = NULL;
>  	i2c->nmsgs = 0;
>  	i2c->state = code;
> -	wake_up(&i2c->wait);
> +	complete(&i2c->completion);
>  }
> 
>  static irqreturn_t xiic_process(int irq, void *dev_id) @@ -677,6 +677,7 @@
> static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs, int num)
> 
>  	i2c->tx_msg = msgs;
>  	i2c->nmsgs = num;
> +	init_completion(&i2c->completion);
> 
>  	ret = xiic_reinit(i2c);
>  	if (!ret)
> @@ -703,23 +704,24 @@ static int xiic_xfer(struct i2c_adapter *adap, struct
> i2c_msg *msgs, int num)
>  	err = xiic_start_xfer(i2c, msgs, num);
>  	if (err < 0) {
>  		dev_err(adap->dev.parent, "Error xiic_start_xfer\n");
> -		goto out;
> +		return err;
>  	}
> 
> -	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
> -		(i2c->state == STATE_DONE), HZ)) {
> -		mutex_lock(&i2c->lock);
> -		err = (i2c->state == STATE_DONE) ? num : -EIO;
> -		goto out;
> -	} else {
> -		mutex_lock(&i2c->lock);
> +	err = wait_for_completion_interruptible_timeout(&i2c->completion,
> +							XIIC_I2C_TIMEOUT);

This is causing random lock up of bus. Since it is "interruptible" API,  once we enter Ctrl+C, 
It is coming out of wait state, the message pointers are made NULL and the ongoing transaction is not completed. 
Can use " wait_for_completion_timeout" API in place of this.

> +	mutex_lock(&i2c->lock);
> +	if (err == 0) {	/* Timeout */
>  		i2c->tx_msg = NULL;
>  		i2c->rx_msg = NULL;
>  		i2c->nmsgs = 0;
>  		err = -ETIMEDOUT;
> -		goto out;
> +	} else if (err < 0) {	/* Completion error */
> +		i2c->tx_msg = NULL;
> +		i2c->rx_msg = NULL;
> +		i2c->nmsgs = 0;
> +	} else {
> +		err = (i2c->state == STATE_DONE) ? num : -EIO;
>  	}
> -out:
>  	mutex_unlock(&i2c->lock);
>  	pm_runtime_mark_last_busy(i2c->dev);
>  	pm_runtime_put_autosuspend(i2c->dev);
> @@ -781,7 +783,6 @@ static int xiic_i2c_probe(struct platform_device *pdev)
>  	i2c->adap.dev.of_node = pdev->dev.of_node;
> 
>  	mutex_init(&i2c->lock);
> -	init_waitqueue_head(&i2c->wait);
> 
>  	i2c->clk = devm_clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(i2c->clk)) {

Raviteja N
Marek Vasut June 28, 2020, 11:41 p.m. UTC | #2
On 6/26/20 2:13 PM, Raviteja Narayanam wrote:

Hi,

[...]

>> @@ -703,23 +704,24 @@ static int xiic_xfer(struct i2c_adapter *adap, struct
>> i2c_msg *msgs, int num)
>>  	err = xiic_start_xfer(i2c, msgs, num);
>>  	if (err < 0) {
>>  		dev_err(adap->dev.parent, "Error xiic_start_xfer\n");
>> -		goto out;
>> +		return err;
>>  	}
>>
>> -	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
>> -		(i2c->state == STATE_DONE), HZ)) {
>> -		mutex_lock(&i2c->lock);
>> -		err = (i2c->state == STATE_DONE) ? num : -EIO;
>> -		goto out;
>> -	} else {
>> -		mutex_lock(&i2c->lock);
>> +	err = wait_for_completion_interruptible_timeout(&i2c->completion,
>> +							XIIC_I2C_TIMEOUT);
> 
> This is causing random lock up of bus. Since it is "interruptible" API,  once we enter Ctrl+C, 
> It is coming out of wait state, the message pointers are made NULL and the ongoing transaction is not completed. 
> Can use " wait_for_completion_timeout" API in place of this.
> 
>> +	mutex_lock(&i2c->lock);

So in case this is interrupted, we would have to somehow reset the
controller ? What sort of lockups do you see exactly ? Can you share
some sort of test case, so I can replicate it ?

Thanks

[...]
Raviteja Narayanam June 29, 2020, 12:53 p.m. UTC | #3
> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Monday, June 29, 2020 5:11 AM
> To: Raviteja Narayanam <rna@xilinx.com>; linux-i2c@vger.kernel.org
> Cc: Michal Simek <michals@xilinx.com>; Shubhrajyoti Datta
> <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
> Subject: Re: [PATCH 4/5] i2c: xiic: Switch from waitqueue to completion
> 
> On 6/26/20 2:13 PM, Raviteja Narayanam wrote:
> 
> Hi,
> 
> [...]
> 
> >> @@ -703,23 +704,24 @@ static int xiic_xfer(struct i2c_adapter *adap,
> >> struct i2c_msg *msgs, int num)
> >>  	err = xiic_start_xfer(i2c, msgs, num);
> >>  	if (err < 0) {
> >>  		dev_err(adap->dev.parent, "Error xiic_start_xfer\n");
> >> -		goto out;
> >> +		return err;
> >>  	}
> >>
> >> -	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
> >> -		(i2c->state == STATE_DONE), HZ)) {
> >> -		mutex_lock(&i2c->lock);
> >> -		err = (i2c->state == STATE_DONE) ? num : -EIO;
> >> -		goto out;
> >> -	} else {
> >> -		mutex_lock(&i2c->lock);
> >> +	err = wait_for_completion_interruptible_timeout(&i2c->completion,
> >> +							XIIC_I2C_TIMEOUT);
> >
> > This is causing random lock up of bus. Since it is "interruptible"
> > API,  once we enter Ctrl+C, It is coming out of wait state, the message
> pointers are made NULL and the ongoing transaction is not completed.
> > Can use " wait_for_completion_timeout" API in place of this.
> >
> >> +	mutex_lock(&i2c->lock);
> 
> So in case this is interrupted, we would have to somehow reset the controller ?

Yes, but the cleanup is not straight forward because we do not know the exact state
Of controller and the I2C bus. That’s why preferring a non-interruptible API.

> What sort of lockups do you see exactly ? 

There is an i2ctransfer going on (let's say a read of 255 bytes), we get interrupt everytime the Rx FIFO is full.
While the controller is receiving data, if there is a signal, this is exiting abruptly and there is no STOP condition
on the bus. So, no master can communicate on that bus further. 

Can you share some sort of test case,
> so I can replicate it ?

I have 3 scripts running with commands like below, and I am randomly giving Ctrl+C.

i=0
while [ 1 ]
do
		i2ctransfer -y -f 0 w1@0X54 0X00 r255@0X54
		i=$(expr $i + 1)
        echo "$i"
done

> 
> Thanks
> 
> [...]

Raviteja N
Marek Vasut June 29, 2020, 9:52 p.m. UTC | #4
On 6/29/20 2:53 PM, Raviteja Narayanam wrote:

Hi,

[...]

>>>> @@ -703,23 +704,24 @@ static int xiic_xfer(struct i2c_adapter *adap,
>>>> struct i2c_msg *msgs, int num)
>>>>  	err = xiic_start_xfer(i2c, msgs, num);
>>>>  	if (err < 0) {
>>>>  		dev_err(adap->dev.parent, "Error xiic_start_xfer\n");
>>>> -		goto out;
>>>> +		return err;
>>>>  	}
>>>>
>>>> -	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
>>>> -		(i2c->state == STATE_DONE), HZ)) {
>>>> -		mutex_lock(&i2c->lock);
>>>> -		err = (i2c->state == STATE_DONE) ? num : -EIO;
>>>> -		goto out;
>>>> -	} else {
>>>> -		mutex_lock(&i2c->lock);
>>>> +	err = wait_for_completion_interruptible_timeout(&i2c->completion,
>>>> +							XIIC_I2C_TIMEOUT);
>>>
>>> This is causing random lock up of bus. Since it is "interruptible"
>>> API,  once we enter Ctrl+C, It is coming out of wait state, the message
>> pointers are made NULL and the ongoing transaction is not completed.
>>> Can use " wait_for_completion_timeout" API in place of this.
>>>
>>>> +	mutex_lock(&i2c->lock);
>>
>> So in case this is interrupted, we would have to somehow reset the controller ?
> 
> Yes, but the cleanup is not straight forward because we do not know the exact state
> Of controller and the I2C bus. That’s why preferring a non-interruptible API.

Ah, right, that makes sense, thanks.

>> What sort of lockups do you see exactly ? 
> 
> There is an i2ctransfer going on (let's say a read of 255 bytes), we get interrupt everytime the Rx FIFO is full.
> While the controller is receiving data, if there is a signal, this is exiting abruptly and there is no STOP condition
> on the bus. So, no master can communicate on that bus further. 
> 
> Can you share some sort of test case,
>> so I can replicate it ?
> 
> I have 3 scripts running with commands like below, and I am randomly giving Ctrl+C.

OK, thanks.
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 87eca9d46afd9..e4c3427b2f3f5 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -23,7 +23,7 @@ 
 #include <linux/platform_device.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
-#include <linux/wait.h>
+#include <linux/completion.h>
 #include <linux/platform_data/i2c-xiic.h>
 #include <linux/io.h>
 #include <linux/slab.h>
@@ -48,7 +48,7 @@  enum xiic_endian {
  * struct xiic_i2c - Internal representation of the XIIC I2C bus
  * @dev:	Pointer to device structure
  * @base:	Memory base of the HW registers
- * @wait:	Wait queue for callers
+ * @completion:	Completion for callers
  * @adap:	Kernel adapter representation
  * @tx_msg:	Messages from above to be sent
  * @lock:	Mutual exclusion
@@ -63,7 +63,7 @@  enum xiic_endian {
 struct xiic_i2c {
 	struct device		*dev;
 	void __iomem		*base;
-	wait_queue_head_t	wait;
+	struct completion	completion;
 	struct i2c_adapter	adap;
 	struct i2c_msg		*tx_msg;
 	struct mutex		lock;
@@ -365,7 +365,7 @@  static void xiic_wakeup(struct xiic_i2c *i2c, int code)
 	i2c->rx_msg = NULL;
 	i2c->nmsgs = 0;
 	i2c->state = code;
-	wake_up(&i2c->wait);
+	complete(&i2c->completion);
 }
 
 static irqreturn_t xiic_process(int irq, void *dev_id)
@@ -677,6 +677,7 @@  static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs, int num)
 
 	i2c->tx_msg = msgs;
 	i2c->nmsgs = num;
+	init_completion(&i2c->completion);
 
 	ret = xiic_reinit(i2c);
 	if (!ret)
@@ -703,23 +704,24 @@  static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	err = xiic_start_xfer(i2c, msgs, num);
 	if (err < 0) {
 		dev_err(adap->dev.parent, "Error xiic_start_xfer\n");
-		goto out;
+		return err;
 	}
 
-	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
-		(i2c->state == STATE_DONE), HZ)) {
-		mutex_lock(&i2c->lock);
-		err = (i2c->state == STATE_DONE) ? num : -EIO;
-		goto out;
-	} else {
-		mutex_lock(&i2c->lock);
+	err = wait_for_completion_interruptible_timeout(&i2c->completion,
+							XIIC_I2C_TIMEOUT);
+	mutex_lock(&i2c->lock);
+	if (err == 0) {	/* Timeout */
 		i2c->tx_msg = NULL;
 		i2c->rx_msg = NULL;
 		i2c->nmsgs = 0;
 		err = -ETIMEDOUT;
-		goto out;
+	} else if (err < 0) {	/* Completion error */
+		i2c->tx_msg = NULL;
+		i2c->rx_msg = NULL;
+		i2c->nmsgs = 0;
+	} else {
+		err = (i2c->state == STATE_DONE) ? num : -EIO;
 	}
-out:
 	mutex_unlock(&i2c->lock);
 	pm_runtime_mark_last_busy(i2c->dev);
 	pm_runtime_put_autosuspend(i2c->dev);
@@ -781,7 +783,6 @@  static int xiic_i2c_probe(struct platform_device *pdev)
 	i2c->adap.dev.of_node = pdev->dev.of_node;
 
 	mutex_init(&i2c->lock);
-	init_waitqueue_head(&i2c->wait);
 
 	i2c->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(i2c->clk)) {