* [PATCH v2 0/2] media: Follow-up for coccinelle lock fixes patchset
@ 2024-08-14 14:10 Ricardo Ribalda
2024-08-14 14:10 ` [PATCH v2 1/2] media: drivers/media/dvb-core: Split dvb_frontend_open() Ricardo Ribalda
2024-08-14 14:10 ` [PATCH v2 2/2] media: drivers/media/dvb-core: Refactor dvb_frontend_open locking Ricardo Ribalda
0 siblings, 2 replies; 7+ messages in thread
From: Ricardo Ribalda @ 2024-08-14 14:10 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: linux-media, linux-kernel, Hans Verkuil, Ricardo Ribalda
Hans requested some changes for this patch:
https://patchwork.linuxtv.org/project/linux-media/patch/20240506-cocci-locks-v1-5-a67952fe5d19@chromium.org/
Refactor the patch and try again :).
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Changes in v2:
- Checkpatch fixes.
- Link to v1: https://lore.kernel.org/r/20240611-coccinelle-followup-v1-0-df2de9c2f320@chromium.org
---
Ricardo Ribalda (2):
media: drivers/media/dvb-core: Split dvb_frontend_open()
media: drivers/media/dvb-core: Refactor dvb_frontend_open locking
drivers/media/dvb-core/dvb_frontend.c | 159 ++++++++++++++++++----------------
1 file changed, 86 insertions(+), 73 deletions(-)
---
base-commit: 03a979b74dc1ad5aeed8026a84d8771842cb1631
change-id: 20240611-coccinelle-followup-f5a62b095f20
Best regards,
--
Ricardo Ribalda <ribalda@chromium.org>
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v2 1/2] media: drivers/media/dvb-core: Split dvb_frontend_open() 2024-08-14 14:10 [PATCH v2 0/2] media: Follow-up for coccinelle lock fixes patchset Ricardo Ribalda @ 2024-08-14 14:10 ` Ricardo Ribalda 2024-08-16 12:30 ` Ricardo Ribalda 2024-08-14 14:10 ` [PATCH v2 2/2] media: drivers/media/dvb-core: Refactor dvb_frontend_open locking Ricardo Ribalda 1 sibling, 1 reply; 7+ messages in thread From: Ricardo Ribalda @ 2024-08-14 14:10 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: linux-media, linux-kernel, Hans Verkuil, Ricardo Ribalda Move the actual opening to its own function. Not intended code change. This is a preparation for the next patch. Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/dvb-core/dvb_frontend.c | 143 +++++++++++++++++----------------- 1 file changed, 70 insertions(+), 73 deletions(-) diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c index 4f78f30b3646..e81b9996530e 100644 --- a/drivers/media/dvb-core/dvb_frontend.c +++ b/drivers/media/dvb-core/dvb_frontend.c @@ -2760,80 +2760,13 @@ 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_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; - 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 == 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); - - if (!adapter->mfe_dvbdev) - adapter->mfe_dvbdev = dvbdev; - - 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; - } - } - - 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; - } - } - } - - if (dvbdev->users == -1 && fe->ops.ts_bus_ctrl) { - if ((ret = fe->ops.ts_bus_ctrl(fe, 1)) < 0) - goto err0; - - /* If we took control of the bus, we need to force - reinitialization. This is because many ts_bus_ctrl() - functions strobe the RESET pin on the demod, and if the - frontend thread already exists then the dvb_init() routine - won't get called (which is what usually does initial - register configuration). */ - fepriv->reinitialise = 1; - } - if ((ret = dvb_generic_open(inode, file)) < 0) goto err1; @@ -2871,8 +2804,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: @@ -2891,9 +2822,75 @@ static int dvb_frontend_open(struct inode *inode, struct file *file) err1: 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); + + 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 { + mutex_lock(&adapter->mfe_lock); + + if (!adapter->mfe_dvbdev) { + adapter->mfe_dvbdev = dvbdev; + } 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; + } + } + + 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; + } + } + } + + ret = __dvb_frontend_open(inode, file); + mutex_unlock(&adapter->mfe_lock); + return ret; } -- 2.46.0.76.ge559c4bf1a-goog ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] media: drivers/media/dvb-core: Split dvb_frontend_open() 2024-08-14 14:10 ` [PATCH v2 1/2] media: drivers/media/dvb-core: Split dvb_frontend_open() Ricardo Ribalda @ 2024-08-16 12:30 ` Ricardo Ribalda 0 siblings, 0 replies; 7+ messages in thread From: Ricardo Ribalda @ 2024-08-16 12:30 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: linux-media, linux-kernel, Hans Verkuil Hi On Wed, 14 Aug 2024 at 16:10, Ricardo Ribalda <ribalda@chromium.org> wrote: > > Move the actual opening to its own function. > > Not intended code change. This is a preparation for the next patch. > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/dvb-core/dvb_frontend.c | 143 +++++++++++++++++----------------- > 1 file changed, 70 insertions(+), 73 deletions(-) > > diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c > index 4f78f30b3646..e81b9996530e 100644 > --- a/drivers/media/dvb-core/dvb_frontend.c > +++ b/drivers/media/dvb-core/dvb_frontend.c > @@ -2760,80 +2760,13 @@ 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_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; > - 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 == 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); > - > - if (!adapter->mfe_dvbdev) > - adapter->mfe_dvbdev = dvbdev; > - > - 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; > - } > - } > - > - 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; > - } > - } > - } > - > - if (dvbdev->users == -1 && fe->ops.ts_bus_ctrl) { > - if ((ret = fe->ops.ts_bus_ctrl(fe, 1)) < 0) > - goto err0; > - > - /* If we took control of the bus, we need to force > - reinitialization. This is because many ts_bus_ctrl() > - functions strobe the RESET pin on the demod, and if the > - frontend thread already exists then the dvb_init() routine > - won't get called (which is what usually does initial > - register configuration). */ > - fepriv->reinitialise = 1; > - } This branch was missing in the patch. I will send a new version. Hopefuly someone with access to the hardware can test it? > - > if ((ret = dvb_generic_open(inode, file)) < 0) > goto err1; > > @@ -2871,8 +2804,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: > @@ -2891,9 +2822,75 @@ static int dvb_frontend_open(struct inode *inode, struct file *file) > err1: > 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); > + > + 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 { > + mutex_lock(&adapter->mfe_lock); > + > + if (!adapter->mfe_dvbdev) { > + adapter->mfe_dvbdev = dvbdev; > + } 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; > + } > + } > + > + 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; > + } > + } > + } > + > + ret = __dvb_frontend_open(inode, file); > + mutex_unlock(&adapter->mfe_lock); > + > return ret; > } > > > -- > 2.46.0.76.ge559c4bf1a-goog > -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] media: drivers/media/dvb-core: Refactor dvb_frontend_open locking 2024-08-14 14:10 [PATCH v2 0/2] media: Follow-up for coccinelle lock fixes patchset Ricardo Ribalda 2024-08-14 14:10 ` [PATCH v2 1/2] media: drivers/media/dvb-core: Split dvb_frontend_open() Ricardo Ribalda @ 2024-08-14 14:10 ` Ricardo Ribalda 2024-08-16 8:16 ` Mauro Carvalho Chehab 1 sibling, 1 reply; 7+ messages in thread From: Ricardo Ribalda @ 2024-08-14 14:10 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: linux-media, linux-kernel, Hans Verkuil, Ricardo Ribalda Split out the wait function, and introduce some new toys: guard and lockdep. 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 <ribalda@chromium.org> --- drivers/media/dvb-core/dvb_frontend.c | 58 ++++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 21 deletions(-) diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c index e81b9996530e..7f5d0c297464 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> @@ -2826,6 +2827,34 @@ static int __dvb_frontend_open(struct inode *inode, struct file *file) return ret; } +static int wait_dvb_frontend(struct dvb_adapter *adapter, + struct dvb_device *mfedev) +{ + struct dvb_frontend *mfe = mfedev->priv; + struct dvb_frontend_private *mfepriv = mfe->frontend_priv; + int mferetry = (dvb_mfe_wait_time << 1); + int ret = 0; + + lockdep_assert_held(&adapter->mfe_lock); + + if (mfedev->users == -1 && !mfepriv->thread) + return 0; + + mutex_unlock(&adapter->mfe_lock); + + while (mferetry-- && (mfedev->users != -1 || mfepriv->thread)) { + if (msleep_interruptible(500)) + if (signal_pending(current)) { + ret = -EINTR; + break; + } + } + + mutex_lock(&adapter->mfe_lock); + + return ret; +} + static int dvb_frontend_open(struct inode *inode, struct file *file) { struct dvb_device *dvbdev = file->private_data; @@ -2840,19 +2869,16 @@ static int dvb_frontend_open(struct inode *inode, struct file *file) if (!adapter->mfe_shared) return __dvb_frontend_open(inode, file); + guard(mutex)(&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); + !adapter->mfe_dvbdev->writers) return -EBUSY; - } adapter->mfe_dvbdev = dvbdev; } } else { - mutex_lock(&adapter->mfe_lock); - if (!adapter->mfe_dvbdev) { adapter->mfe_dvbdev = dvbdev; } else if (adapter->mfe_dvbdev != dvbdev) { @@ -2862,34 +2888,24 @@ static int dvb_frontend_open(struct inode *inode, struct file *file) *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; - } - } - mutex_lock(&adapter->mfe_lock); + ret = wait_dvb_frontend(adapter, mfedev); + if (ret) + return ret; + 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); + mfepriv->thread) return -EBUSY; - } adapter->mfe_dvbdev = dvbdev; } } } ret = __dvb_frontend_open(inode, file); - mutex_unlock(&adapter->mfe_lock); return ret; } -- 2.46.0.76.ge559c4bf1a-goog ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] media: drivers/media/dvb-core: Refactor dvb_frontend_open locking 2024-08-14 14:10 ` [PATCH v2 2/2] media: drivers/media/dvb-core: Refactor dvb_frontend_open locking Ricardo Ribalda @ 2024-08-16 8:16 ` Mauro Carvalho Chehab 2024-08-16 8:20 ` Ricardo Ribalda 0 siblings, 1 reply; 7+ messages in thread From: Mauro Carvalho Chehab @ 2024-08-16 8:16 UTC (permalink / raw) To: Ricardo Ribalda Cc: Mauro Carvalho Chehab, linux-media, linux-kernel, Hans Verkuil Em Wed, 14 Aug 2024 14:10:23 +0000 Ricardo Ribalda <ribalda@chromium.org> escreveu: > Split out the wait function, and introduce some new toys: guard and > lockdep. > > 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 Hi Ricardo, Every time someone tries to fix this lock, we end having regression reports, because of the diversity of devices, and the way they registers there. That's specially true for devices with multiple frontends and custom zigzag methods. On what devices have you tested this patch? Regards, Mauro > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/dvb-core/dvb_frontend.c | 58 ++++++++++++++++++++++------------- > 1 file changed, 37 insertions(+), 21 deletions(-) > > diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c > index e81b9996530e..7f5d0c297464 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> > @@ -2826,6 +2827,34 @@ static int __dvb_frontend_open(struct inode *inode, struct file *file) > return ret; > } > > +static int wait_dvb_frontend(struct dvb_adapter *adapter, > + struct dvb_device *mfedev) > +{ > + struct dvb_frontend *mfe = mfedev->priv; > + struct dvb_frontend_private *mfepriv = mfe->frontend_priv; > + int mferetry = (dvb_mfe_wait_time << 1); > + int ret = 0; > + > + lockdep_assert_held(&adapter->mfe_lock); > + > + if (mfedev->users == -1 && !mfepriv->thread) > + return 0; > + > + mutex_unlock(&adapter->mfe_lock); > + > + while (mferetry-- && (mfedev->users != -1 || mfepriv->thread)) { > + if (msleep_interruptible(500)) > + if (signal_pending(current)) { > + ret = -EINTR; > + break; > + } > + } > + > + mutex_lock(&adapter->mfe_lock); > + > + return ret; > +} > + > static int dvb_frontend_open(struct inode *inode, struct file *file) > { > struct dvb_device *dvbdev = file->private_data; > @@ -2840,19 +2869,16 @@ static int dvb_frontend_open(struct inode *inode, struct file *file) > if (!adapter->mfe_shared) > return __dvb_frontend_open(inode, file); > > + guard(mutex)(&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); > + !adapter->mfe_dvbdev->writers) > return -EBUSY; > - } > adapter->mfe_dvbdev = dvbdev; > } > } else { > - mutex_lock(&adapter->mfe_lock); > - > if (!adapter->mfe_dvbdev) { > adapter->mfe_dvbdev = dvbdev; > } else if (adapter->mfe_dvbdev != dvbdev) { > @@ -2862,34 +2888,24 @@ static int dvb_frontend_open(struct inode *inode, struct file *file) > *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; > - } > - } > > - mutex_lock(&adapter->mfe_lock); > + ret = wait_dvb_frontend(adapter, mfedev); > + if (ret) > + return ret; > + > 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); > + mfepriv->thread) > return -EBUSY; > - } > adapter->mfe_dvbdev = dvbdev; > } > } > } > > ret = __dvb_frontend_open(inode, file); > - mutex_unlock(&adapter->mfe_lock); > > return ret; > } > Thanks, Mauro ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] media: drivers/media/dvb-core: Refactor dvb_frontend_open locking 2024-08-16 8:16 ` Mauro Carvalho Chehab @ 2024-08-16 8:20 ` Ricardo Ribalda 2024-08-16 10:16 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 7+ messages in thread From: Ricardo Ribalda @ 2024-08-16 8:20 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Mauro Carvalho Chehab, linux-media, linux-kernel, Hans Verkuil Hi Mauro On Fri, 16 Aug 2024 at 10:17, Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > Em Wed, 14 Aug 2024 14:10:23 +0000 > Ricardo Ribalda <ribalda@chromium.org> escreveu: > > > Split out the wait function, and introduce some new toys: guard and > > lockdep. > > > > 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 > > Hi Ricardo, Hi Mauro > > Every time someone tries to fix this lock, we end having regression reports, > because of the diversity of devices, and the way they registers there. > > That's specially true for devices with multiple frontends and custom > zigzag methods. > > On what devices have you tested this patch? I do not have access to any device, it is just "compiled tested". I think that the patch is mainly a refactor, it does not really change how the lock is handled. Regards! > > Regards, > Mauro > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > drivers/media/dvb-core/dvb_frontend.c | 58 ++++++++++++++++++++++------------- > > 1 file changed, 37 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c > > index e81b9996530e..7f5d0c297464 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> > > @@ -2826,6 +2827,34 @@ static int __dvb_frontend_open(struct inode *inode, struct file *file) > > return ret; > > } > > > > +static int wait_dvb_frontend(struct dvb_adapter *adapter, > > + struct dvb_device *mfedev) > > +{ > > + struct dvb_frontend *mfe = mfedev->priv; > > + struct dvb_frontend_private *mfepriv = mfe->frontend_priv; > > + int mferetry = (dvb_mfe_wait_time << 1); > > + int ret = 0; > > + > > + lockdep_assert_held(&adapter->mfe_lock); > > + > > + if (mfedev->users == -1 && !mfepriv->thread) > > + return 0; > > + > > + mutex_unlock(&adapter->mfe_lock); > > + > > + while (mferetry-- && (mfedev->users != -1 || mfepriv->thread)) { > > + if (msleep_interruptible(500)) > > + if (signal_pending(current)) { > > + ret = -EINTR; > > + break; > > + } > > + } > > + > > + mutex_lock(&adapter->mfe_lock); > > + > > + return ret; > > +} > > + > > static int dvb_frontend_open(struct inode *inode, struct file *file) > > { > > struct dvb_device *dvbdev = file->private_data; > > @@ -2840,19 +2869,16 @@ static int dvb_frontend_open(struct inode *inode, struct file *file) > > if (!adapter->mfe_shared) > > return __dvb_frontend_open(inode, file); > > > > + guard(mutex)(&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); > > + !adapter->mfe_dvbdev->writers) > > return -EBUSY; > > - } > > adapter->mfe_dvbdev = dvbdev; > > } > > } else { > > - mutex_lock(&adapter->mfe_lock); > > - > > if (!adapter->mfe_dvbdev) { > > adapter->mfe_dvbdev = dvbdev; > > } else if (adapter->mfe_dvbdev != dvbdev) { > > @@ -2862,34 +2888,24 @@ static int dvb_frontend_open(struct inode *inode, struct file *file) > > *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; > > - } > > - } > > > > - mutex_lock(&adapter->mfe_lock); > > + ret = wait_dvb_frontend(adapter, mfedev); > > + if (ret) > > + return ret; > > + > > 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); > > + mfepriv->thread) > > return -EBUSY; > > - } > > adapter->mfe_dvbdev = dvbdev; > > } > > } > > } > > > > ret = __dvb_frontend_open(inode, file); > > - mutex_unlock(&adapter->mfe_lock); > > > > return ret; > > } > > > > > > Thanks, > Mauro -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] media: drivers/media/dvb-core: Refactor dvb_frontend_open locking 2024-08-16 8:20 ` Ricardo Ribalda @ 2024-08-16 10:16 ` Mauro Carvalho Chehab 0 siblings, 0 replies; 7+ messages in thread From: Mauro Carvalho Chehab @ 2024-08-16 10:16 UTC (permalink / raw) To: Ricardo Ribalda Cc: Mauro Carvalho Chehab, linux-media, linux-kernel, Hans Verkuil Em Fri, 16 Aug 2024 10:20:47 +0200 Ricardo Ribalda <ribalda@chromium.org> escreveu: > Hi Mauro > > On Fri, 16 Aug 2024 at 10:17, Mauro Carvalho Chehab > <mchehab+huawei@kernel.org> wrote: > > > > Em Wed, 14 Aug 2024 14:10:23 +0000 > > Ricardo Ribalda <ribalda@chromium.org> escreveu: > > > > > Split out the wait function, and introduce some new toys: guard and > > > lockdep. > > > > > > 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 > > > > Hi Ricardo, > > Hi Mauro > > > > > Every time someone tries to fix this lock, we end having regression reports, > > because of the diversity of devices, and the way they registers there. > > > > That's specially true for devices with multiple frontends and custom > > zigzag methods. > > > > On what devices have you tested this patch? > > I do not have access to any device, it is just "compiled tested". > > I think that the patch is mainly a refactor, it does not really change > how the lock is handled. While I liked your approach, in the specific case of this lock, I have to disagree: there were at least 2 or 3 previous attempts to fix issues on it, with patches made by someone and dully tested on some hardware, ended being reverted due to corner cases with some boards. So, I'm not willing to take the risk of accept patches touching dvb frontend locking schema without tests with real hardware covering common and corner cases (multi-frontend, custom zigzag, ...) and/or a formal code verification to proof that the lock works on all circumstances. Regards, Mauro ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-08-16 12:30 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-08-14 14:10 [PATCH v2 0/2] media: Follow-up for coccinelle lock fixes patchset Ricardo Ribalda 2024-08-14 14:10 ` [PATCH v2 1/2] media: drivers/media/dvb-core: Split dvb_frontend_open() Ricardo Ribalda 2024-08-16 12:30 ` Ricardo Ribalda 2024-08-14 14:10 ` [PATCH v2 2/2] media: drivers/media/dvb-core: Refactor dvb_frontend_open locking Ricardo Ribalda 2024-08-16 8:16 ` Mauro Carvalho Chehab 2024-08-16 8:20 ` Ricardo Ribalda 2024-08-16 10:16 ` Mauro Carvalho Chehab
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox