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]

Message-ID: <20080527180048.6a27dbf7@gaivota>
Date:	Tue, 27 May 2008 18:00:48 -0300
From:	Mauro Carvalho Chehab <mchehab@...radead.org>
To:	"Devin Heitmueller" <devin.heitmueller@...il.com>
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

Hi Devin,

On Tue, 27 May 2008 15:26:27 -0400
"Devin Heitmueller" <devin.heitmueller@...il.com> wrote:

> Hello Mauro,
> 
> On Tue, May 27, 2008 at 2:59 PM, Mauro Carvalho Chehab
> <mchehab@...radead.org> wrote:
> > For example, em28xx has already a lock at the operations that change values at
> > "dev" struct, including open() method. However, since the lock is not called at
> > get operations, it needs to be fixed. I would also change it from mutex to a
> > read/write semaphore, since two (or more) get operations can safely happen in
> > parallel.
> 
> Please bear in mind that we have not worked out the locking semantics
> for hybrid tuner devices, and it's entirely possible that the get()
> routines will need to switch the tuner mode, which would eliminate any
> benefits of converting to a read/write semaphore.

Arjan pointed some good reasons about why we shouldn't use r/w semaphores. So,
it seems better to keep using mutexes.

> 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
--
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/