Skip to contentSkip to conversation

cbus-tahvo: Add tahvo_write_reg locking

PATCH In review Michael Büsch 2 participants 4 messages Nov 5, 2011 – Nov 7, 2011

lore
Share & tools
lore

Michael Büsch

author
#0 lore raw

PATCH 1/1 cbus-tahvo: Add tahvo_write_reg locking

tahvo_write_reg() needs to take the mutex to avoid a race condition with tahvo_set_clear_reg_bits:

tahvo_set_clear_reg_bits():   |  tahvo_write_reg():
    __tahvo_read_reg()        |
                              |      __tahvo_write_reg() <-- race here
    __tahvo_write_reg()       |
Signed-off-by Michael Buesch <[email protected]>

---

drivers/cbus/tahvo.c +2 -0
Index: linux-3.1/drivers/cbus/tahvo.c
===================================================================
@@ -104,7 +104,9 @@ void tahvo_write_reg(struct device *chil
 {
 	struct tahvo		*tahvo = dev_get_drvdata(child->parent);
 
+	mutex_lock(&tahvo->mutex);
 	__tahvo_write_reg(tahvo, reg, val);
+	mutex_unlock(&tahvo->mutex);
 }
 EXPORT_SYMBOL(tahvo_write_reg);
Signature · 1 line
Greetings, Michael.
#1 lore raw

Hi,

Quoted 17 lines · Michael Büsch

tahvo_write_reg() needs to take the mutex to avoid a race condition with tahvo_set_clear_reg_bits:

tahvo_set_clear_reg_bits():   |  tahvo_write_reg():
    __tahvo_read_reg()        |
                              |      __tahvo_write_reg() <-- race here
    __tahvo_write_reg()       |
Signed-off-by Michael Buesch <[email protected]>

---

Index: linux-3.1/drivers/cbus/tahvo.c
===================================================================

drivers/cbus/tahvo.c void tahvo_write_reg(struct device *chil · line 109

Quoted 6 lines
drivers/cbus/tahvo.c +2 -0
@@ -104,7 +104,9 @@ void tahvo_write_reg(struct device *chil
 {
 	struct tahvo		*tahvo = dev_get_drvdata(child->parent);
 
+	mutex_lock(&tahvo->mutex);
 	__tahvo_write_reg(tahvo, reg, val);
+	mutex_unlock(&tahvo->mutex);

yeah, my bad. The same should be done with tahvo_read_reg(). Care to resend this patch adding the change to tahvo_read_reg() too...

Signature · 1 line
balbi
author
#2 lore raw
Quoted 23 lines · On Mon, 7 Nov 2011 08:10:45 +0200 Felipe Balbi <[email protected]> wrote:

Hi,

Quoted 20 lines · Michael Büsch

tahvo_write_reg() needs to take the mutex to avoid a race condition with tahvo_set_clear_reg_bits:

tahvo_set_clear_reg_bits():   |  tahvo_write_reg():
    __tahvo_read_reg()        |
                              |      __tahvo_write_reg() <-- race here
    __tahvo_write_reg()       |
Signed-off-by Michael Buesch <[email protected]>

---

Index: linux-3.1/drivers/cbus/tahvo.c
===================================================================

drivers/cbus/tahvo.c +0 -0
@@ -104,7 +104,9 @@ void tahvo_write_reg(struct device *chil
 {
 	struct tahvo		*tahvo = dev_get_drvdata(child->parent);
Quoted 6 lines

drivers/cbus/tahvo.c void tahvo_write_reg(struct device *chil · line 109

Quoted 3 lines
drivers/cbus/tahvo.c +2 -0
+	mutex_lock(&tahvo->mutex);
 	__tahvo_write_reg(tahvo, reg, val);
+	mutex_unlock(&tahvo->mutex);

yeah, my bad. The same should be done with tahvo_read_reg(). Care to resend this patch adding the change to tahvo_read_reg() too...

I think tahvo_read_reg is fine without a lock. read vs write is already atomic due to the cbus lock.

it's the same situation as for retu. where we also don't need the lock in retu_read_reg.

Signature · 5 lines
Greetings, Michael.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Acked-by
#3 lore raw

Hi,

Quoted 28 lines · Michael Büsch
Quoted 21 lines · Michael Büsch
Quoted 20 lines

tahvo_write_reg() needs to take the mutex to avoid a race condition with tahvo_set_clear_reg_bits:

tahvo_set_clear_reg_bits():   |  tahvo_write_reg():
    __tahvo_read_reg()        |
                              |      __tahvo_write_reg() <-- race here
    __tahvo_write_reg()       |
Signed-off-by Michael Buesch <[email protected]>

---

Index: linux-3.1/drivers/cbus/tahvo.c
===================================================================

drivers/cbus/tahvo.c +0 -0
@@ -104,7 +104,9 @@ void tahvo_write_reg(struct device *chil
 {
 	struct tahvo		*tahvo = dev_get_drvdata(child->parent);
Quoted 6 lines

drivers/cbus/tahvo.c void tahvo_write_reg(struct device *chil · line 109

Quoted 3 lines
drivers/cbus/tahvo.c +2 -0
+	mutex_lock(&tahvo->mutex);
 	__tahvo_write_reg(tahvo, reg, val);
+	mutex_unlock(&tahvo->mutex);

yeah, my bad. The same should be done with tahvo_read_reg(). Care to resend this patch adding the change to tahvo_read_reg() too...

Quoted 5 lines

I think tahvo_read_reg is fine without a lock. read vs write is already atomic due to the cbus lock.

it's the same situation as for retu. where we also don't need the lock in retu_read_reg.

ok then, makes sense.

FWIW:

Acked-by Felipe Balbi <[email protected]>
Signature · 1 line
balbi