2024-05-06 21:10:46

by Ricardo Ribalda

[permalink] [raw]
Subject: [PATCH 0/5] media: Fix some cocci locks warnings

After this set is merged, there are no preceding lock warnings.

Signed-off-by: Ricardo Ribalda <[email protected]>
---
Ricardo Ribalda (5):
media: ivtv: Factor out schedule functions
media: imon: Fix race getting ictx->lock
media: dvb-frontends/stv090x: Refactor tuner_i2c_lock
media: go7007: Refactor Adlink PCI-MPG24 i2c mutex
media: drivers/media/dvb-core: Refactor dvb_frontend_open

drivers/media/dvb-core/dvb_frontend.c | 116 ++++++++++++++++++++--------------
drivers/media/dvb-frontends/stv090x.c | 37 ++++++-----
drivers/media/pci/ivtv/ivtv-fileops.c | 66 +++++++++++--------
drivers/media/rc/imon.c | 5 +-
drivers/media/usb/go7007/go7007-i2c.c | 30 +++++----
5 files changed, 149 insertions(+), 105 deletions(-)
---
base-commit: e695668af8523b059127dfa8b261c76e7c9cde10
change-id: 20240506-cocci-locks-ef2b7ac6931e

Best regards,
--
Ricardo Ribalda <[email protected]>



2024-05-06 21:10:51

by Ricardo Ribalda

[permalink] [raw]
Subject: [PATCH 1/5] media: ivtv: Factor out schedule functions

Cocci is very confused by unlock-lock a mutex in the middle of a
function.

Factor the schedules out, avoid code duplication and make cocci a bit
happier.

Fix the following cocci warnings:
drivers/media/pci/ivtv/ivtv-fileops.c:223:4-10: preceding lock on line 267
drivers/media/pci/ivtv/ivtv-fileops.c:230:3-9: preceding lock on line 267
drivers/media/pci/ivtv/ivtv-fileops.c:236:4-10: preceding lock on line 267
drivers/media/pci/ivtv/ivtv-fileops.c:245:3-9: preceding lock on line 267
drivers/media/pci/ivtv/ivtv-fileops.c:251:3-9: preceding lock on line 267
drivers/media/pci/ivtv/ivtv-fileops.c:257:3-9: preceding lock on line 267
drivers/media/pci/ivtv/ivtv-fileops.c:272:3-9: preceding lock on line 267
drivers/media/pci/ivtv/ivtv-fileops.c:598:4-10: preceding lock on line 627
drivers/media/pci/ivtv/ivtv-fileops.c:598:4-10: preceding lock on line 689
drivers/media/pci/ivtv/ivtv-fileops.c:606:3-9: preceding lock on line 627
drivers/media/pci/ivtv/ivtv-fileops.c:606:3-9: preceding lock on line 689
drivers/media/pci/ivtv/ivtv-fileops.c:648:3-9: preceding lock on line 627
drivers/media/pci/ivtv/ivtv-fileops.c:648:3-9: preceding lock on line 689
drivers/media/pci/ivtv/ivtv-fileops.c:692:4-10: preceding lock on line 689

Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/pci/ivtv/ivtv-fileops.c | 66 +++++++++++++++++++++--------------
1 file changed, 39 insertions(+), 27 deletions(-)

diff --git a/drivers/media/pci/ivtv/ivtv-fileops.c b/drivers/media/pci/ivtv/ivtv-fileops.c
index 4202c3a47d33..cfa28d035586 100644
--- a/drivers/media/pci/ivtv/ivtv-fileops.c
+++ b/drivers/media/pci/ivtv/ivtv-fileops.c
@@ -21,6 +21,7 @@
#include "ivtv-ioctl.h"
#include "ivtv-cards.h"
#include "ivtv-firmware.h"
+#include <linux/lockdep.h>
#include <media/v4l2-event.h>
#include <media/i2c/saa7115.h>

@@ -190,12 +191,27 @@ static void ivtv_update_pgm_info(struct ivtv *itv)
itv->pgm_info_write_idx = (itv->pgm_info_write_idx + i) % itv->pgm_info_num;
}

