linux-kernel - Re: [PATCH] video4linux: Push down the BKL

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 27 May 2008 17:22:14 -0400
From:	"Devin Heitmueller" <devin.heitmueller@...il.com>
To:	"Mauro Carvalho Chehab" <mchehab@...radead.org>
Cc:	"Jonathan Corbet" <corbet@....net>, "Alan Cox" <alan@...hat.com>,
	video4linux-list@...hat.com, linux-kernel@...r.kernel.org,
	"Alan Cox" <alan@...rguk.ukuu.org.uk>
Subject: Re: [PATCH] video4linux: Push down the BKL

Hello Mauro,

On Tue, May 27, 2008 at 5:00 PM, Mauro Carvalho Chehab
<mchehab@...radead.org> wrote:
>> I'm not sure yet exactly how that's going to work, but it's something
>> that might prompt you to defer converting it from a mutex until we
>> have that worked out.
>
>
> The hybrid device mode lock is somewhat complex. The simplest solution would be
> to block an open() call, if the device is already used by a different mode.
>
> This will minimize things like firmware reload, on xc3028 devices, where you
> have different firmwares for analog and digital modes.
>
> Also, some USB devices (like HVR-900/HVR-950) switches off analog audio and tv
> demod chips when in digital mode (the reverse is also true - e.g. digital demod
> is switched off at analog mode).
>
> So, if you are in digital mode, on HVR-900, and changes to analog, you'll need
> to re-initialize tvp5150/msp3400. The current code for those devices handles
> this at open(). If we let this to happen later, we'll need to re-send the video
> and audio parameters to all I2C connected devices, when switching mode.
>
> On the other hand, some userspace apps, like mythtv, opens both analog and
> digital API's. I'm not sure if it does this at the same time, but, if so, a
> lock at open() will cause a regression there (someone told me that this is the
> case - I didn't test it here yet).
>
> One possible solution of providing a proper code to change mode, and not
> blocking open() would be to write something like this:
>
> static int check_and_switch_mode(void *priv, int digital)
> {
>        struct dev_foo *dev = priv;
>
>        mutex_lock(dev->lock);
>
>        if (digital)
>                return change_to_digital(dev);
>        else
>                return change_to_analog(dev);
>
>        mutex_unlock(dev->lock);
> }
>
> Since this should be called for every valid V4L2 and DVB ioctl, the better
> place for it would be to add this as a new function callback, at video_ioctl2.
> Something like [1]:
>
> --- a/linux/drivers/media/video/videodev.c      Tue May 27 16:02:56 2008 -0300
> +++ b/linux/drivers/media/video/videodev.c      Tue May 27 17:34:04 2008 -0300
> @@ -821,6 +821,10 @@
>                v4l_print_ioctl(vfd->name, cmd);
>                printk("\n");
>        }
> +
> +       if (_IOC_TYPE(cmd)=='v') || _IOC_TYPE(cmd)=='V') &&
> +               vfd->vidioc_switch_mode)
> +               ret=vfd->vidioc_switch_mode(fh, 0);
>
>  #ifdef CONFIG_VIDEO_V4L1_COMPAT
>        /***********************************************************
>
>
>
> And something like this, at dvb core [2]:
>
> diff -r b94d587ee596 linux/drivers/media/dvb/dvb-core/dvb_frontend.c
> --- a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c   Tue May 27 16:02:56 2008 -0300
> +++ b/linux/drivers/media/dvb/dvb-core/dvb_frontend.c   Tue May 27 17:35:57 2008 -0300
> @@ -784,6 +784,9 @@
>             cmd == FE_DISEQC_RECV_SLAVE_REPLY))
>                return -EPERM;
>
> +       if (fe->ops.switch_mode)
> +               err = fe->ops.switch_mode(fe, 1);
> +
>        if (down_interruptible (&fepriv->sem))
>                return -ERESTARTSYS;
>
> [1] The code can be more conservative, changing mode only if S_STD or a video
> stream ioctl is called.
>
> [2] We need to think more about the proper places for the DVB changing mode. I
> suspect that we'll need to add the mode change callback there and/or at other
> different places.
>
> PS.: I suspect that the real code will be much more complex than the above skeletons.
>
> Cheers,
> Mauro

These are all good points (and I had come to some of the same
conclusions).  I was just trying to say that the fact that we haven't
worked through these details is a good reason to not attempt to
optimize the locking routines in the em28xx driver at this point.  But
it seems like Arjan's comments make that a moot argument anyway.

Thanks,

-- 
Devin J. Heitmueller
http://www.devinheitmueller.com
AIM: devinheitmueller
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/