diff mbox series

[v2,2/2] media: drivers/media/dvb-core: Refactor dvb_frontend_open locking

Message ID 20240814-coccinelle-followup-v2-2-88b4e4a9af56@chromium.org (mailing list archive)
State New
Headers show
Series media: Follow-up for coccinelle lock fixes patchset | expand

Commit Message

Ricardo Ribalda Aug. 14, 2024, 2:10 p.m. UTC
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(-)

Comments

Mauro Carvalho Chehab Aug. 16, 2024, 8:16 a.m. UTC | #1
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
Ricardo Ribalda Aug. 16, 2024, 8:20 a.m. UTC | #2
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
Mauro Carvalho Chehab Aug. 16, 2024, 10:16 a.m. UTC | #3
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
diff mbox series

Patch

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;
 }