Messages in this thread | | | Date | Tue, 12 May 2009 11:10:10 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH v2] x86: fix ktermios-termio conversion |
| |
* Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 12 May 2009, Ingo Molnar wrote: > > Hm, that looks a bit ugly and it also adds 150 bytes of bloat to the > > kernel: > > > > drivers/char/tty_ioctl.o: > > > > text data bss dec hex filename > > 4704 0 0 4704 1260 tty_ioctl.o.before > > 4841 0 0 4841 12e9 tty_ioctl.o.after > > It's easy to go back to a macro (or replace it by another inline > function if you prefer that stylistically) to make it look nicer.
no, we shouldnt go back to a macro.
> Regarding the bloat, I'm still looking for a better version. The > code I posted is basically what is in asm-generic/termios.h > traditionally, so at least I'm reasonably confident that it is > correct, while the x86 version currently fails to return -EFAULT > on incorrect pointers and misses c_line changes. > > I now tried this version, which theoretically should be fairly > compact on most architectures: > > static inline int set_low_termios_bits(unsigned int *termios, > const short __user *termio) > { > unsigned short tmp; > int ret; > > ret = __get_user(tmp, termio); > *termios = (0xffff0000 & *termios) | tmp; > > return ret; > } > > /* > * Translate a "termio" structure into a "termios". Ugh. > */ > static inline int user_termio_to_kernel_termios(struct ktermios *termios, > const struct termio __user *termio) > { > if (access_ok(VERIFY_READ, termio, sizeof (*termio))) > return -EFAULT; > > return set_low_termios_bits(&termios->c_iflag, &termio->c_iflag) | > set_low_termios_bits(&termios->c_oflag, &termio->c_oflag) | > set_low_termios_bits(&termios->c_cflag, &termio->c_cflag) | > set_low_termios_bits(&termios->c_lflag, &termio->c_lflag) | > __get_user(termios->c_line, &termio->c_line) | > (__copy_from_user(termios->c_cc, termio->c_cc, NCC) > ? -EFAULT : 0); > } > > /* > * Translate a "termios" structure into a "termio". Ugh. > */ > static inline int kernel_termios_to_user_termio(struct termio __user *termio, > struct ktermios *termios) > { > if (access_ok(VERIFY_WRITE, termio, sizeof (*termio))) > return -EFAULT; > > return __put_user(termios->c_iflag, &termio->c_iflag) | > __put_user(termios->c_oflag, &termio->c_oflag) | > __put_user(termios->c_cflag, &termio->c_cflag) | > __put_user(termios->c_lflag, &termio->c_lflag) | > __put_user(termios->c_line, &termio->c_line) | > (__copy_to_user(termio->c_cc, termios->c_cc, NCC) > ? -EFAULT : 0); > } > > Unfortunately, this is even more code on x86, where get_user/put_user > is out-of-line, while __get_user/__put_user is inline and produces > fixup code. > The best I could come up with is somewhat slower but also shorter: > > static inline int set_low_termios_bits(unsigned int *termios, > const short __user *termio) > { > unsigned short tmp; > int ret; > > ret = get_user(tmp, termio); > *termios = (0xffff0000 & *termios) | tmp; > > return ret; > } > > /* > * Translate a "termio" structure into a "termios". Ugh. > */ > static inline int user_termio_to_kernel_termios(struct ktermios *termios, > const struct termio __user *termio) > { > return (set_low_termios_bits(&termios->c_iflag, &termio->c_iflag) || > set_low_termios_bits(&termios->c_oflag, &termio->c_oflag) || > set_low_termios_bits(&termios->c_cflag, &termio->c_cflag) || > set_low_termios_bits(&termios->c_lflag, &termio->c_lflag) || > get_user(termios->c_line, &termio->c_line) || > copy_from_user(termios->c_cc, termio->c_cc, NCC)) > ? -EFAULT : 0; > } > > /* > * Translate a "termios" structure into a "termio". Ugh. > */ > static inline int kernel_termios_to_user_termio(struct termio __user *termio, > struct ktermios *termios) > { > return put_user(termios->c_iflag, &termio->c_iflag) || > put_user(termios->c_oflag, &termio->c_oflag) || > put_user(termios->c_cflag, &termio->c_cflag) || > put_user(termios->c_lflag, &termio->c_lflag) || > put_user(termios->c_line, &termio->c_line) || > (copy_to_user(termio->c_cc, termios->c_cc, NCC)) > ? -EFAULT : 0; > } > > If you think that looks better, I can send another patch. I have > not tested this code functionally though.
hm, on x86 you could use get_user_try / get_user_ex() / get_user_catch() approach to linearize the dependencies and reduce register pressure.
Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |