recursive locking problem

recursive locking problem

[Antti Palosaari]

I am working with AF9015 I2C-adapter lock. I need lock I2C-bus since 
there is two tuners having same I2C address on same bus, demod I2C gate 
is used to select correct tuner.

I am trapping demod .i2c_gate_ctrl() calls and locking bus according to 
that.

Is there any lock can do recursive locking but unlock frees all locks?

Like that:
gate_open
+gate_open
+gate_close
== lock is free

AFAIK mutex can do only simple lock() + unlock(). Semaphore can do 
recursive locking, like lock() + lock() + unlock() + unlock(). But how I 
can do lock() + lock() + unlock() == free.


Antti
-- 
http://palosaari.fi/

Re: recursive locking problem

[Hans Petter Selasky]

On Thursday 08 September 2011 18:34:32 Antti Palosaari wrote:
> lock() + lock() + unlock() == free.

Hi,

As far as I can see the Linux kernel's mutex API doesn't have support for 
checking if a mutex is owned. I guess you would have to do something like:

while (mutex_owned(&xxx))
	mutex_unlock(&xxx);

--HPS

Re: recursive locking problem

[David Waring]

On 08/09/11 17:34, Antti Palosaari wrote:
> [snip]
> 
> Is there any lock can do recursive locking but unlock frees all locks?
> 
> Like that:
> gate_open
> +gate_open
> +gate_close
> == lock is free
> 
> AFAIK mutex can do only simple lock() + unlock(). Semaphore can do
> recursive locking, like lock() + lock() + unlock() + unlock(). But how I
> can do lock() + lock() + unlock() == free.
> 
Antti,

It's a very bad idea to try and use a mutex like that. The number of
locks and unlocks must be balanced otherwise you risk accessing
variables without a lock.

Consider:

static struct mutex foo_mutex;
static int foo=3;

void a() {
  mutex_lock(&foo_mutex);
  if (foo<5) foo++;
  b();
  foo--; /* <<< still need lock here */
  mutex_unlock(&foo_mutex);
}

void b() {
  mutex_lock(&foo_mutex);
  if (foo>6) foo=(foo>>1);
  mutex_unlock(&foo_mutex);
}

