| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <27d438e0-8c57-4029-ba53-8f6995de6c33@xs4all.nl>
Date: Thu, 30 May 2024 15:19:57 +0200
From: Hans Verkuil <hverkuil-cisco@...all.nl>
To: Ricardo Ribalda <ribalda@...omium.org>,
Andy Walls <awalls@...metrocast.net>,
Mauro Carvalho Chehab <mchehab@...nel.org>, Sean Young <sean@...s.org>,
Jarod Wilson <jarod@...hat.com>
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
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 <ribalda@...omium.org>
> ---
> 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;
>
Powered by blists - more mailing lists