From: Mauro Carvalho Chehab <mchehab@redhat.com>
To: Alexey Khoroshilov <khoroshilov@ispras.ru>
Cc: Mauro Carvalho Chehab <mchehab@infradead.org>,
Patrick Boettcher <Patrick.Boettcher@dibcom.fr>,
Olivier Grenie <olivier.grenie@dibcom.fr>,
linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
ldv-project@ispras.ru
Subject: Re: [PATCH 2/2] [media] dib9000: implement error handling for DibAcquireLock
Date: Mon, 19 Mar 2012 14:35:17 -0300 [thread overview]
Message-ID: <4F676E55.1060003@redhat.com> (raw)
In-Reply-To: <1331068109-11613-1-git-send-email-khoroshilov@ispras.ru>
Em 06-03-2012 18:08, Alexey Khoroshilov escreveu:
> DibAcquireLock() is implemented as mutex_lock_interruptible()
> but the driver does not handle unsuccessful locking.
> As a result it may lead to unlock of an unheld mutex.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> ---
> drivers/media/dvb/frontends/dib9000.c | 115 ++++++++++++++++++++++++++-------
> 1 files changed, 91 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/media/dvb/frontends/dib9000.c b/drivers/media/dvb/frontends/dib9000.c
> index d96b6a1..80848b4 100644
> --- a/drivers/media/dvb/frontends/dib9000.c
> +++ b/drivers/media/dvb/frontends/dib9000.c
> @@ -33,7 +33,7 @@ struct i2c_device {
>
> /* lock */
> #define DIB_LOCK struct mutex
> -#define DibAcquireLock(lock) do { if (mutex_lock_interruptible(lock) < 0) dprintk("could not get the lock"); } while (0)
> +#define DibAcquireLock(lock) mutex_lock_interruptible(lock)
> #define DibReleaseLock(lock) mutex_unlock(lock)
> #define DibInitLock(lock) mutex_init(lock)
> #define DibFreeLock(lock)
It will likely be better to remove all those macros, as they just make the
driver code harder to review.
I'll apply this patch, as it fixes a bug, but it would be great if you
could send us a patch to get rid of those macros.
Thanks,
Mauro
> @@ -446,7 +446,10 @@ static int dib9000_risc_mem_read(struct dib9000_state *state, u8 cmd, u8 * b, u1
> if (!state->platform.risc.fw_is_running)
> return -EIO;
>
> - DibAcquireLock(&state->platform.risc.mem_lock);
> + if (DibAcquireLock(&state->platform.risc.mem_lock) < 0) {
> + dprintk("could not get the lock");
> + return -EINTR;
> + }
> dib9000_risc_mem_setup(state, cmd | 0x80);
> dib9000_risc_mem_read_chunks(state, b, len);
> DibReleaseLock(&state->platform.risc.mem_lock);
> @@ -459,7 +462,10 @@ static int dib9000_risc_mem_write(struct dib9000_state *state, u8 cmd, const u8
> if (!state->platform.risc.fw_is_running)
> return -EIO;
>
> - DibAcquireLock(&state->platform.risc.mem_lock);
> + if (DibAcquireLock(&state->platform.risc.mem_lock) < 0) {
> + dprintk("could not get the lock");
> + return -EINTR;
> + }
> dib9000_risc_mem_setup(state, cmd);
> dib9000_risc_mem_write_chunks(state, b, m->size);
> DibReleaseLock(&state->platform.risc.mem_lock);
> @@ -531,7 +537,10 @@ static int dib9000_mbx_send_attr(struct dib9000_state *state, u8 id, u16 * data,
> if (!state->platform.risc.fw_is_running)
> return -EINVAL;
>
> - DibAcquireLock(&state->platform.risc.mbx_if_lock);
> + if (DibAcquireLock(&state->platform.risc.mbx_if_lock) < 0) {
> + dprintk("could not get the lock");
> + return -EINTR;
> + }
> tmp = MAX_MAILBOX_TRY;
> do {
> size = dib9000_read_word_attr(state, 1043, attr) & 0xff;
> @@ -593,7 +602,10 @@ static u8 dib9000_mbx_read(struct dib9000_state *state, u16 * data, u8 risc_id,
> if (!state->platform.risc.fw_is_running)
> return 0;
>
> - DibAcquireLock(&state->platform.risc.mbx_if_lock);
> + if (DibAcquireLock(&state->platform.risc.mbx_if_lock) < 0) {
> + dprintk("could not get the lock");
> + return 0;
> + }
> if (risc_id == 1)
> mc_base = 16;
> else
> @@ -701,7 +713,10 @@ static int dib9000_mbx_process(struct dib9000_state *state, u16 attr)
> if (!state->platform.risc.fw_is_running)
> return -1;
>
> - DibAcquireLock(&state->platform.risc.mbx_lock);
> + if (DibAcquireLock(&state->platform.risc.mbx_lock) < 0) {
> + dprintk("could not get the lock");
> + return -1;
> + }
>
> if (dib9000_mbx_count(state, 1, attr)) /* 1=RiscB */
> ret = dib9000_mbx_fetch_to_cache(state, attr);
> @@ -1178,7 +1193,10 @@ static int dib9000_fw_get_channel(struct dvb_frontend *fe)
> struct dibDVBTChannel *ch;
> int ret = 0;
>
> - DibAcquireLock(&state->platform.risc.mem_mbx_lock);
> + if (DibAcquireLock(&state->platform.risc.mem_mbx_lock) < 0) {
> + dprintk("could not get the lock");
> + return -EINTR;
> + }
> if (dib9000_fw_memmbx_sync(state, FE_SYNC_CHANNEL) < 0) {
> ret = -EIO;
> goto error;
> @@ -1660,7 +1678,10 @@ static int dib9000_fw_component_bus_xfer(struct i2c_adapter *i2c_adap, struct i2
> p[12] = 0;
> }
>
> - DibAcquireLock(&state->platform.risc.mem_mbx_lock);
> + if (DibAcquireLock(&state->platform.risc.mem_mbx_lock) < 0) {
> + dprintk("could not get the lock");
> + return 0;
> + }
>
> dib9000_risc_mem_write(state, FE_MM_W_COMPONENT_ACCESS, p);
>
> @@ -1768,7 +1789,10 @@ int dib9000_fw_pid_filter_ctrl(struct dvb_frontend *fe, u8 onoff)
> return 0;
> }
>
> - DibAcquireLock(&state->demod_lock);
> + if (DibAcquireLock(&state->demod_lock) < 0) {
> + dprintk("could not get the lock");
> + return -EINTR;
> + }
>
> val = dib9000_read_word(state, 294 + 1) & 0xffef;
> val |= (onoff & 0x1) << 4;
> @@ -1800,7 +1824,10 @@ int dib9000_fw_pid_filter(struct dvb_frontend *fe, u8 id, u16 pid, u8 onoff)
> return 0;
> }
>
> - DibAcquireLock(&state->demod_lock);
> + if (DibAcquireLock(&state->demod_lock) < 0) {
> + dprintk("could not get the lock");
> + return -EINTR;
> + }
> dprintk("Index %x, PID %d, OnOff %d", id, pid, onoff);
> ret = dib9000_write_word(state, 300 + 1 + id,
> onoff ? (1 << 13) | pid : 0);
> @@ -1848,7 +1875,10 @@ static int dib9000_sleep(struct dvb_frontend *fe)
> u8 index_frontend;
> int ret = 0;
>
> - DibAcquireLock(&state->demod_lock);
> + if (DibAcquireLock(&state->demod_lock) < 0) {
> + dprintk("could not get the lock");
> + return -EINTR;
> + }
> for (index_frontend = 1; (index_frontend < MAX_NUMBER_OF_FRONTENDS) && (state->fe[index_frontend] != NULL); index_frontend++) {
> ret = state->fe[index_frontend]->ops.sleep(state->fe[index_frontend]);
> if (ret < 0)
> @@ -1874,8 +1904,12 @@ static int dib9000_get_frontend(struct dvb_frontend *fe)
> fe_status_t stat;
> int ret = 0;
>
> - if (state->get_frontend_internal == 0)
> - DibAcquireLock(&state->demod_lock);
> + if (state->get_frontend_internal == 0) {
> + if (DibAcquireLock(&state->demod_lock) < 0) {
> + dprintk("could not get the lock");
> + return -EINTR;
> + }
> + }
>
> for (index_frontend = 1; (index_frontend < MAX_NUMBER_OF_FRONTENDS) && (state->fe[index_frontend] != NULL); index_frontend++) {
> state->fe[index_frontend]->ops.read_status(state->fe[index_frontend], &stat);
> @@ -1978,7 +2012,10 @@ static int dib9000_set_frontend(struct dvb_frontend *fe)
> }
>
> state->pid_ctrl_index = -1; /* postpone the pid filtering cmd */
> - DibAcquireLock(&state->demod_lock);
> + if (DibAcquireLock(&state->demod_lock) < 0) {
> + dprintk("could not get the lock");
> + return 0;
> + }
>
> fe->dtv_property_cache.delivery_system = SYS_DVBT;
>
> @@ -2138,7 +2175,10 @@ static int dib9000_read_status(struct dvb_frontend *fe, fe_status_t * stat)
> u8 index_frontend;
> u16 lock = 0, lock_slave = 0;
>
> - DibAcquireLock(&state->demod_lock);
> + if (DibAcquireLock(&state->demod_lock) < 0) {
> + dprintk("could not get the lock");
> + return -EINTR;
> + }
> for (index_frontend = 1; (index_frontend < MAX_NUMBER_OF_FRONTENDS) && (state->fe[index_frontend] != NULL); index_frontend++)
> lock_slave |= dib9000_read_lock(state->fe[index_frontend]);
>
> @@ -2168,8 +2208,15 @@ static int dib9000_read_ber(struct dvb_frontend *fe, u32 * ber)
> u16 *c;
> int ret = 0;
>
> - DibAcquireLock(&state->demod_lock);
> - DibAcquireLock(&state->platform.risc.mem_mbx_lock);
> + if (DibAcquireLock(&state->demod_lock) < 0) {
> + dprintk("could not get the lock");
> + return -EINTR;
> + }
> + if (DibAcquireLock(&state->platform.risc.mem_mbx_lock) < 0) {
> + dprintk("could not get the lock");
> + ret = -EINTR;
> + goto error;
> + }
> if (dib9000_fw_memmbx_sync(state, FE_SYNC_CHANNEL) < 0) {
> DibReleaseLock(&state->platform.risc.mem_mbx_lock);
> ret = -EIO;
> @@ -2196,7 +2243,10 @@ static int dib9000_read_signal_strength(struct dvb_frontend *fe, u16 * strength)
> u16 val;
> int ret = 0;
>
> - DibAcquireLock(&state->demod_lock);
> + if (DibAcquireLock(&state->demod_lock) < 0) {
> + dprintk("could not get the lock");
> + return -EINTR;
> + }
> *strength = 0;
> for (index_frontend = 1; (index_frontend < MAX_NUMBER_OF_FRONTENDS) && (state->fe[index_frontend] != NULL); index_frontend++) {
> state->fe[index_frontend]->ops.read_signal_strength(state->fe[index_frontend], &val);
> @@ -2206,7 +2256,11 @@ static int dib9000_read_signal_strength(struct dvb_frontend *fe, u16 * strength)
> *strength += val;
> }
>
> - DibAcquireLock(&state->platform.risc.mem_mbx_lock);
> + if (DibAcquireLock(&state->platform.risc.mem_mbx_lock) < 0) {
> + dprintk("could not get the lock");
> + ret = -EINTR;
> + goto error;
> + }
> if (dib9000_fw_memmbx_sync(state, FE_SYNC_CHANNEL) < 0) {
> DibReleaseLock(&state->platform.risc.mem_mbx_lock);
> ret = -EIO;
> @@ -2233,10 +2287,13 @@ static u32 dib9000_get_snr(struct dvb_frontend *fe)
> u32 n, s, exp;
> u16 val;
>
> - DibAcquireLock(&state->platform.risc.mem_mbx_lock);
> + if (DibAcquireLock(&state->platform.risc.mem_mbx_lock) < 0) {
> + dprintk("could not get the lock");
> + return 0;
> + }
> if (dib9000_fw_memmbx_sync(state, FE_SYNC_CHANNEL) < 0) {
> DibReleaseLock(&state->platform.risc.mem_mbx_lock);
> - return -EIO;
> + return 0;
> }
> dib9000_risc_mem_read(state, FE_MM_R_FE_MONITOR, (u8 *) c, 16 * 2);
> DibReleaseLock(&state->platform.risc.mem_mbx_lock);
> @@ -2269,7 +2326,10 @@ static int dib9000_read_snr(struct dvb_frontend *fe, u16 * snr)
> u8 index_frontend;
> u32 snr_master;
>
> - DibAcquireLock(&state->demod_lock);
> + if (DibAcquireLock(&state->demod_lock) < 0) {
> + dprintk("could not get the lock");
> + return -EINTR;
> + }
> snr_master = dib9000_get_snr(fe);
> for (index_frontend = 1; (index_frontend < MAX_NUMBER_OF_FRONTENDS) && (state->fe[index_frontend] != NULL); index_frontend++)
> snr_master += dib9000_get_snr(state->fe[index_frontend]);
> @@ -2291,8 +2351,15 @@ static int dib9000_read_unc_blocks(struct dvb_frontend *fe, u32 * unc)
> u16 *c = (u16 *)state->i2c_read_buffer;
> int ret = 0;
>
> - DibAcquireLock(&state->demod_lock);
> - DibAcquireLock(&state->platform.risc.mem_mbx_lock);
> + if (DibAcquireLock(&state->demod_lock) < 0) {
> + dprintk("could not get the lock");
> + return -EINTR;
> + }
> + if (DibAcquireLock(&state->platform.risc.mem_mbx_lock) < 0) {
> + dprintk("could not get the lock");
> + ret = -EINTR;
> + goto error;
> + }
> if (dib9000_fw_memmbx_sync(state, FE_SYNC_CHANNEL) < 0) {
> DibReleaseLock(&state->platform.risc.mem_mbx_lock);
> ret = -EIO;
next prev parent reply other threads:[~2012-03-19 17:35 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-03-06 21:08 [PATCH 2/2] [media] dib9000: implement error handling for DibAcquireLock Alexey Khoroshilov
2012-03-19 17:35 ` Mauro Carvalho Chehab [this message]
2012-03-19 20:48 ` [PATCH] [media] dib9000: get rid of Dib*Lock macros Alexey Khoroshilov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4F676E55.1060003@redhat.com \
--to=mchehab@redhat.com \
--cc=Patrick.Boettcher@dibcom.fr \
--cc=khoroshilov@ispras.ru \
--cc=ldv-project@ispras.ru \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@infradead.org \
--cc=olivier.grenie@dibcom.fr \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.