Note: this assumes mutex_lock will allow the same thread get multiple
locks as you would like (which it doesn't).

As pointed out in the code, when a() is called, you still need the lock
for accesses to foo after the call to b() that also requires the lock.
If we used the locks in the way you propose then foo would be accessed
without a lock.

To code properly for cases like these I usually use a wrapper functions
to acquire the lock and call a thread unsafe version (i.e. doesn't use
locks) of the function that only uses other thread unsafe functions. e.g.

void a() {
  mutex_lock(&foo_mutex);
  __a_thr_unsafe();
  mutex_unlock(&foo_mutex);
}

void b() {
  mutex_lock(&foo_mutex);
  __b_thr_unsafe();
  mutex_unlock(&foo_mutex);
}

static void __a_thr_unsafe() {
  if (foo<5) foo++;
  __b_thr_unsafe();
  foo--;
}

static void __b_thr_unsafe() {
  if (foo>6) foo=(foo>>1);
}

This way a call to a() or b() will acquire the lock once for that
thread, perform all actions and then release the lock. The mutex is
handled properly.

Can you restructure the code so that you don't need multiple locks?

-- 
David Waring

Re: recursive locking problem

[Antti Palosaari]

On 09/09/2011 01:45 PM, David Waring wrote:
> On 08/09/11 17:34, Antti Palosaari wrote:
>> [snip]
>>
>> Is there any lock can do recursive locking but unlock frees all locks?
>>
>> Like that:
>> gate_open
>> +gate_open
>> +gate_close
>> == lock is free
>>
>> AFAIK mutex can do only simple lock() + unlock(). Semaphore can do
>> recursive locking, like lock() + lock() + unlock() + unlock(). But how I
>> can do lock() + lock() + unlock() == free.
>>
> Antti,
>
> It's a very bad idea to try and use a mutex like that. The number of
> locks and unlocks must be balanced otherwise you risk accessing
> variables without a lock.
>
> Consider:
>
> static struct mutex foo_mutex;
> static int foo=3;
>
> void a() {
>    mutex_lock(&foo_mutex);
>    if (foo<5) foo++;
>    b();
>    foo--; /*<<<  still need lock here */
>    mutex_unlock(&foo_mutex);
> }
>
> void b() {
>    mutex_lock(&foo_mutex);
>    if (foo>6) foo=(foo>>1);
>    mutex_unlock(&foo_mutex);
> }
>
> Note: this assumes mutex_lock will allow the same thread get multiple
> locks as you would like (which it doesn't).
>
> As pointed out in the code, when a() is called, you still need the lock
> for accesses to foo after the call to b() that also requires the lock.
> If we used the locks in the way you propose then foo would be accessed
> without a lock.
>
> To code properly for cases like these I usually use a wrapper functions
> to acquire the lock and call a thread unsafe version (i.e. doesn't use
> locks) of the function that only uses other thread unsafe functions. e.g.
>
> void a() {
>    mutex_lock(&foo_mutex);
>    __a_thr_unsafe();
>    mutex_unlock(&foo_mutex);
> }
>
> void b() {
>    mutex_lock(&foo_mutex);
>    __b_thr_unsafe();
>    mutex_unlock(&foo_mutex);
> }
>
> static void __a_thr_unsafe() {
>    if (foo<5) foo++;
>    __b_thr_unsafe();
>    foo--;
> }
>
> static void __b_thr_unsafe() {
>    if (foo>6) foo=(foo>>1);
> }
>
> This way a call to a() or b() will acquire the lock once for that
> thread, perform all actions and then release the lock. The mutex is
> handled properly.
>
> Can you restructure the code so that you don't need multiple locks?

Thank you for very long and detailed reply with examples :)

I need lock for hardware access. Single I2C-adapter have two I2C-clients 
that have same I2C-address in same bus. There is gate (demod I2C-gate) 
logic that is used to select desired tuner. See that:
http://palosaari.fi/linux/v4l-dvb/controlling_tuner_af9015_dual_demod.txt

You can never know surely how tuner drivers calls to open or close gate, 
very commonly there is situations where multiple close or open happens. 
That's why lock/unlock is problematic.

.i2c_gate_ctrl() is demod driver callback (struct dvb_frontend_ops) 
which controls gate that gate. That callback is always called from tuner 
driver when gate is needed to open or close.

regards
Antti

-- 
http://palosaari.fi/

Re: recursive locking problem

[Steve Kerrison]

At the risk of sounding silly, why do we rely on i2c gating so much? The
whole point of i2c is that you can sit a bunch of devices on the same
pair of wires and talk to one at a time.

Why not just open up the gates and be done with it, except for
situations where the i2c chain foolishly has two devices that have the
same address because somebody didn't net the address configuration pins
correctly, or it's a really big system with more devices on it that a
particular chip family has sub-addresses?

>From a signal-driving point of view, the gate circuitry is surely a
sufficient buffer. I guess the only two things I can think of as real
reasons are to 1) reduce the chance of misbehaving devices from causing
problems and 2) to save power by not sending clocks and data where we
know they aren't needed.

Can any one shed some light on this? I appreciate it's not a linux or
indeed linux-media specific issue as the hardware itself is designed
this way.

Getting back to the subject, Antti's af9015 example demonstrates
precisely when gating is needed. But I have to ask, does the MXL5003
really only support one address; can it not be reconfigured? it's a bit
late to ask that question once the PCB is fabbed, I admit.

Would it make any sense to haul the gate locking (or some wrapper at
least) up into the uC for that particular configuration?

E.g:

Tuner driver wants to i2c write tuner 0
Calls i2c_gate_ctrl(open)
uC checks if a gate is already open, if it is, and is the same gate as
is already open, just carry on. If not, wait on a lock, then set which
gate is open and proceed to the 'real' gate_ctrl op.
Similar path for i2c_gate_ctrl(close), as relaxed as it needs to be to
cope with how different tuner drivers work.

