Messages in this thread | | | Date | Fri, 7 Aug 2009 10:04:29 +0200 | From | Frederic Weisbecker <> | Subject | Re: [PATCH 3/5] tty: handle VT specific compat ioctls in vt driver |
| |
On Fri, Aug 07, 2009 at 09:04:11AM +0200, Arnd Bergmann wrote: > On Friday 07 August 2009, Frederic Weisbecker wrote: > > > On Thu, Aug 06, 2009 at 03:09:28PM +0200, Arnd Bergmann wrote: > > > The VT specific compat_ioctl handlers are the only ones > > > in common code that require the BKL. Moving them into > > > the vt driver lets us remove the BKL from the other handlers > > > and cleans up the code. > > > > > > Why does it require the bkl? > > > > All the VT ioctls are currently called under the BKL. I did not try > to find out why that is but simply kept that state. All other > compat ioctl do not interact with device driver state at all, > so they obviously do not need the BKL.
Ah ok.
> > > + > > > +long vt_compat_ioctl(struct tty_struct *tty, struct file * file, > > > + unsigned int cmd, unsigned long arg) > > > +{ > > > + struct vc_data *vc = tty->driver_data; > > > + struct console_font_op op; /* used in multiple places here */ > > > + struct kbd_struct *kbd; > > > + unsigned int console; > > > + void __user *up = (void __user *)arg; > > > + int perm; > > > + int ret = 0; > > > + > > > + console = vc->vc_num; > > > + > > > + lock_kernel(); > > > > > > > > It would be really nice to add a comment here that explain what it > > is protecting. > > I would like to work on removing the bkl from tty, and such nude lock_kernel() > > don't help much to work in this area. > > This is more than ever a FUD lock, nothing about its role in tty in the Lockronomicon, > > and even not in the comments :-) > > This function is a straight copy from vt_ioctl, with the data structures replaced, > and calling it vt_ioctl where they are identical. > > You are right that this needs more code comments to make that obvious. > > Arnd <><
Ok. This looks like a nice series. A bkl pushdown that only goes down in one site among several others enlightens the understanding of what it is protecting (beside the nice fact it also burned three bkl callsites :-)
Thanks!
| |