diff mbox

[1/3] i2c-cht-wc: Add locking to interrupt / smbus_xfer functions

Message ID 20170814201726.21045-2-hdegoede@redhat.com
State Accepted
Headers show

Commit Message

Hans de Goede Aug. 14, 2017, 8:17 p.m. UTC 
Although unlikely without locking the smbux_xfer function may miss
the nack flag and further fixes in this patch-set add some more
complex constructions which need protection.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/busses/i2c-cht-wc.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

Comments

Andy Shevchenko Aug. 14, 2017, 8:35 p.m. UTC | #1 
On Mon, 2017-08-14 at 22:17 +0200, Hans de Goede wrote:
> Although unlikely without locking the smbux_xfer function may miss
> the nack flag and further fixes in this patch-set add some more
> complex constructions which need protection.
 
> -	if (read_write == I2C_SMBUS_READ) {
> +	ret = 0;
> +	mutex_lock(&adap->adap_lock);
> +	if (adap->nack)
> +		ret = -EIO;
> +	else if (read_write == I2C_SMBUS_READ) {
>  		ret = regmap_read(adap->regmap, CHT_WC_I2C_RDDATA,
> &reg);
> -		if (ret)
> -			return ret;
> -

>  		data->byte = reg;

Can this be moved out to keep logic the same (don't dirt data on error)?

>  	}
> +	mutex_unlock(&adap->adap_lock);
>  
> -	return 0;
> +	return ret;
Hans de Goede Aug. 14, 2017, 8:55 p.m. UTC | #2 
Hi,

On 14-08-17 22:35, Andy Shevchenko wrote:
> On Mon, 2017-08-14 at 22:17 +0200, Hans de Goede wrote:
>> Although unlikely without locking the smbux_xfer function may miss
>> the nack flag and further fixes in this patch-set add some more
>> complex constructions which need protection.
>   
>> -	if (read_write == I2C_SMBUS_READ) {
>> +	ret = 0;
>> +	mutex_lock(&adap->adap_lock);
>> +	if (adap->nack)
>> +		ret = -EIO;
>> +	else if (read_write == I2C_SMBUS_READ) {
>>   		ret = regmap_read(adap->regmap, CHT_WC_I2C_RDDATA,
>> &reg);
>> -		if (ret)
>> -			return ret;
>> -
> 
>>   		data->byte = reg;
> 
> Can this be moved out to keep logic the same (don't dirt data on error)?

Setting data on error is not really a problem and this gets
fixed in the next patch in the series where the reading
of the data moves to the irq-handler.

Regards,

Hans
Wolfram Sang Aug. 17, 2017, 4:16 p.m. UTC | #3 
On Mon, Aug 14, 2017 at 10:17:24PM +0200, Hans de Goede wrote:
> Although unlikely without locking the smbux_xfer function may miss
> the nack flag and further fixes in this patch-set add some more
> complex constructions which need protection.

Could be reworded, but after reading it 3 times, I got it.

> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Applied to for-next, thanks! Please don't forget the "i2c: " prefix in the
subject.
diff mbox

Patch

diff --git a/drivers/i2c/busses/i2c-cht-wc.c b/drivers/i2c/busses/i2c-cht-wc.c
index fe5caf70c7fe..09e0df49df6b 100644
--- a/drivers/i2c/busses/i2c-cht-wc.c
+++ b/drivers/i2c/busses/i2c-cht-wc.c
@@ -47,6 +47,7 @@  struct cht_wc_i2c_adap {
 	struct i2c_adapter adapter;
 	wait_queue_head_t wait;
 	struct irq_chip irqchip;
+	struct mutex adap_lock;
 	struct mutex irqchip_lock;
 	struct regmap *regmap;
 	struct irq_domain *irq_domain;
@@ -63,10 +64,13 @@  static irqreturn_t cht_wc_i2c_adap_thread_handler(int id, void *data)
 	struct cht_wc_i2c_adap *adap = data;
 	int ret, reg;
 
+	mutex_lock(&adap->adap_lock);
+
 	/* Read IRQs */
 	ret = regmap_read(adap->regmap, CHT_WC_EXTCHGRIRQ, &reg);
 	if (ret) {
 		dev_err(&adap->adapter.dev, "Error reading extchgrirq reg\n");
+		mutex_unlock(&adap->adap_lock);
 		return IRQ_NONE;
 	}
 
@@ -80,6 +84,16 @@  static irqreturn_t cht_wc_i2c_adap_thread_handler(int id, void *data)
 	if (ret)
 		dev_err(&adap->adapter.dev, "Error writing extchgrirq reg\n");
 
+	if (reg & CHT_WC_EXTCHGRIRQ_ADAP_IRQMASK) {
+		adap->nack = !!(reg & CHT_WC_EXTCHGRIRQ_NACK_IRQ);
+		adap->done = true;
+	}
+
+	mutex_unlock(&adap->adap_lock);
+
+	if (reg & CHT_WC_EXTCHGRIRQ_ADAP_IRQMASK)
+		wake_up(&adap->wait);
+
 	/*
 	 * Do NOT use handle_nested_irq here, the client irq handler will
 	 * likely want to do i2c transfers and the i2c controller uses this
@@ -96,12 +110,6 @@  static irqreturn_t cht_wc_i2c_adap_thread_handler(int id, void *data)
 		local_irq_enable();
 	}
 
-	if (reg & CHT_WC_EXTCHGRIRQ_ADAP_IRQMASK) {
-		adap->nack = !!(reg & CHT_WC_EXTCHGRIRQ_NACK_IRQ);
-		adap->done = true;
-		wake_up(&adap->wait);
-	}
-
 	return IRQ_HANDLED;
 }
 
@@ -119,8 +127,10 @@  static int cht_wc_i2c_adap_smbus_xfer(struct i2c_adapter *_adap, u16 addr,
 	struct cht_wc_i2c_adap *adap = i2c_get_adapdata(_adap);
 	int ret, reg;
 
+	mutex_lock(&adap->adap_lock);
 	adap->nack = false;
 	adap->done = false;
+	mutex_unlock(&adap->adap_lock);
 
 	ret = regmap_write(adap->regmap, CHT_WC_I2C_CLIENT_ADDR, addr);
 	if (ret)
@@ -146,18 +156,18 @@  static int cht_wc_i2c_adap_smbus_xfer(struct i2c_adapter *_adap, u16 addr,
 	ret = wait_event_timeout(adap->wait, adap->done, 3 * HZ);
 	if (ret == 0)
 		return -ETIMEDOUT;
-	if (adap->nack)
-		return -EIO;
 
-	if (read_write == I2C_SMBUS_READ) {
+	ret = 0;
+	mutex_lock(&adap->adap_lock);
+	if (adap->nack)
+		ret = -EIO;
+	else if (read_write == I2C_SMBUS_READ) {
 		ret = regmap_read(adap->regmap, CHT_WC_I2C_RDDATA, &reg);
-		if (ret)
-			return ret;
-
 		data->byte = reg;
 	}
+	mutex_unlock(&adap->adap_lock);
 
-	return 0;
+	return ret;
 }
 
 static const struct i2c_algorithm cht_wc_i2c_adap_algo = {
@@ -241,6 +251,7 @@  static int cht_wc_i2c_adap_i2c_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	init_waitqueue_head(&adap->wait);
+	mutex_init(&adap->adap_lock);
 	mutex_init(&adap->irqchip_lock);
 	adap->irqchip = cht_wc_i2c_irq_chip;
 	adap->regmap = pmic->regmap;