You could wrap the checking of which gate is open in a lock for
atomicity, and only wait on a device lock where necessary. This way you
can also check whether you're trying to change the state of a gate or
not and early-out (unless calling gate_ctrl(open) more than once does
something /different/ to calling it just once, in which case we're
doomed and this was nothing more than naive rambling).

Cheers,
-- 
Steve Kerrison MEng Hons.
http://www.stevekerrison.com/ 

On Tue, 2011-09-13 at 23:59 +0300, Antti Palosaari wrote:
> On 09/09/2011 01:45 PM, David Waring wrote:
> > On 08/09/11 17:34, Antti Palosaari wrote:
> >> [snip]
> >>
> >> Is there any lock can do recursive locking but unlock frees all locks?
> >>
> >> Like that:
> >> gate_open
> >> +gate_open
> >> +gate_close
> >> == lock is free
> >>
> >> AFAIK mutex can do only simple lock() + unlock(). Semaphore can do
> >> recursive locking, like lock() + lock() + unlock() + unlock(). But how I
> >> can do lock() + lock() + unlock() == free.
> >>
> > Antti,
> >
> > It's a very bad idea to try and use a mutex like that. The number of
> > locks and unlocks must be balanced otherwise you risk accessing
> > variables without a lock.
> >
> > Consider:
> >
> > static struct mutex foo_mutex;
> > static int foo=3;
> >
> > void a() {
> >    mutex_lock(&foo_mutex);
> >    if (foo<5) foo++;
> >    b();
> >    foo--; /*<<<  still need lock here */
> >    mutex_unlock(&foo_mutex);
> > }
> >
> > void b() {
> >    mutex_lock(&foo_mutex);
> >    if (foo>6) foo=(foo>>1);
> >    mutex_unlock(&foo_mutex);
> > }
> >
> > Note: this assumes mutex_lock will allow the same thread get multiple
> > locks as you would like (which it doesn't).
> >
> > As pointed out in the code, when a() is called, you still need the lock
> > for accesses to foo after the call to b() that also requires the lock.
> > If we used the locks in the way you propose then foo would be accessed
> > without a lock.
> >
> > To code properly for cases like these I usually use a wrapper functions
> > to acquire the lock and call a thread unsafe version (i.e. doesn't use
> > locks) of the function that only uses other thread unsafe functions. e.g.
> >
> > void a() {
> >    mutex_lock(&foo_mutex);
> >    __a_thr_unsafe();
> >    mutex_unlock(&foo_mutex);
> > }
> >
> > void b() {
> >    mutex_lock(&foo_mutex);
> >    __b_thr_unsafe();
> >    mutex_unlock(&foo_mutex);
> > }
> >
> > static void __a_thr_unsafe() {
> >    if (foo<5) foo++;
> >    __b_thr_unsafe();
> >    foo--;
> > }
> >
> > static void __b_thr_unsafe() {
> >    if (foo>6) foo=(foo>>1);
> > }
> >
> > This way a call to a() or b() will acquire the lock once for that
> > thread, perform all actions and then release the lock. The mutex is
> > handled properly.
> >
> > Can you restructure the code so that you don't need multiple locks?
> 
> Thank you for very long and detailed reply with examples :)
> 
> I need lock for hardware access. Single I2C-adapter have two I2C-clients 
> that have same I2C-address in same bus. There is gate (demod I2C-gate) 
> logic that is used to select desired tuner. See that:
> http://palosaari.fi/linux/v4l-dvb/controlling_tuner_af9015_dual_demod.txt
> 
> You can never know surely how tuner drivers calls to open or close gate, 
> very commonly there is situations where multiple close or open happens. 
> That's why lock/unlock is problematic.
> 
> .i2c_gate_ctrl() is demod driver callback (struct dvb_frontend_ops) 
> which controls gate that gate. That callback is always called from tuner 
> driver when gate is needed to open or close.
> 
> regards
> Antti
> 

Re: recursive locking problem

[Steven Toth]

> Can any one shed some light on this? I appreciate it's not a linux or
> indeed linux-media specific issue as the hardware itself is designed
> this way.

