Re: [PATCH 2/2] media: drivers/media/dvb-core: Refactor dvb_frontend_open locking

[Ricardo Ribalda]

Hi Hans

I clicked on send too fast sorry :S

CHECK: Alignment should match open parenthesis
#37: FILE: drivers/media/dvb-core/dvb_frontend.c:2831:
+static int wait_dvb_frontend(struct dvb_adapter *adapter,
+                              struct dvb_device *mfedev)

CHECK: Please don't use multiple blank lines
#71: FILE: drivers/media/dvb-core/dvb_frontend.c:2872:

+

Let me know if I should resend or you can handle it while merging. I
already fixed in my tree in case I have to send a v2

Thanks!

On Tue, 11 Jun 2024 at 13:03, Ricardo Ribalda <ribalda@xxxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxx>
> ---
>  drivers/media/dvb-core/dvb_frontend.c | 59 ++++++++++++++++++++++-------------
>  1 file changed, 38 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> index e81b9996530e..a7739f5e78cb 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,17 @@ 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 +2889,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.45.2.505.gda0bf45e8d-goog
>


-- 
Ricardo Ribalda