Re: [PATCH] zr36067: Debugging cleanups (updated again)

On Mon, 28 May 2007, Ronald Bultje wrote:
> On 5/28/07, Trent Piepho <[email protected]> wrote:
> >
> > You can set the frame buffer memory address to anything you want, that
> > could be bad.  You can mmap /dev/kmem and do all kinds of bad things.
> > There's a million bad things you can do.
>
>
> For a framebuffer, there's a reason it's not just w, but sysadmin()
> protected. Per setting, I'd prefer to be as correct and safe as possible.
> Let's go through all of them:

Since the sysfs entries are owned by root, making them user write only is
pretty much the same as capable(CAP_SYS_ADMIN).  You can also set the vidmem
parameter to anything you want when you load the module.

> - card, encoder, decoder, vidmem, video_nr:: pointless, b/c only used on
> module init (specifically zr36057_init(), which is therefore marked as
> __devinit). Exporting it would make people expect that it's settable, and it
> isn't.

These are exported as read-only, so it's quite clear they aren't settable.
There are hundreds of read-only module parameters that are visible via sysfs.
So again, if you think read-only module parameters shouldn't appear in sysfs,
it's not me you should be arguing with.

There are plenty of good reasons why this is can be useful.  Many times a
module parameter's value when the module is loaded on bootup isn't what is
intended.  Being able to look in sysfs is great way to check that video_nr
really is set to what it is supposed to be.  It's also great for remote
debugging or helping people over irc.  You can tell them to type "echo
/sys/module/whatever" and then you know what settings are being used.

> - default_input/norm could be exported as 0444, but anything else would not
> be correct, because maybe the variable is 0644, the _setting_ isn't, and

That's why I export them 0444.

> - pass_through: sort of 0644, although it's only after a device-close, not
> instant-apply. If we make this 0644, on the long-run it should be instant
> apply using a callback from sysfs.

Is there way to make setting a sysfs parameter trigger a callback?  I can't
find any drivers that do this.

> {jpg,v4l}_{bufsize,nbufs}: I think v4l_bufsize/nbufs are per device
> open-close, so same "objection" as above, although I'm ok with it with some
> documentation that it is not instant-apply. jpg-*, as said, introduces
> races. If possible, this would have to be sysadmin protected, w-protection
> alone is not enough since most people _need_ w access to the device to be
> able to write data to it (e.g. using lavplay).

sysfs entries are owned by root, not the owner of the device.  The race
introduction is a red herring.  All settable module parameters introduce a
race condition.  Just changing the order a variable is accessed in C code
won't fix a race, you must at least have a memory barrier.

> debug: should obviously be in sysfs and is already instant-apply.

Of course since there is no spinlock and it's not an atomic_t, there is still
a race condition every single time it is used.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/