i2c gates exist to isolate the downstream components from any spurious
RF noise generated by noisy components on the i2c bus. You don't want
to couple demod noise into the tuner by default.

-- 
Steven Toth - Kernel Labs
http://www.kernellabs.com

Re: recursive locking problem

[Devin Heitmueller]

On Tue, Sep 13, 2011 at 5:58 PM, Steven Toth <stoth@kernellabs.com> wrote:
>> Can any one shed some light on this? I appreciate it's not a linux or
>> indeed linux-media specific issue as the hardware itself is designed
>> this way.
>
> i2c gates exist to isolate the downstream components from any spurious
> RF noise generated by noisy components on the i2c bus. You don't want
> to couple demod noise into the tuner by default.

Steve (Kerrison),

I would take Steven Toth's explanation as more authoritative than mine
any day of the week.  It's possible that I may have been misinformed
regarding the rationale for why the gate is required.

Regards,

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: recursive locking problem

[Steven Toth]

> I would take Steven Toth's explanation as more authoritative than mine
> any day of the week.  It's possible that I may have been misinformed
> regarding the rationale for why the gate is required.

In general, complete bus isolation lowers noise levels, whether
through RF coupling or needless interrupt servicing related. Either
rationale is valid and varies between designs.

-- 
Steven Toth - Kernel Labs
http://www.kernellabs.com

Re: recursive locking problem

[Devin Heitmueller]

On Tue, Sep 13, 2011 at 5:34 PM, Steve Kerrison <steve@stevekerrison.com> wrote:
> At the risk of sounding silly, why do we rely on i2c gating so much? The
> whole point of i2c is that you can sit a bunch of devices on the same
> pair of wires and talk to one at a time.

Steve,

There are essentially two issues here.  To address the general
question, many tuner chips require an i2c gate because their onboard
i2c controller is implemented using interrupts, and servicing the
interrupts to even check if the traffic is intended for the tuner can
interfere with the core tuning function.  In other words, the cost of
the chip "watching for traffic" can adversely effect tuning quality.
As a result, most hardware designs are such that the demodulator gates
the i2c traffic such that the tuner only *ever* sees traffic intended
for it.

The second issue is that within the LinuxTV drivers there is
inconsistency regarding whether the i2c gate is opened/closed by the
tuner driver or whether it's done by the demod.  Some drivers have the
demod driver open the gate, issue the tuning request, and then close
the gate, while in other drivers the tuner driver opens/closes the
gate whenever there are register reads/writes to the tuner.  It's all
about the granularity of implementation (the demod approach only
involves one open/close but it's for potentially a longer period of
time, versus the tuner approach which opens/closes the gate repeatedly
as needed, which means more open/closes but the gate is open for the
bare minimum of time required).

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: recursive locking problem

[Daniel Glöckner]

On Thu, Sep 08, 2011 at 07:34:32PM +0300, Antti Palosaari wrote:
> I am working with AF9015 I2C-adapter lock. I need lock I2C-bus since
> there is two tuners having same I2C address on same bus, demod I2C
> gate is used to select correct tuner.

Would it be possible to use the i2c-mux framework to handle this?
Each tuner will then have its own i2c bus.

  Daniel

Re: recursive locking problem

[Antti Palosaari]

On 09/09/2011 02:46 PM, Daniel Glöckner wrote:
> On Thu, Sep 08, 2011 at 07:34:32PM +0300, Antti Palosaari wrote:
>> I am working with AF9015 I2C-adapter lock. I need lock I2C-bus since
>> there is two tuners having same I2C address on same bus, demod I2C
>> gate is used to select correct tuner.
>
> Would it be possible to use the i2c-mux framework to handle this?
> Each tuner will then have its own i2c bus.

Interesting idea, but it didn't worked. It deadlocks. I think it locks 
since I2C-mux is controlled by I2C "switch" in same I2C bus, not GPIO or 
some other HW.

* tuner does I2C xfer on I2C-mux adapter
* I2C-mux adapter calls demod .i2c_gate_ctrl()
* demod does register access using I2C
* DEADLOCK

Maybe since tuner I2C xfer have already locked I2C-adater. Then demod 
access same adapter and it is locked.

But nice I2C mux anyhow.

@David Daney, see that pic in order to get understanding what kind of 
problem I am working;
http://palosaari.fi/linux/v4l-dv/controlling_tuner_af9015_dual_demod.txt


regards
Antti


-- 
http://palosaari.fi/

Re: recursive locking problem

[Daniel Glöckner]

On Wed, Sep 14, 2011 at 04:03:58AM +0300, Antti Palosaari wrote:
> On 09/09/2011 02:46 PM, Daniel Glöckner wrote:
> >On Thu, Sep 08, 2011 at 07:34:32PM +0300, Antti Palosaari wrote:
> >>I am working with AF9015 I2C-adapter lock. I need lock I2C-bus since
> >>there is two tuners having same I2C address on same bus, demod I2C
> >>gate is used to select correct tuner.
> >
> >Would it be possible to use the i2c-mux framework to handle this?
> >Each tuner will then have its own i2c bus.
> 
> Interesting idea, but it didn't worked. It deadlocks. I think it
> locks since I2C-mux is controlled by I2C "switch" in same I2C bus,
> not GPIO or some other HW.

Take a look at drivers/i2c/muxes/pca954x.c. You need to use
parent->algo->master_xfer/smbus_xfer directly as the lock that
protects you from having both gates open is the lock of the
root i2c bus.

  Daniel

Re: recursive locking problem

[Antti Palosaari]

On 09/14/2011 09:19 AM, Daniel Glöckner wrote:
> On Wed, Sep 14, 2011 at 04:03:58AM +0300, Antti Palosaari wrote:
>> On 09/09/2011 02:46 PM, Daniel Glöckner wrote:
>>> On Thu, Sep 08, 2011 at 07:34:32PM +0300, Antti Palosaari wrote:
>>>> I am working with AF9015 I2C-adapter lock. I need lock I2C-bus since
>>>> there is two tuners having same I2C address on same bus, demod I2C
>>>> gate is used to select correct tuner.
>>>
>>> Would it be possible to use the i2c-mux framework to handle this?
>>> Each tuner will then have its own i2c bus.
>>
>> Interesting idea, but it didn't worked. It deadlocks. I think it
>> locks since I2C-mux is controlled by I2C "switch" in same I2C bus,
>> not GPIO or some other HW.
>
> Take a look at drivers/i2c/muxes/pca954x.c. You need to use
> parent->algo->master_xfer/smbus_xfer directly as the lock that
> protects you from having both gates open is the lock of the
> root i2c bus.

Ah yes, rather similar case. I see it as commented in pca954x.c:
/* Write to mux register. Don't use i2c_transfer()/i2c_smbus_xfer()
    for this as they will try to lock adapter a second time */

This looks even more hackish solution than calling existing demod 
.i2c_gate_ctrl() callback from USB-interface driver. But yes, it must 
work - not beautiful but workable workaround.


regards
Antti

-- 
http://palosaari.fi/

Re: recursive locking problem

[Daniel Glöckner]

On Wed, Sep 14, 2011 at 01:45:47PM +0300, Antti Palosaari wrote:
> >>Interesting idea, but it didn't worked. It deadlocks. I think it
> >>locks since I2C-mux is controlled by I2C "switch" in same I2C bus,
> >>not GPIO or some other HW.
> >
> >Take a look at drivers/i2c/muxes/pca954x.c. You need to use
> >parent->algo->master_xfer/smbus_xfer directly as the lock that
> >protects you from having both gates open is the lock of the
> >root i2c bus.
> 
> Ah yes, rather similar case. I see it as commented in pca954x.c:
> /* Write to mux register. Don't use i2c_transfer()/i2c_smbus_xfer()
>    for this as they will try to lock adapter a second time */
> 
> This looks even more hackish solution than calling existing demod
> .i2c_gate_ctrl() callback from USB-interface driver. But yes, it
> must work - not beautiful but workable workaround.

This is not a hack. This is the official way to do it.
The I2C mux framework was designed to allow multiplexers controlled
through the same I2C bus.

  Daniel