NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
kern/45872: Issues and fixes for tpcalib (touchscreen calibration)
>Number: 45872
>Category: kern
>Synopsis: Issues and fixes for tpcalib (touchscreen calibration)
>Confidential: no
>Severity: non-critical
>Priority: medium
>Responsible: kern-bug-people
>State: open
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Wed Jan 25 14:20:00 +0000 2012
>Originator: Pierre Pronchery <khorben%defora.org@localhost>
>Release: NetBSD 5.99.60
>Organization:
>Environment:
System: NetBSD syn.defora.rom 5.99.60 NetBSD 5.99.60 (GENERIC) #43: Wed Jan 25
04:05:30 CET 2012
khorben%syn.defora.rom@localhost:/home/amd64/obj/sys/arch/amd64/compile/GENERIC
amd64
Architecture: x86_64
Machine: amd64
>Description:
While working on the uts(4) driver, I had to calibrate the device for
proper mapping of screen coordinates. The driver itself uses the tpcalib
helper from the kernel, as also used in the ztp(4) and uep(4) drivers,
or on the hpcarm, hpcmips and hpcsh architectures apparently.
First, I have tried to add (partial) calibration support to the
wsconsctl user-land utility, for which patch is found below. While
apparently functional, this patch is not very helpful as it handles only
the range in which the values returned are desired. In practice, it is
sample collection which really helps in calibration (eg matching up to
WSMOUSE_CALIBCOORDS_MAX pairs of raw x/y versus desired x/y
coordinates). I think it would be helpful to commit anyway, maybe
without the warning about calibration support though, which would
otherwise affect 99% of users.
Then, I have been banging my head on the touchscreen (figuratively) in
order to get calibration to work. Turns out, there are some nasty bugs
in tpcalib_ioctl() with WSMOUSEIO_SCALIBCOORDS, when setting calibration
values through sample collection:
1. an invalid array access (and potential kernel crash?)
2. division-by-zero in the kernel (should it crash?)
3. a integer overflow (and invalid calibration)
I have been able to fix 1, but am still not sure about 2. I do not know
how to fix 3 at the moment, as I managed to calibrate the screen
properly without triggering it (by limiting my input to 4 samples).
1. Invalid array access
-----------------------
This bug is due to an invalid cast in sys/dev/wscons/mra.c, which
assumes the samples in struct wsmouse_calibcoord are of type long
instead of int. As a result, wrong values are used for the calibration;
worse, they may be read from uninitialized or invalid memory.
struct wsmouse_calibcoord {
int rawx, rawy; /* raw coordinate */
int x, y; /* translated coordinate */
} samples[WSMOUSE_CALIBCOORDS_MAX]; /* sample coordinates */
s = sizeof(struct wsmouse_calibcoord);
d = (const struct wsmouse_calibcoords *)data;
[...]
} else if (mra_Y_AX1_BX2_C(&d->samples[0].x, s,
&d->samples[0].rawx, s,
&d->samples[0].rawy, s,
d->samplelen, SCALE,
&sc->sc_ax, &sc->sc_bx, &sc->sc_cx)
and then:
int
mra_Y_AX1_BX2_C(const int *y, int ys,
const int *x1, int x1s, const int *x2, int x2s,
int n, int scale,
int *a, int *b, int *c)
{
[...]
#define AA(p, s, i) (*((const long *)(((const char *)(p)) + (s) * (i))))
#define X1(i) AA(x1, x1s, i)
#define X2(i) AA(x2, x2s, i)
#define Y(i) AA(y, ys, i)
for (i = 0; i < n; i++) {
X1a += X1(i);
Changing "const long *" to "const int *" in the AA macro is enough to
correct the behavior.
2. Division by zero
-------------------
The tpcalib_ioctl() function does not check if any sample was provided
in the first place, and will happily call the mra_Y_AX1_BX2_C() routine
anyway. As a result, nothing happens in the loop above, n is set to 0,
and this is called:
X1a /= n; X2a /= n; Ya /= n;
My experience in kernel programming (and NetBSD in particular) is not
sufficient to know if this would/should be a problem. In any case, my
test machine (amd64) did not crash, or print a warning either, although
I am sure that I have reached this code path multiple times.
3. Integer overflow
-------------------
My calibration attempts when supplying more than 4 samples have
consistently failed, inverting either of the X and Y axis, changing
their "speed", or otherwise triggering obviously wrong calibration. I am
absolutely sure that the values from my samples are correct; I have just
repeated the first ones (which work perfectly).
I therefore suspect integer overflows while processing the samples,
especially when I read this in the mra_Y_AX1_BX2_C() routine:
X1a += X1(i);
X2a += X2(i);
Ya += Y(i);
X1X1s += X1(i) * X1(i);
X2X2s += X2(i) * X2(i);
X1X2s += X1(i) * X2(i);
Does not look good to me, with up to 16 samples and coordinates
potentially as high as 2^31.
>How-To-Repeat:
Trying to calibrate a touchscreen with the tpcalib and wsmouse
frameworks.
>Fix:
Most essential:
Index: sys/dev/wscons/mra.c
===================================================================
RCS file: /cvsroot/src/sys/dev/wscons/mra.c,v
retrieving revision 1.4
diff -p -u -r1.4 mra.c
--- sys/dev/wscons/mra.c 9 Oct 2006 10:43:01 -0000 1.4
+++ sys/dev/wscons/mra.c 25 Jan 2012 12:50:53 -0000
@@ -54,13 +54,13 @@ mra_Y_AX1_BX2_C(const int *y, int ys,
int64_t S11, S22, S12;
int64_t SYY, S1Y, S2Y;
int64_t A, B, C, M;
-#define AA(p, s, i) (*((const long *)(((const char *)(p)) + (s) * (i))))
+#define AA(p, s, i) (*((const int *)(((const char *)(p)) + (s) * (i))))
#define X1(i) AA(x1, x1s, i)
#define X2(i) AA(x2, x2s, i)
#define Y(i) AA(y, ys, i)
/*
- * get avarage and sum
+ * get average and sum
*/
X1a = 0; X2a = 0; Ya = 0;
X1X1s = 0; X2X2s = 0; X1X2s = 0;
Would be welcome:
Index: sys/dev/usb/uts.c
===================================================================
RCS file: /cvsroot/src/sys/dev/usb/uts.c,v
retrieving revision 1.1
diff -p -u -r1.1 uts.c
--- sys/dev/usb/uts.c 17 Jan 2012 03:49:20 -0000 1.1
+++ sys/dev/usb/uts.c 25 Jan 2012 12:50:52 -0000
@@ -142,6 +142,7 @@ uts_attach(device_t parent, device_t sel
struct hid_item item;
aprint_naive("\n");
+ aprint_normal("\n");
sc->sc_hdev.sc_dev = self;
sc->sc_hdev.sc_intr = uts_intr;
@@ -226,8 +227,6 @@ uts_attach(device_t parent, device_t sel
/* calibrate the touchscreen */
memset(&sc->sc_calibcoords, 0, sizeof(sc->sc_calibcoords));
- sc->sc_calibcoords.maxx = 4095;
- sc->sc_calibcoords.maxy = 4095;
sc->sc_calibcoords.samplelen = WSMOUSE_CALIBCOORDS_RESET;
d = hid_start_parse(desc, size, hid_input);
while (hid_get_item(d, &item)) {
@@ -244,6 +243,9 @@ uts_attach(device_t parent, device_t sel
sc->sc_calibcoords.maxy = item.logical_maximum;
}
}
+ if (sc->sc_calibcoords.maxx != 0
+ && sc->sc_calibcoords.maxy != 0)
+ sc->sc_calibcoords.samplelen = 0;
hid_end_parse(d);
tpcalib_init(&sc->sc_tpcalib);
To be considered:
Index: sys/dev/wscons/tpcalib.c
===================================================================
RCS file: /cvsroot/src/sys/dev/wscons/tpcalib.c,v
retrieving revision 1.11
diff -p -u -r1.11 tpcalib.c
--- sys/dev/wscons/tpcalib.c 4 Mar 2007 06:02:51 -0000 1.11
+++ sys/dev/wscons/tpcalib.c 25 Jan 2012 12:50:53 -0000
@@ -65,6 +65,8 @@ tpcalib_init(struct tpcalib_softc *sc)
void
tpcalib_reset(struct tpcalib_softc *sc)
{
+ memset(sc, 0, sizeof(struct tpcalib_softc));
+
/* This indicate 'raw mode'. No translation will be done. */
sc->sc_saved.samplelen = WSMOUSE_CALIBCOORDS_RESET;
}
@@ -99,10 +101,20 @@ tpcalib_ioctl(struct tpcalib_softc *sc,
case WSMOUSEIO_SCALIBCOORDS:
s = sizeof(struct wsmouse_calibcoord);
d = (const struct wsmouse_calibcoords *)data;
- if (d->samplelen == WSMOUSE_CALIBCOORDS_RESET) {
+ if (d->samplelen == WSMOUSE_CALIBCOORDS_RESET ||
+ d->samplelen < 0 ||
+ d->samplelen >= WSMOUSE_CALIBCOORDS_MAX) {
tpcalib_reset(sc);
- } else
- if (mra_Y_AX1_BX2_C(&d->samples[0].x, s,
+ break;
+ }
+ if (d->samplelen == 0) {
+ sc->sc_ax = SCALE;
+ sc->sc_bx = 0;
+ sc->sc_cx = 0;
+ sc->sc_ay = 0;
+ sc->sc_by = SCALE;
+ sc->sc_cy = 0;
+ } else if (mra_Y_AX1_BX2_C(&d->samples[0].x, s,
&d->samples[0].rawx, s,
&d->samples[0].rawy, s,
d->samplelen, SCALE,
@@ -112,24 +124,23 @@ tpcalib_ioctl(struct tpcalib_softc *sc,
&d->samples[0].rawy, s,
d->samplelen, SCALE,
&sc->sc_ay, &sc->sc_by, &sc->sc_cy)) {
- printf("tpcalib: MRA error");
- tpcalib_reset(sc);
+ printf("tpcalib: MRA error\n");
+ tpcalib_reset(sc);
- return (EINVAL);
- } else {
- sc->sc_minx = d->minx;
- sc->sc_maxx = d->maxx;
- sc->sc_miny = d->miny;
- sc->sc_maxy = d->maxy;
- sc->sc_saved = *d;
- DPRINTF(("tpcalib: x=%d~%d y=%d~%d\n",
- sc->sc_minx, sc->sc_maxx,
- sc->sc_miny, sc->sc_maxy));
- DPRINTF(("tpcalib: Ax=%d Bx=%d Cx=%d\n",
- sc->sc_ax, sc->sc_bx, sc->sc_cx));
- DPRINTF(("tpcalib: Ay=%d By=%d Cy=%d\n",
- sc->sc_ay, sc->sc_by, sc->sc_cy));
- }
+ return (EINVAL);
+ }
+ sc->sc_minx = d->minx;
+ sc->sc_maxx = d->maxx;
+ sc->sc_miny = d->miny;
+ sc->sc_maxy = d->maxy;
+ sc->sc_saved = *d;
+ DPRINTF(("tpcalib: x=%d~%d y=%d~%d\n",
+ sc->sc_minx, sc->sc_maxx,
+ sc->sc_miny, sc->sc_maxy));
+ DPRINTF(("tpcalib: Ax=%d Bx=%d Cx=%d\n",
+ sc->sc_ax, sc->sc_bx, sc->sc_cx));
+ DPRINTF(("tpcalib: Ay=%d By=%d Cy=%d\n",
+ sc->sc_ay, sc->sc_by, sc->sc_cy));
break;
case WSMOUSEIO_GCALIBCOORDS:
As well as:
Index: sbin/wsconsctl/mouse.c
===================================================================
RCS file: /cvsroot/src/sbin/wsconsctl/mouse.c,v
retrieving revision 1.8
diff -p -u -r1.8 mouse.c
--- sbin/wsconsctl/mouse.c 28 Apr 2008 20:23:09 -0000 1.8
+++ sbin/wsconsctl/mouse.c 25 Jan 2012 12:50:34 -0000
@@ -45,8 +45,12 @@
static int mstype;
static int resolution;
static int samplerate;
+static struct wsmouse_calibcoords calibration;
static struct wsmouse_repeat repeat;
+static void mouse_get_calibration(int);
+static void mouse_put_calibration(int);
+
static void mouse_get_repeat(int);
static void mouse_put_repeat(int);
@@ -54,6 +58,14 @@ struct field mouse_field_tab[] = {
{ "resolution", &resolution, FMT_UINT, FLG_WRONLY },
{ "samplerate", &samplerate, FMT_UINT, FLG_WRONLY },
{ "type", &mstype, FMT_MSTYPE, FLG_RDONLY },
+ { "calibration.minx", &calibration.minx,
+ FMT_INT, FLG_MODIFY },
+ { "calibration.miny", &calibration.miny,
+ FMT_INT, FLG_MODIFY },
+ { "calibration.maxx", &calibration.maxx,
+ FMT_INT, FLG_MODIFY },
+ { "calibration.maxy", &calibration.maxy,
+ FMT_INT, FLG_MODIFY },
{ "repeat.buttons", &repeat.wr_buttons,
FMT_BITFIELD, FLG_MODIFY },
{ "repeat.delay.first", &repeat.wr_delay_first,
@@ -75,6 +87,12 @@ mouse_get_values(int fd)
if (ioctl(fd, WSMOUSEIO_GTYPE, &mstype) < 0)
err(EXIT_FAILURE, "WSMOUSEIO_GTYPE");
+ if (field_by_value(&calibration.minx)->flags & FLG_GET ||
+ field_by_value(&calibration.miny)->flags & FLG_GET ||
+ field_by_value(&calibration.maxx)->flags & FLG_GET ||
+ field_by_value(&calibration.maxy)->flags & FLG_GET)
+ mouse_get_calibration(fd);
+
if (field_by_value(&repeat.wr_buttons)->flags & FLG_GET ||
field_by_value(&repeat.wr_delay_first)->flags & FLG_GET ||
field_by_value(&repeat.wr_delay_decrement)->flags & FLG_GET ||
@@ -83,6 +101,31 @@ mouse_get_values(int fd)
}
static void
+mouse_get_calibration(int fd)
+{
+ struct wsmouse_calibcoords tmp;
+
+ if (ioctl(fd, WSMOUSEIO_GCALIBCOORDS, &tmp) == -1)
+ {
+ warn("WSMOUSEIO_GCALIBCOORDS");
+ field_disable_by_value(&calibration.minx);
+ field_disable_by_value(&calibration.miny);
+ field_disable_by_value(&calibration.maxx);
+ field_disable_by_value(&calibration.maxy);
+ return;
+ }
+
+ if (field_by_value(&calibration.minx)->flags & FLG_GET)
+ calibration.minx = tmp.minx;
+ if (field_by_value(&calibration.miny)->flags & FLG_GET)
+ calibration.miny = tmp.miny;
+ if (field_by_value(&calibration.maxx)->flags & FLG_GET)
+ calibration.maxx = tmp.maxx;
+ if (field_by_value(&calibration.maxy)->flags & FLG_GET)
+ calibration.maxy = tmp.maxy;
+}
+
+static void
mouse_get_repeat(int fd)
{
struct wsmouse_repeat tmp;
@@ -119,6 +162,12 @@ mouse_put_values(int fd)
pr_field(field_by_value(&samplerate), " -> ");
}
+ if (field_by_value(&calibration.minx)->flags & FLG_SET ||
+ field_by_value(&calibration.miny)->flags & FLG_SET ||
+ field_by_value(&calibration.maxx)->flags & FLG_SET ||
+ field_by_value(&calibration.maxy)->flags & FLG_SET)
+ mouse_put_calibration(fd);
+
if (field_by_value(&repeat.wr_buttons)->flags & FLG_SET ||
field_by_value(&repeat.wr_delay_first)->flags & FLG_SET ||
field_by_value(&repeat.wr_delay_decrement)->flags & FLG_SET ||
@@ -127,6 +176,40 @@ mouse_put_values(int fd)
}
static void
+mouse_put_calibration(int fd)
+{
+ struct wsmouse_calibcoords tmp;
+
+ /* Fetch current values into the temporary structure. */
+ if (ioctl(fd, WSMOUSEIO_GCALIBCOORDS, &tmp) == -1)
+ err(EXIT_FAILURE, "WSMOUSEIO_GCALIBCOORDS");
+
+ /* Overwrite the desired values in the temporary structure. */
+ if (field_by_value(&calibration.minx)->flags & FLG_SET)
+ tmp.minx = calibration.minx;
+ if (field_by_value(&calibration.miny)->flags & FLG_SET)
+ tmp.miny = calibration.miny;
+ if (field_by_value(&calibration.maxx)->flags & FLG_SET)
+ tmp.maxx = calibration.maxx;
+ if (field_by_value(&calibration.maxy)->flags & FLG_SET)
+ tmp.maxy = calibration.maxy;
+
+ /* Set new values for calibrating events. */
+ if (ioctl(fd, WSMOUSEIO_SCALIBCOORDS, &tmp) == -1)
+ err(EXIT_FAILURE, "WSMOUSEIO_SCALIBCOORDS");
+
+ /* Now print what changed. */
+ if (field_by_value(&calibration.minx)->flags & FLG_SET)
+ pr_field(field_by_value(&calibration.minx), " -> ");
+ if (field_by_value(&calibration.miny)->flags & FLG_SET)
+ pr_field(field_by_value(&calibration.miny), " -> ");
+ if (field_by_value(&calibration.maxx)->flags & FLG_SET)
+ pr_field(field_by_value(&calibration.maxx), " -> ");
+ if (field_by_value(&calibration.maxy)->flags & FLG_SET)
+ pr_field(field_by_value(&calibration.maxy), " -> ");
+}
+
+static void
mouse_put_repeat(int fd)
{
struct wsmouse_repeat tmp;
Index: sbin/wsconsctl/util.c
===================================================================
RCS file: /cvsroot/src/sbin/wsconsctl/util.c,v
retrieving revision 1.30
diff -p -u -r1.30 util.c
--- sbin/wsconsctl/util.c 15 Dec 2011 14:25:12 -0000 1.30
+++ sbin/wsconsctl/util.c 25 Jan 2012 12:50:34 -0000
@@ -263,6 +263,9 @@ pr_field(struct field *f, const char *se
case FMT_BITFIELD:
pr_bitfield(*((unsigned int *) f->valp));
break;
+ case FMT_INT:
+ (void)printf("%d", *((int *) f->valp));
+ break;
case FMT_KBDTYPE:
p = int2name(*((unsigned int *) f->valp), 1,
kbtype_tab, TABLEN(kbtype_tab));
@@ -368,6 +371,14 @@ rd_field(struct field *f, char *val, int
case FMT_BITFIELD:
*((unsigned int *) f->valp) = rd_bitfield(val);
break;
+ case FMT_INT:
+ if (sscanf(val, "%d", &i) != 1)
+ errx(EXIT_FAILURE, "%s: not a number", val);
+ if (merge)
+ *((int *) f->valp) += i;
+ else
+ *((int *) f->valp) = i;
+ break;
case FMT_KBDENC:
p = strchr(val, '.');
if (p != NULL)
Index: sbin/wsconsctl/wsconsctl.h
===================================================================
RCS file: /cvsroot/src/sbin/wsconsctl/wsconsctl.h,v
retrieving revision 1.11
diff -p -u -r1.11 wsconsctl.h
--- sbin/wsconsctl/wsconsctl.h 27 Aug 2011 19:01:34 -0000 1.11
+++ sbin/wsconsctl/wsconsctl.h 25 Jan 2012 12:50:34 -0000
@@ -55,6 +55,7 @@ struct field {
#define FMT_UINT 1 /* unsigned integer */
#define FMT_STRING 2 /* zero terminated string */
#define FMT_BITFIELD 3 /* bit field */
+#define FMT_INT 4 /* signed integer */
#define FMT_KBDTYPE 101 /* keyboard type */
#define FMT_MSTYPE 102 /* mouse type */
#define FMT_DPYTYPE 103 /* display type */
And for the record, the code I used to submit samples:
=== BEGIN PASTE ===
#include <sys/ioctl.h>
#include <fcntl.h>
#include <unistd.h>
#include <stdio.h>
#include <string.h>
#include <dev/wscons/wsconsio.h>
static int _error(char const * message, int ret)
{
fputs("calib: ", stderr);
perror(message);
return ret;
}
static int _usage(void)
{
fputs("Usage: calib\n", stderr);
return 1;
}
int main(int argc, char * argv[])
{
int o;
int fd;
struct wsmouse_calibcoords calib;
while((o = getopt(argc, argv, "")) != -1)
switch(o)
{
default:
return _usage();
}
if(optind != argc)
return _usage();
if((fd = open("/dev/wsmouse", O_WRONLY)) < 0)
fd = open("/dev/wsmouse", O_RDONLY);
if(fd < 0)
return _error("/dev/wsmouse", 2);
memset(&calib, 0, sizeof(calib));
if(ioctl(fd, WSMOUSEIO_GCALIBCOORDS, &calib) != 0)
{
close(fd);
return _error("WSMOUSEIO_GCALIBCOORDS", 2);
}
printf("x=(%d,%d) y=(%d,%d) %d samples\n", calib.minx, calib.maxx,
calib.miny, calib.maxy, calib.samplelen);
#if 0
calib.minx = 0;
calib.miny = 0;
calib.maxx = 4095;
calib.maxy = 4095;
#endif
calib.samplelen = 4;
memset(&calib.samples, 0, sizeof(calib.samples));
/* maximum values likely */
calib.samples[0].rawx = 4095;
calib.samples[0].rawy = 4095;
calib.samples[0].x = 1023;
calib.samples[0].y = 599;
/* top left */
calib.samples[1].rawx = 256;
calib.samples[1].rawy = 328;
calib.samples[1].x = 64;
calib.samples[1].y = 48;
/* top right */
calib.samples[2].rawx = 3840;
calib.samples[2].rawy = 328;
calib.samples[2].x = 960;
calib.samples[2].y = 48;
/* bottom right */
calib.samples[3].rawx = 3840;
calib.samples[3].rawy = 3768;
calib.samples[3].x = 960;
calib.samples[3].y = 552;
#if 0 /* FIXME disabled as it seems to overflow inside the kernel */
/* bottom left */
calib.samples[4].rawx = 256;
calib.samples[4].rawy = 3768;
calib.samples[4].x = 64;
calib.samples[4].y = 552;
#endif
if(ioctl(fd, WSMOUSEIO_SCALIBCOORDS, &calib) != 0)
{
close(fd);
return _error("WSMOUSEIO_SCALIBCOORDS", 2);
}
if(close(fd) != 0)
return _error("/dev/wsmouse", 2);
return 0;
}
=== END PASTE ===
Would either of the addition of uts(4) and these fixes be worth an entry
in doc/CHANGES?
Cheers!
>Unformatted:
On -current from January 25th, 00:09 CET
Home |
Main Index |
Thread Index |
Old Index