[PATCH] cbus-tahvo: Add tahvo_write_reg locking

[PATCH] cbus-tahvo: Add tahvo_write_reg locking

[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 <m@bues.ch>

---

Index: linux-3.1/drivers/cbus/tahvo.c
===================================================================
--- linux-3.1.orig/drivers/cbus/tahvo.c	2011-11-05 17:03:39.598846119 +0100
+++ linux-3.1/drivers/cbus/tahvo.c	2011-11-05 17:04:36.274768324 +0100
@@ -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);
 


-- 
Greetings, Michael.

Re: [PATCH] cbus-tahvo: Add tahvo_write_reg locking

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1100 bytes --]

Hi,

On Sat, Nov 05, 2011 at 05:19:54PM +0100, Michael Büsch wrote:
> 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 <m@bues.ch>
> 
> ---
> 
> Index: linux-3.1/drivers/cbus/tahvo.c
> ===================================================================
> --- linux-3.1.orig/drivers/cbus/tahvo.c	2011-11-05 17:03:39.598846119 +0100
> +++ linux-3.1/drivers/cbus/tahvo.c	2011-11-05 17:04:36.274768324 +0100
> @@ -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...

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] cbus-tahvo: Add tahvo_write_reg locking

[Michael Büsch]

On Mon, 7 Nov 2011 08:10:45 +0200
Felipe Balbi <balbi@ti.com> wrote:

> Hi,
> 
> On Sat, Nov 05, 2011 at 05:19:54PM +0100, Michael Büsch wrote:
> > 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 <m@bues.ch>
> > 
> > ---
> > 
> > Index: linux-3.1/drivers/cbus/tahvo.c
> > ===================================================================
> > --- linux-3.1.orig/drivers/cbus/tahvo.c	2011-11-05 17:03:39.598846119 +0100
> > +++ linux-3.1/drivers/cbus/tahvo.c	2011-11-05 17:04:36.274768324 +0100
> > @@ -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...

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.

-- 
Greetings, Michael.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cbus-tahvo: Add tahvo_write_reg locking

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1557 bytes --]

Hi,

On Mon, Nov 07, 2011 at 09:18:36AM +0000, Michael Büsch wrote:
> > On Sat, Nov 05, 2011 at 05:19:54PM +0100, Michael Büsch wrote:
> > > 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 <m@bues.ch>
> > > 
> > > ---
> > > 
> > > Index: linux-3.1/drivers/cbus/tahvo.c
> > > ===================================================================
> > > --- linux-3.1.orig/drivers/cbus/tahvo.c	2011-11-05 17:03:39.598846119 +0100
> > > +++ linux-3.1/drivers/cbus/tahvo.c	2011-11-05 17:04:36.274768324 +0100
> > > @@ -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...
> 
> 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 <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]