Received-SPF: pass (google.com: domain of linux-kernel+bounces-195410-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <27d438e0-8c57-4029-ba53-8f6995de6c33@xs4all.nl>
Date: Thu, 30 May 2024 15:19:57 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 5/5] media: drivers/media/dvb-core: Refactor
 dvb_frontend_open
To: Ricardo Ribalda <ribalda@chromium.org>,
 Andy Walls <awalls@md.metrocast.net>,
 Mauro Carvalho Chehab <mchehab@kernel.org>, Sean Young <sean@mess.org>,
 Jarod Wilson <jarod@redhat.com>
Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240506-cocci-locks-v1-0-a67952fe5d19@chromium.org>
 <20240506-cocci-locks-v1-5-a67952fe5d19@chromium.org>
Content-Language: en-US, nl
From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
In-Reply-To: <20240506-cocci-locks-v1-5-a67952fe5d19@chromium.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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@chromium.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;
>