+static void ivtv_schedule(struct ivtv_stream *s)
+{
+ struct ivtv *itv = s->itv;
+ DEFINE_WAIT(wait);
+
+ lockdep_assert_held(&itv->serialize_lock);
+
+ mutex_unlock(&itv->serialize_lock);
+ prepare_to_wait(&s->waitq, &wait, TASK_INTERRUPTIBLE);
+ /* New buffers might have become free before we were added to the waitqueue */
+ if (!s->q_free.buffers)
+ schedule();
+ finish_wait(&s->waitq, &wait);
+ mutex_lock(&itv->serialize_lock);
+}
+
static struct ivtv_buffer *ivtv_get_buffer(struct ivtv_stream *s, int non_block, int *err)
{
struct ivtv *itv = s->itv;
struct ivtv_stream *s_vbi = &itv->streams[IVTV_ENC_STREAM_TYPE_VBI];
struct ivtv_buffer *buf;
- DEFINE_WAIT(wait);

*err = 0;
while (1) {
@@ -258,13 +274,7 @@ static struct ivtv_buffer *ivtv_get_buffer(struct ivtv_stream *s, int non_block,
}

/* wait for more data to arrive */
- mutex_unlock(&itv->serialize_lock);
- prepare_to_wait(&s->waitq, &wait, TASK_INTERRUPTIBLE);
- /* New buffers might have become available before we were added to the waitqueue */
- if (!s->q_full.buffers)
- schedule();
- finish_wait(&s->waitq, &wait);
- mutex_lock(&itv->serialize_lock);
+ ivtv_schedule(s);
if (signal_pending(current)) {
/* return if a signal was received */
IVTV_DEBUG_INFO("User stopped %s\n", s->name);
@@ -533,6 +543,25 @@ int ivtv_start_decoding(struct ivtv_open_id *id, int speed)
return 0;
}

+static int ivtv_schedule_dma(struct ivtv_stream *s)
+{
+ struct ivtv *itv = s->itv;
+ int got_sig;
+ DEFINE_WAIT(wait);
+
+ lockdep_assert_held(&itv->serialize_lock);
+
+ mutex_unlock(&itv->serialize_lock);
+ prepare_to_wait(&itv->dma_waitq, &wait, TASK_INTERRUPTIBLE);
+ while (!(got_sig = signal_pending(current)) &&
+ test_bit(IVTV_F_S_DMA_PENDING, &s->s_flags))
+ schedule();
+ finish_wait(&itv->dma_waitq, &wait);
+ mutex_lock(&itv->serialize_lock);
+
+ return got_sig;
+}
+
static ssize_t ivtv_write(struct file *filp, const char __user *user_buf, size_t count, loff_t *pos)
{
struct ivtv_open_id *id = fh2id(filp->private_data);
@@ -544,7 +573,6 @@ static ssize_t ivtv_write(struct file *filp, const char __user *user_buf, size_t
int bytes_written = 0;
int mode;
int rc;
- DEFINE_WAIT(wait);

IVTV_DEBUG_HI_FILE("write %zd bytes to %s\n", count, s->name);

@@ -618,13 +646,7 @@ static ssize_t ivtv_write(struct file *filp, const char __user *user_buf, size_t
break;
if (filp->f_flags & O_NONBLOCK)
return -EAGAIN;
- mutex_unlock(&itv->serialize_lock);
- prepare_to_wait(&s->waitq, &wait, TASK_INTERRUPTIBLE);
- /* New buffers might have become free before we were added to the waitqueue */
- if (!s->q_free.buffers)
- schedule();
- finish_wait(&s->waitq, &wait);
- mutex_lock(&itv->serialize_lock);
+ ivtv_schedule(s);
if (signal_pending(current)) {
IVTV_DEBUG_INFO("User stopped %s\n", s->name);
return -EINTR;
@@ -674,20 +696,10 @@ static ssize_t ivtv_write(struct file *filp, const char __user *user_buf, size_t

if (test_bit(IVTV_F_S_NEEDS_DATA, &s->s_flags)) {
if (s->q_full.length >= itv->dma_data_req_size) {
- int got_sig;
-
if (mode == OUT_YUV)
ivtv_yuv_setup_stream_frame(itv);

- mutex_unlock(&itv->serialize_lock);
- prepare_to_wait(&itv->dma_waitq, &wait, TASK_INTERRUPTIBLE);
- while (!(got_sig = signal_pending(current)) &&
- test_bit(IVTV_F_S_DMA_PENDING, &s->s_flags)) {
- schedule();
- }
- finish_wait(&itv->dma_waitq, &wait);
- mutex_lock(&itv->serialize_lock);
- if (got_sig) {
+ if (ivtv_schedule_dma(s)) {
IVTV_DEBUG_INFO("User interrupted %s\n", s->name);
return -EINTR;
}

--
2.45.0.rc1.225.g2a3ae87e7f-goog


2024-05-06 21:11:10

by Ricardo Ribalda

[permalink] [raw]
Subject: [PATCH 3/5] media: dvb-frontends/stv090x: Refactor tuner_i2c_lock

Move the lock logic to it's own function. There is less code duplication
and cocci is much happier.

Fix the following cocci warning:
drivers/media/dvb-frontends/stv090x.c:799:1-7: preceding lock on line 768

Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/dvb-frontends/stv090x.c | 37 ++++++++++++++++++++---------------
1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/media/dvb-frontends/stv090x.c b/drivers/media/dvb-frontends/stv090x.c
index cc45139057ba..3b02d504941f 100644
--- a/drivers/media/dvb-frontends/stv090x.c
+++ b/drivers/media/dvb-frontends/stv090x.c
@@ -748,6 +748,22 @@ static int stv090x_write_reg(struct stv090x_state *state, unsigned int reg, u8 d
return stv090x_write_regs(state, reg, &tmp, 1);
}

+static inline void stv090x_tuner_i2c_lock(struct stv090x_state *state)
+{
+ if (state->config->tuner_i2c_lock)
+ state->config->tuner_i2c_lock(&state->frontend, 1);
+ else
+ mutex_lock(&state->internal->tuner_lock);
+}
+
+static inline void stv090x_tuner_i2c_unlock(struct stv090x_state *state)
+{
+ if (state->config->tuner_i2c_lock)
+ state->config->tuner_i2c_lock(&state->frontend, 0);
+ else
+ mutex_unlock(&state->internal->tuner_lock);
+}
+
static int stv090x_i2c_gate_ctrl(struct stv090x_state *state, int enable)
{
u32 reg;
@@ -761,12 +777,8 @@ static int stv090x_i2c_gate_ctrl(struct stv090x_state *state, int enable)
* In case of any error, the lock is unlocked and exit within the
* relevant operations themselves.
*/
- if (enable) {
- if (state->config->tuner_i2c_lock)
- state->config->tuner_i2c_lock(&state->frontend, 1);
- else
- mutex_lock(&state->internal->tuner_lock);
- }
+ if (enable)
+ stv090x_tuner_i2c_lock(state);

reg = STV090x_READ_DEMOD(state, I2CRPT);
if (enable) {
@@ -782,20 +794,13 @@ static int stv090x_i2c_gate_ctrl(struct stv090x_state *state, int enable)
goto err;
}

- if (!enable) {
- if (state->config->tuner_i2c_lock)
- state->config->tuner_i2c_lock(&state->frontend, 0);
- else
- mutex_unlock(&state->internal->tuner_lock);
- }
+ if (!enable)
+ stv090x_tuner_i2c_unlock(state);

return 0;
err:
dprintk(FE_ERROR, 1, "I/O error");
- if (state->config->tuner_i2c_lock)
- state->config->tuner_i2c_lock(&state->frontend, 0);
- else
- mutex_unlock(&state->internal->tuner_lock);
+ stv090x_tuner_i2c_unlock(state);
return -1;
}


--
2.45.0.rc1.225.g2a3ae87e7f-goog


2024-05-06 21:11:22

by Ricardo Ribalda

[permalink] [raw]
Subject: [PATCH 2/5] media: imon: Fix race getting ictx->lock

Lets fix a race between mutex_is_lock() and mutex_lock().

<-mutex is not locked
if (!mutex_is_locked(&ictx->lock)) {
unlock = true; <- mutex is locked externaly
mutex_lock(&ictx->lock);
}

Let's use mutex_trylock() that does mutex_is_lock() and mutex_lock()
atomically.

Fix the following cocci warning:
drivers/media/rc/imon.c:1167:1-7: preceding lock on line 1153

Fixes: 23ef710e1a6c ("[media] imon: add conditional locking in change_protocol")
Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/rc/imon.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
index 0b55314a8082..8f1361bcce3a 100644
--- a/drivers/media/rc/imon.c
+++ b/drivers/media/rc/imon.c
@@ -1148,10 +1148,7 @@ static int imon_ir_change_protocol(struct rc_dev *rc, u64 *rc_proto)

memcpy(ictx->usb_tx_buf, &ir_proto_packet, sizeof(ir_proto_packet));

- if (!mutex_is_locked(&ictx->lock)) {
- unlock = true;
- mutex_lock(&ictx->lock);
- }
+ unlock = mutex_trylock(&ictx->lock);

retval = send_packet(ictx);
if (retval)

--
2.45.0.rc1.225.g2a3ae87e7f-goog


2024-05-06 21:11:24

by Ricardo Ribalda

[permalink] [raw]
Subject: [PATCH 4/5] media: go7007: Refactor Adlink PCI-MPG24 i2c mutex

Move the lock/unlock to its own function. It makes the code cleaner and
cocci happier.

Fix the following cocci warning:
drivers/media/usb/go7007/go7007-i2c.c:125:1-7: preceding lock on line 61

Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/usb/go7007/go7007-i2c.c | 30 +++++++++++++++++++-----------
1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/media/usb/go7007/go7007-i2c.c b/drivers/media/usb/go7007/go7007-i2c.c
index 2880370e45c8..f6ce28a4a768 100644
--- a/drivers/media/usb/go7007/go7007-i2c.c
+++ b/drivers/media/usb/go7007/go7007-i2c.c
@@ -33,7 +33,21 @@

/* There is only one I2C port on the TW2804 that feeds all four GO7007 VIPs
* on the Adlink PCI-MPG24, so access is shared between all of them. */
-static DEFINE_MUTEX(adlink_mpg24_i2c_lock);
+static DEFINE_MUTEX(adlink_mpg24_i2c_mutex);
+
+static inline void adlink_mpg24_i2c_lock(struct go7007 *go)
+{
+ /* Bridge the I2C port on this GO7007 to the shared bus */
+ mutex_lock(&adlink_mpg24_i2c_mutex);
+ go7007_write_addr(go, 0x3c82, 0x0020);
+}
+
+static inline void adlink_mpg24_i2c_unlock(struct go7007 *go)
+{
+ /* Isolate the I2C port on this GO7007 from the shared bus */
+ go7007_write_addr(go, 0x3c82, 0x0000);
+ mutex_unlock(&adlink_mpg24_i2c_mutex);
+}

static int go7007_i2c_xfer(struct go7007 *go, u16 addr, int read,
u16 command, int flags, u8 *data)
@@ -56,11 +70,8 @@ static int go7007_i2c_xfer(struct go7007 *go, u16 addr, int read,

mutex_lock(&go->hw_lock);

- if (go->board_id == GO7007_BOARDID_ADLINK_MPG24) {
- /* Bridge the I2C port on this GO7007 to the shared bus */
- mutex_lock(&adlink_mpg24_i2c_lock);
- go7007_write_addr(go, 0x3c82, 0x0020);
- }
+ if (go->board_id == GO7007_BOARDID_ADLINK_MPG24)
+ adlink_mpg24_i2c_lock(go);

/* Wait for I2C adapter to be ready */
for (i = 0; i < 10; ++i) {
@@ -116,11 +127,8 @@ static int go7007_i2c_xfer(struct go7007 *go, u16 addr, int read,
ret = 0;

i2c_done:
- if (go->board_id == GO7007_BOARDID_ADLINK_MPG24) {
- /* Isolate the I2C port on this GO7007 from the shared bus */
- go7007_write_addr(go, 0x3c82, 0x0000);
- mutex_unlock(&adlink_mpg24_i2c_lock);
- }
+ if (go->board_id == GO7007_BOARDID_ADLINK_MPG24)
+ adlink_mpg24_i2c_unlock(go);
mutex_unlock(&go->hw_lock);
return ret;
}

--
2.45.0.rc1.225.g2a3ae87e7f-goog


2024-05-06 21:14:16

by Ricardo Ribalda

[permalink] [raw]
Subject: [PATCH 5/5] media: drivers/media/dvb-core: Refactor dvb_frontend_open

Move the shared frontend logic to its own function. This allows using
guard() to handle the mfe_lock mutex, and using lockdep asserts.

There is no intended change of behavior in this code.

This fixes the following cocci warnings:
drivers/media/dvb-core/dvb_frontend.c:2897:1-7: preceding lock on line 2776
drivers/media/dvb-core/dvb_frontend.c:2897:1-7: preceding lock on line 2786
drivers/media/dvb-core/dvb_frontend.c:2897:1-7: preceding lock on line 2809

Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/dvb-core/dvb_frontend.c | 116 ++++++++++++++++++++--------------
1 file changed, 69 insertions(+), 47 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index 4f78f30b3646..d04a6092f1ba 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -30,6 +30,7 @@
#include <linux/kthread.h>
#include <linux/ktime.h>
#include <linux/compat.h>
+#include <linux/lockdep.h>
#include <asm/processor.h>

#include <media/dvb_frontend.h>
@@ -2760,66 +2761,70 @@ static __poll_t dvb_frontend_poll(struct file *file, struct poll_table_struct *w
return 0;
}

-static int dvb_frontend_open(struct inode *inode, struct file *file)
+static int dvb_get_shared_frontend(struct dvb_adapter *adapter,
+ struct dvb_device *dvbdev,
+ struct file *file)
{
- struct dvb_device *dvbdev = file->private_data;
- struct dvb_frontend *fe = dvbdev->priv;
- struct dvb_frontend_private *fepriv = fe->frontend_priv;
- struct dvb_adapter *adapter = fe->dvb;
- int ret;
+ struct dvb_device *mfedev;
+ struct dvb_frontend *mfe;
+ struct dvb_frontend_private *mfepriv;
+ int mferetry;

- dev_dbg(fe->dvb->device, "%s:\n", __func__);
- if (fe->exit == DVB_FE_DEVICE_REMOVED)
- return -ENODEV;
+ lockdep_assert_held(&adapter->mfe_lock);

if (adapter->mfe_shared == 2) {
- mutex_lock(&adapter->mfe_lock);
if ((file->f_flags & O_ACCMODE) != O_RDONLY) {
if (adapter->mfe_dvbdev &&
!adapter->mfe_dvbdev->writers) {
- mutex_unlock(&adapter->mfe_lock);
return -EBUSY;
}
adapter->mfe_dvbdev = dvbdev;
}
- } else if (adapter->mfe_shared) {
- mutex_lock(&adapter->mfe_lock);
+ return 0;
+ }

- if (!adapter->mfe_dvbdev)
- adapter->mfe_dvbdev = dvbdev;
+ if (!adapter->mfe_dvbdev) {
+ adapter->mfe_dvbdev = dvbdev;
+ return 0;
+ }

- else if (adapter->mfe_dvbdev != dvbdev) {
- struct dvb_device
- *mfedev = adapter->mfe_dvbdev;
- struct dvb_frontend
- *mfe = mfedev->priv;
- struct dvb_frontend_private
- *mfepriv = mfe->frontend_priv;
- int mferetry = (dvb_mfe_wait_time << 1);
-
- mutex_unlock(&adapter->mfe_lock);
- while (mferetry-- && (mfedev->users != -1 ||
- mfepriv->thread)) {
- if (msleep_interruptible(500)) {
- if (signal_pending(current))
- return -EINTR;
- }
- }
+ if (adapter->mfe_dvbdev == dvbdev)
+ return 0;

- mutex_lock(&adapter->mfe_lock);
- if (adapter->mfe_dvbdev != dvbdev) {
- mfedev = adapter->mfe_dvbdev;
- mfe = mfedev->priv;
- mfepriv = mfe->frontend_priv;
- if (mfedev->users != -1 ||
- mfepriv->thread) {
- mutex_unlock(&adapter->mfe_lock);
- return -EBUSY;
- }
- adapter->mfe_dvbdev = dvbdev;
+ mfedev = adapter->mfe_dvbdev;
+ mfe = mfedev->priv;
+ mfepriv = mfe->frontend_priv;
+ mferetry = (dvb_mfe_wait_time << 1);
+
+ mutex_unlock(&adapter->mfe_lock);
+ while (mferetry-- && (mfedev->users != -1 || mfepriv->thread)) {
+ if (msleep_interruptible(500)) {
+ if (signal_pending(current)) {
+ mutex_lock(&adapter->mfe_lock);
+ return -EINTR;
}
}
}
+ mutex_lock(&adapter->mfe_lock);
+
+ if (adapter->mfe_dvbdev != dvbdev) {
+ mfedev = adapter->mfe_dvbdev;
+ mfe = mfedev->priv;
+ mfepriv = mfe->frontend_priv;
+ if (mfedev->users != -1 || mfepriv->thread)
+ return -EBUSY;
+ adapter->mfe_dvbdev = dvbdev;
+ }
+
+ return 0;
+}
+
+static int __dvb_frontend_open(struct inode *inode, struct file *file)
+{
+ struct dvb_device *dvbdev = file->private_data;
+ struct dvb_frontend *fe = dvbdev->priv;
+ struct dvb_frontend_private *fepriv = fe->frontend_priv;
+ int ret;

if (dvbdev->users == -1 && fe->ops.ts_bus_ctrl) {
if ((ret = fe->ops.ts_bus_ctrl(fe, 1)) < 0)
@@ -2871,8 +2876,6 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)

dvb_frontend_get(fe);

- if (adapter->mfe_shared)
- mutex_unlock(&adapter->mfe_lock);
return ret;

err3:
@@ -2892,11 +2895,30 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
if (dvbdev->users == -1 && fe->ops.ts_bus_ctrl)
fe->ops.ts_bus_ctrl(fe, 0);
err0:
- if (adapter->mfe_shared)
- mutex_unlock(&adapter->mfe_lock);
return ret;
}

+static int dvb_frontend_open(struct inode *inode, struct file *file)
+{
+ struct dvb_device *dvbdev = file->private_data;
+ struct dvb_frontend *fe = dvbdev->priv;
+ struct dvb_adapter *adapter = fe->dvb;
+ int ret;
+
+ dev_dbg(fe->dvb->device, "%s:\n", __func__);
+ if (fe->exit == DVB_FE_DEVICE_REMOVED)
+ return -ENODEV;
+
+ if (!adapter->mfe_shared)
+ return __dvb_frontend_open(inode, file);
+
+ guard(mutex)(&adapter->mfe_lock);
+ ret = dvb_get_shared_frontend(adapter, dvbdev, file);
+ if (ret)
+ return ret;
+ return __dvb_frontend_open(inode, file);
+}
+
static int dvb_frontend_release(struct inode *inode, struct file *file)
{
struct dvb_device *dvbdev = file->private_data;

--
2.45.0.rc1.225.g2a3ae87e7f-goog


2024-05-30 13:20:10

by Hans Verkuil

[permalink] [raw]
Subject: Re: [PATCH 5/5] media: drivers/media/dvb-core: Refactor dvb_frontend_open

On 06/05/2024 23:10, Ricardo Ribalda wrote:
> Move the shared frontend logic to its own function. This allows using
> guard() to handle the mfe_lock mutex, and using lockdep asserts.
>
> There is no intended change of behavior in this code.
>
> This fixes the following cocci warnings:
> drivers/media/dvb-core/dvb_frontend.c:2897:1-7: preceding lock on line 2776
> drivers/media/dvb-core/dvb_frontend.c:2897:1-7: preceding lock on line 2786
> drivers/media/dvb-core/dvb_frontend.c:2897:1-7: preceding lock on line 2809

OK, this patch is really hard to review. And since it is refactoring core
framework code, I think this might work better if it is reworked.

I *think* (not 100% certain) that it is enough to just split off
__dvb_frontend_open.

The dvb_frontend_open() can then just do:

if (!adapter->mfe_shared)
return __dvb_frontend_open(inode, file);

and after that you can just take the mfe_lock and keep the existing code.
While you can introduce the use of guard(), I don't think it is necessary.
This keeps the 'get_shared_frontend' code in dvb_frontend_open() rather
then splitting it up, but I think this will produce a much more readable patch.

Which IMHO is really important for code like this.

Regards,

Hans

>
> Signed-off-by: Ricardo Ribalda <[email protected]>
> ---
> drivers/media/dvb-core/dvb_frontend.c | 116 ++++++++++++++++++++--------------
> 1 file changed, 69 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> index 4f78f30b3646..d04a6092f1ba 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -30,6 +30,7 @@
> #include <linux/kthread.h>
> #include <linux/ktime.h>
> #include <linux/compat.h>
> +#include <linux/lockdep.h>
> #include <asm/processor.h>
>
> #include <media/dvb_frontend.h>
> @@ -2760,66 +2761,70 @@ static __poll_t dvb_frontend_poll(struct file *file, struct poll_table_struct *w
> return 0;
> }
>
> -static int dvb_frontend_open(struct inode *inode, struct file *file)
> +static int dvb_get_shared_frontend(struct dvb_adapter *adapter,
> + struct dvb_device *dvbdev,
> + struct file *file)
> {
> - struct dvb_device *dvbdev = file->private_data;
> - struct dvb_frontend *fe = dvbdev->priv;
> - struct dvb_frontend_private *fepriv = fe->frontend_priv;
> - struct dvb_adapter *adapter = fe->dvb;
> - int ret;
> + struct dvb_device *mfedev;
> + struct dvb_frontend *mfe;
> + struct dvb_frontend_private *mfepriv;
> + int mferetry;
>
> - dev_dbg(fe->dvb->device, "%s:\n", __func__);
> - if (fe->exit == DVB_FE_DEVICE_REMOVED)
> - return -ENODEV;
> + lockdep_assert_held(&adapter->mfe_lock);
>
> if (adapter->mfe_shared == 2) {
> - mutex_lock(&adapter->mfe_lock);
> if ((file->f_flags & O_ACCMODE) != O_RDONLY) {
> if (adapter->mfe_dvbdev &&
> !adapter->mfe_dvbdev->writers) {
> - mutex_unlock(&adapter->mfe_lock);
> return -EBUSY;
> }
> adapter->mfe_dvbdev = dvbdev;
> }
> - } else if (adapter->mfe_shared) {
> - mutex_lock(&adapter->mfe_lock);
> + return 0;
> + }
>
> - if (!adapter->mfe_dvbdev)
> - adapter->mfe_dvbdev = dvbdev;
> + if (!adapter->mfe_dvbdev) {
> + adapter->mfe_dvbdev = dvbdev;
> + return 0;
> + }
>
> - else if (adapter->mfe_dvbdev != dvbdev) {
> - struct dvb_device
> - *mfedev = adapter->mfe_dvbdev;
> - struct dvb_frontend
> - *mfe = mfedev->priv;
> - struct dvb_frontend_private
> - *mfepriv = mfe->frontend_priv;
> - int mferetry = (dvb_mfe_wait_time << 1);
> -
> - mutex_unlock(&adapter->mfe_lock);
> - while (mferetry-- && (mfedev->users != -1 ||
> - mfepriv->thread)) {
> - if (msleep_interruptible(500)) {
> - if (signal_pending(current))
> - return -EINTR;
> - }
> - }
> + if (adapter->mfe_dvbdev == dvbdev)
> + return 0;
>
> - mutex_lock(&adapter->mfe_lock);
> - if (adapter->mfe_dvbdev != dvbdev) {
> - mfedev = adapter->mfe_dvbdev;
> - mfe = mfedev->priv;
> - mfepriv = mfe->frontend_priv;
> - if (mfedev->users != -1 ||
> - mfepriv->thread) {
> - mutex_unlock(&adapter->mfe_lock);
> - return -EBUSY;
> - }
> - adapter->mfe_dvbdev = dvbdev;
> + mfedev = adapter->mfe_dvbdev;
> + mfe = mfedev->priv;
> + mfepriv = mfe->frontend_priv;
> + mferetry = (dvb_mfe_wait_time << 1);
> +
> + mutex_unlock(&adapter->mfe_lock);
> + while (mferetry-- && (mfedev->users != -1 || mfepriv->thread)) {
> + if (msleep_interruptible(500)) {
> + if (signal_pending(current)) {
> + mutex_lock(&adapter->mfe_lock);
> + return -EINTR;
> }
> }
> }
> + mutex_lock(&adapter->mfe_lock);
> +
> + if (adapter->mfe_dvbdev != dvbdev) {
> + mfedev = adapter->mfe_dvbdev;
> + mfe = mfedev->priv;
> + mfepriv = mfe->frontend_priv;
> + if (mfedev->users != -1 || mfepriv->thread)
> + return -EBUSY;
> + adapter->mfe_dvbdev = dvbdev;
> + }
> +
> + return 0;
> +}
> +
> +static int __dvb_frontend_open(struct inode *inode, struct file *file)
> +{
> + struct dvb_device *dvbdev = file->private_data;
> + struct dvb_frontend *fe = dvbdev->priv;
> + struct dvb_frontend_private *fepriv = fe->frontend_priv;
> + int ret;
>
> if (dvbdev->users == -1 && fe->ops.ts_bus_ctrl) {
> if ((ret = fe->ops.ts_bus_ctrl(fe, 1)) < 0)
> @@ -2871,8 +2876,6 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
>
> dvb_frontend_get(fe);
>
> - if (adapter->mfe_shared)
> - mutex_unlock(&adapter->mfe_lock);
> return ret;
>
> err3:
> @@ -2892,11 +2895,30 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
> if (dvbdev->users == -1 && fe->ops.ts_bus_ctrl)
> fe->ops.ts_bus_ctrl(fe, 0);
> err0:
> - if (adapter->mfe_shared)
> - mutex_unlock(&adapter->mfe_lock);
> return ret;
> }
>
> +static int dvb_frontend_open(struct inode *inode, struct file *file)
> +{
> + struct dvb_device *dvbdev = file->private_data;
> + struct dvb_frontend *fe = dvbdev->priv;
> + struct dvb_adapter *adapter = fe->dvb;
> + int ret;
> +
> + dev_dbg(fe->dvb->device, "%s:\n", __func__);
> + if (fe->exit == DVB_FE_DEVICE_REMOVED)
> + return -ENODEV;
> +
> + if (!adapter->mfe_shared)
> + return __dvb_frontend_open(inode, file);
> +
> + guard(mutex)(&adapter->mfe_lock);
> + ret = dvb_get_shared_frontend(adapter, dvbdev, file);
> + if (ret)
> + return ret;
> + return __dvb_frontend_open(inode, file);
> +}
> +
> static int dvb_frontend_release(struct inode *inode, struct file *file)
> {
> struct dvb_device *dvbdev = file->private_data;
>