| 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
| ||
|
Message-Id: <20080205165853.561d5f3d.akpm@linux-foundation.org>
Date: Tue, 5 Feb 2008 16:58:53 -0800
From: Andrew Morton <akpm@...ux-foundation.org>
To: Samuel Thibault <samuel.thibault@...-lyon.org>
Cc: torvalds@...ux-foundation.org, linux-kernel@...r.kernel.org,
Dmitry Torokhov <dtor@...l.ru>, Jiri Kosina <jkosina@...e.cz>
Subject: Re: [PATCH,RESEND] Basic braille screen reader support
On Tue, 5 Feb 2008 22:00:54 +0000
Samuel Thibault <samuel.thibault@...-lyon.org> wrote:
> This adds a minimalistic braille screen reader support.
> This is meant to be used by blind people e.g. on boot failures or when /
> cannot be mounted etc and thus the userland screen readers can not work.
Could you feed it through scritps/checkpatch.pl please? That finds a lot
of trivial stuff which we'd prefer be fixed.
> +#define beep(freq) do { if (sound) kd_mksound(freq, HZ/10); } while(0)
This can (and hence should!) be impemented in a C function (I think?).
> +
> +/* mini console */
> +#define WIDTH 40
> +#define BRAILLE_KEY KEY_INSERT
> +static u16 console_buf[WIDTH];
> +static int console_cursor = 0;
Unneeded initialisation (checkpatch will tell you about this)
> +/* mini view of VC */
> +static int vc_x, vc_y, lastvc_x, lastvc_y;
> +
> +/* show console ? (or show VC) */
> +static int console_show = 1;
> +/* pending newline ? */
> +static int console_newline = 1;
> +static int lastVC = -1;
> +
> +static struct console *braille_co;
> +
> +/* Very VisioBraille-specific */
> +static void braille_write(u16 *buf)
> +{
> + static u16 lastwrite[WIDTH];
> + unsigned char data[1 + 1 + 2*WIDTH + 2 + 1], csum = 0, *c;
> + u16 out;
> + int i;
> +
> + if (!braille_co)
> + return;
> +
> + if (!memcmp(lastwrite, buf, WIDTH * sizeof(*buf)))
> + return;
> + memcpy(lastwrite, buf, WIDTH * sizeof(*buf));
`lastwrite' is a kernel-wide singleton and hence at least needs some
locking protecting its consistency.
If this is a single-opener-only device then I _guess_ this approach is OK.
If not, `lastwrite' really should be some dynamically-allocated, per-open thing,
presumably accessed by file.private_data.
> +#define SOH 1
> +#define STX 2
> +#define ETX 2
> +#define EOT 4
> +#define ENQ 5
> + data[0] = STX;
> + data[1] = '>';
> + csum ^= '>';
> + c = &data[2];
> + for (i=0; i<WIDTH; i++) {
> + out = buf[i];
> + if (out >= 0x100)
> + out = '?';
> + else if (out == 0x00)
> + out = ' ';
> + csum ^= out;
> + if (out <= 0x05) {
> + *c++ = SOH;
> + out |= 0x40;
> + }
> + *c++ = out;
> + }
> +
> + if (csum <= 0x05) {
> + *c++ = SOH;
> + csum = 0x40;
> + }
> + *c++ = csum;
> + *c++ = ETX;
> +
> + serial8250_console.write(braille_co, data, c - data);
> +}
hm. Is it appropriate that this driver wire itself directly into
serial8250? What if the screen reader is attached to some other sort of
uart, or a terminal server, or...
Maybe this all should be implemented as a line discipline, or something
like that?
> +/*
> + * Link to keyboard
> + */
> +
> +static int keyboard_notifier_call(struct notifier_block *blk, unsigned long code, void *_param)
> +{
> + struct keyboard_notifier_param *param = _param;
> + struct vc_data *vc = param->vc;
> + int ret = NOTIFY_OK;
> +
> + if (!param->down)
> + return ret;
> +
> + switch (code) {
> + case KBD_KEYCODE:
Maybe Dmitry and Jiri would have time to review this code?
> +static struct notifier_block keyboard_notifier_block = {
> + .notifier_call = keyboard_notifier_call,
> +};
> +
> +static int vt_notifier_call(struct notifier_block *blk, unsigned long code, void *_param)
> +{
> + struct vt_notifier_param *param = _param;
> + struct vc_data *vc = param->vc;
> + switch (code) {
> + case VT_ALLOCATE:
> + break;
> + case VT_DEALLOCATE:
> + break;
> + case VT_WRITE:
> + {
> + unsigned char c = param->c;
> + switch (c) {
> + case '\b':
> + case 127:
> + if (console_cursor > 0) {
> + console_cursor--;
> + console_buf[console_cursor] = ' ';
> + }
> + break;
> + case '\n':
> + case '\v':
> + case '\f':
> + case '\r':
> + console_newline = 1;
> + break;
> + case '\t':
> + c = ' ';
> + /* Fallthrough */
> + default:
> + if (c < 32)
> + /* Ignore other control sequences */
> + break;
> + if (console_newline) {
> + memset(console_buf, 0, sizeof(console_buf));
> + console_cursor = 0;
> + console_newline = 0;
> + }
> + if (console_cursor == WIDTH)
> + memmove(console_buf, &console_buf[1], (WIDTH-1) * sizeof(*console_buf));
> + else
> + console_cursor++;
> + console_buf[console_cursor-1] = c;
> + break;
> + }
> + if (console_show)
> + braille_write(console_buf);
> + else {
> + vc_maybe_cursor_moved(vc);
> + vc_refresh(vc);
> + }
> + break;
> + }
> + case VT_UPDATE:
> + /* Maybe a VT switch, flush */
> + if (console_show) {
> + if (vc->vc_num != lastVC) {
> + lastVC = vc->vc_num;
> + memset(console_buf, 0, sizeof(console_buf));
> + console_cursor = 0;
> + braille_write(console_buf);
> + }
> + } else {
> + vc_maybe_cursor_moved(vc);
> + vc_refresh(vc);
> + }
> + break;
> + }
> + return NOTIFY_OK;
> +}
> +
> +static struct notifier_block vt_notifier_block = {
> + .notifier_call = vt_notifier_call,
> +};
> +
> +/*
> + * Link to serial console
> + */
> +
> +static int __init braille_console_setup(struct console *co, char *options)
> +{
> + int ret = 0;
> + if (co->index == -1)
> + co->index = 0;
> +#ifndef MODULE
> + ret = serial8250_console.setup(co, options);
> + if (ret < 0)
> + return ret;
> +#endif
That's pretty ungainly. Again, if we had some clear spearation between the
protocol layer and the device-driver layer and some way of binding them
under userspace control (like a line discipline), all this would get better.
> + braille_co = co;
> + return ret;
> +}
> +
> +static struct console braille_console = {
> + .name = "brl",
> + .setup = braille_console_setup,
> + .flags = CON_PRINTBUFFER,
> + .index = -1,
> +};
> +
>
> ...
>
> diff -urN linux-2.6.24-orig/drivers/a11y/Kconfig linux-2.6.24-perso/drivers/a11y/Kconfig
> --- linux-2.6.24-orig/drivers/a11y/Kconfig 1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.24-perso/drivers/a11y/Kconfig 2008-02-04 01:54:36.000000000 +0000
> @@ -0,0 +1,22 @@
> +menuconfig A11Y
> + bool "Accessibility support"
That's cute, but perhaps we should be boring and call it
CONFIG_ACCESSIBILITY. That would be more accessible ;)
> + ---help---
> + Enable a submenu where accessibility items may be enabled.
> +
> + If unsure, say N.
> +
> +if A11Y
> +config A11Y_BRAILLE_CONSOLE
And that would get very lengthy.
> + tristate "Console on braille device"
> + depends on SERIAL_8250_CONSOLE
> + ---help---
> + Enables console output on a braille device connected to a 8250
> + serial port. For now only the VisioBraille device is supported.
> +
> + To actually enable it, you need to pass option
> + console=brl0
> + to the kernel. Options are the same as for serial console.
> +
> + If unsure, say N.
> +
> +endif # A11Y
--
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/
Powered by blists - more mailing lists