| Message ID | 20171021083641.7226-1-d.scheller.oss@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Daniel Scheller writes: > From: Daniel Scheller <d.scheller@gmx.net> > > When calling gate_ctrl() with enable=0 if previously the mutex wasn't > locked (ie. on enable=1 failure and subdrivers not handling this properly, > or by otherwise badly behaving drivers), the i2c_lock could be unlocked I think drivers and subdrivers should rather be fixed so that this cannot happen. But to do this we will first need to define exactly how a failure in gate_ctrl() is supposed to be handled, both inside gate_ctrl() and by calling drivers. > consecutively which isn't allowed. Prevent this by keeping track of the > lock state, and actually call mutex_unlock() only when certain the lock > is held. Why not use mutex_is_locked()? And there should be a debug message if it (tried double unlocking) happens. Regards, Ralph
Am Sat, 21 Oct 2017 11:28:10 +0200 schrieb Ralph Metzler <rjkm@metzlerbros.de>: > Daniel Scheller writes: > > From: Daniel Scheller <d.scheller@gmx.net> > > > > When calling gate_ctrl() with enable=0 if previously the mutex > > wasn't locked (ie. on enable=1 failure and subdrivers not handling > > this properly, or by otherwise badly behaving drivers), the > > i2c_lock could be unlocked > > I think drivers and subdrivers should rather be fixed so that this > cannot happen. As long as stv6111 remains the only chip/driver interfacing with the stv0910, that's an easy task. However, if other hardware has some other stv0910+tunerchip combination, things get interesting. In a perfect world with unicorns and such, every component interacts as intended, but that's not the case, so I believe this should be handled at the root. > But to do this we will first need to define exactly how a failure in > gate_ctrl() is supposed to be handled, both inside gate_ctrl() and > by calling drivers. Well, IMHO (and thats the intention) if gate_ctrl fails due to a hardware/I2C problem, it isn't opened so there's no need to hold the lock (since the gate isn't - exclusively - opened). For reasons stated above this keeps things safe from deadlocking (and we want to avoid that, even more than double unlocking). > > consecutively which isn't allowed. Prevent this by keeping track > > of the lock state, and actually call mutex_unlock() only when > > certain the lock is held. > > Why not use mutex_is_locked()? Good catch (I should try harder finding out what the kernel API has to offer...). If you prefer that, I'll respin with this and without the var as v2. > And there should be a debug message if it (tried double unlocking) > happens. Ok. Should IMHO go to dev_dbg then - if drivers don't catch that situation, this may else lead do kernel log spam. Best regards, Daniel Scheller
Em Sat, 21 Oct 2017 11:57:57 +0200 Daniel Scheller <d.scheller.oss@gmail.com> escreveu: > Am Sat, 21 Oct 2017 11:28:10 +0200 > schrieb Ralph Metzler <rjkm@metzlerbros.de>: > > > Daniel Scheller writes: > > > From: Daniel Scheller <d.scheller@gmx.net> > > > > > > When calling gate_ctrl() with enable=0 if previously the mutex > > > wasn't locked (ie. on enable=1 failure and subdrivers not handling > > > this properly, or by otherwise badly behaving drivers), the > > > i2c_lock could be unlocked > > > > I think drivers and subdrivers should rather be fixed so that this > > cannot happen. Agreed. > > As long as stv6111 remains the only chip/driver interfacing with the > stv0910, that's an easy task. However, if other hardware has some other > stv0910+tunerchip combination, things get interesting. In a perfect > world with unicorns and such, every component interacts as intended, > but that's not the case, so I believe this should be handled at the > root. No matter what tuner/demod is used, if the locks there are not properly handled, bad things will happen. So, if this patch is needed for the driver(s) to work, it means that we have a serious issue urging a real fix. This ugly hack won't prevent it to happen. It just masks the issue, by keeping the driver locked for more time than needed, with possibly cause other issues. > > > But to do this we will first need to define exactly how a failure in > > gate_ctrl() is supposed to be handled, both inside gate_ctrl() and > > by calling drivers. > > Well, IMHO (and thats the intention) if gate_ctrl fails due to a > hardware/I2C problem, it isn't opened so there's no need to hold the > lock (since the gate isn't - exclusively - opened). For reasons stated > above this keeps things safe from deadlocking (and we want to avoid > that, even more than double unlocking). > > > > consecutively which isn't allowed. Prevent this by keeping track > > > of the lock state, and actually call mutex_unlock() only when > > > certain the lock is held. > > > > Why not use mutex_is_locked()? > > Good catch (I should try harder finding out what the kernel API has to > offer...). If you prefer that, I'll respin with this and without the > var as v2. I may consider accepting something like: WARN_ON(mutex_is_locked(mutex)); That will actually cause a bug at the driver with a stack dump, with may lead to a proper fix on the drivers, instead of a hackish solution that would just let the bug to stay there forever. Thanks, Mauro
diff --git a/drivers/media/dvb-frontends/stv0910.c b/drivers/media/dvb-frontends/stv0910.c index 73f6df0abbfe..36ef96ec64c1 100644 --- a/drivers/media/dvb-frontends/stv0910.c +++ b/drivers/media/dvb-frontends/stv0910.c @@ -80,6 +80,7 @@ struct stv_base { u8 adr; struct i2c_adapter *i2c; struct mutex i2c_lock; /* shared I2C access protect */ + u8 i2c_islocked; /* I2C lock state */ struct mutex reg_lock; /* shared register write protect */ int count; @@ -1233,6 +1234,7 @@ static int gate_ctrl(struct dvb_frontend *fe, int enable) if (enable) { mutex_lock(&state->base->i2c_lock); + state->base->i2c_islocked = 1; i2crpt |= 0x80; } else { i2crpt |= 0x02; @@ -1240,8 +1242,15 @@ static int gate_ctrl(struct dvb_frontend *fe, int enable) if (write_reg(state, state->nr ? RSTV0910_P2_I2CRPT : RSTV0910_P1_I2CRPT, i2crpt) < 0) { - /* don't hold the I2C bus lock on failure */ - mutex_unlock(&state->base->i2c_lock); + /* + * don't hold the I2C bus lock on failure while preventing + * consecutive and disallowed calls to mutex_unlock() + */ + if (state->base->i2c_islocked) { + state->base->i2c_islocked = 0; + mutex_unlock(&state->base->i2c_lock); + } + dev_err(&state->base->i2c->dev, "%s() write_reg failure (enable=%d)\n", __func__, enable); @@ -1250,8 +1259,13 @@ static int gate_ctrl(struct dvb_frontend *fe, int enable) state->i2crpt = i2crpt; - if (!enable) - mutex_unlock(&state->base->i2c_lock); + if (!enable) { + if (state->base->i2c_islocked) { + state->base->i2c_islocked = 0; + mutex_unlock(&state->base->i2c_lock); + } + } + return 0; } @@ -1795,6 +1809,7 @@ struct dvb_frontend *stv0910_attach(struct i2c_adapter *i2c, mutex_init(&base->i2c_lock); mutex_init(&base->reg_lock); + base->i2c_islocked = 0; state->base = base; if (probe(state) < 0) { dev_info(&i2c->dev, "No demod found at adr %02X on %s\n",