Re: login_fbtab glob
On Fri, 15 Apr 2022 13:28:33 -0500, joshua stein wrote: > Anyone want to bikeshed this language? I think it is more helpful to refer to glob(7) than glob(3). Perhaps something like this. - todd Index: share/man/man5/fbtab.5 === RCS file: /cvs/src/share/man/man5/fbtab.5,v retrieving revision 1.14 diff -u -p -u -r1.14 fbtab.5 --- share/man/man5/fbtab.5 8 Sep 2014 01:27:55 - 1.14 +++ share/man/man5/fbtab.5 16 Apr 2022 00:52:58 - @@ -51,15 +51,11 @@ An octal permission number (0600), as us .It Other devices The final field is a colon .Pq Ql \&: -delimited list of devices (e.g., -.Dq /dev/console:/dev/fd0a ) . -All device names are absolute paths. -A path that ends in -.Dq /\&* -refers to all directory entries except -.Dq \&. -and -.Dq \&.\&. . +delimited list of device paths (e.g., +.Dq /dev/console:/dev/fd0a:/dev/wskbd* ) . +Device paths may include shell-style globbing patterns (see +.Xr glob 7 ) , +potentially matching multiple devices. .El .Pp The @@ -83,6 +79,7 @@ the files once again belonging to root. .Sh SEE ALSO .Xr login 1 , .Xr login_fbtab 3 , +.Xr glob 7 , .Xr init 8 .Sh AUTHORS .An Guido van Rooij
wsmouse(4): tap-and-drag inputs with multiple taps
The touchpad input driver of wsmouse(4) uses a flawed shortcut for handling double taps, which has the effect that tap-and-drag inputs with multiple taps before the final contact almost never work correctly when the number of the taps is even: https://marc.info/?l=openbsd-misc=164986426617019=2 After each second tap, the driver generates a button-up and a button-down event at once, and issues a second button-up event almost immediately afterwards in order to make that work. It doesn't wait for another touch within the proper wait interval (WSMOUSECG_TAP_CLICKTIME). With this patch, the driver serializes the events properly - with an additional timeout, if necessary. A new "tap state" keeps the input handler and the timeout function in sync. Tests and OKs would be welcome. Index: dev/wscons/wstpad.c === RCS file: /cvs/src/sys/dev/wscons/wstpad.c,v retrieving revision 1.30 diff -u -p -r1.30 wstpad.c --- dev/wscons/wstpad.c 24 Mar 2021 18:28:24 - 1.30 +++ dev/wscons/wstpad.c 15 Apr 2022 17:27:35 - @@ -61,7 +61,7 @@ #define TAP_LOCKTIME_DEFAULT 0 #define TAP_BTNMAP_SIZE3 -#define CLICKDELAY_MS 20 +#define CLICKDELAY_MS 10 #define FREEZE_MS 100 #define MATCHINTERVAL_MS 45 #define STOPINTERVAL_MS55 @@ -83,6 +83,7 @@ enum tap_state { TAP_IGNORE, TAP_LIFTED, TAP_2ND_TOUCH, + TAP_REPEATED, TAP_LOCKED, TAP_NTH_TOUCH, }; @@ -93,7 +94,6 @@ enum tpad_cmd { SOFTBUTTON_UP, TAPBUTTON_DOWN, TAPBUTTON_UP, - TAPBUTTON_DOUBLECLK, VSCROLL, HSCROLL, }; @@ -828,8 +828,8 @@ wstpad_tap(struct wsmouseinput *input, u case TAP_2ND_TOUCH: if (t) { if (wstpad_is_tap(tp, t)) { - *cmds |= 1 << TAPBUTTON_DOUBLECLK; - tp->tap.state = TAP_LIFTED; + *cmds |= 1 << TAPBUTTON_UP; + tp->tap.state = TAP_REPEATED; err = !timeout_add_msec(>tap.to, CLICKDELAY_MS); } else if (tp->contacts == nmasked) { @@ -847,6 +847,14 @@ wstpad_tap(struct wsmouseinput *input, u tp->tap.state = TAP_DETECT; } break; + case TAP_REPEATED: + if (tp->contacts > nmasked) { + timeout_del(>tap.to); + tp->tap.state = TAP_2ND_TOUCH; + if ((input->sbtn.buttons & tp->tap.button) == 0) + *cmds |= 1 << TAPBUTTON_DOWN; + } + break; case TAP_LOCKED: if (tp->contacts > nmasked) { timeout_del(>tap.to); @@ -888,19 +896,28 @@ wstpad_tap_timeout(void *p) struct wstpad *tp = input->tp; struct evq_access evq; u_int btn; - int s; + int s, ev; s = spltty(); evq.evar = *input->evar; - if (evq.evar != NULL && tp != NULL && - (tp->tap.state == TAP_LIFTED || tp->tap.state == TAP_LOCKED)) { - tp->tap.state = TAP_DETECT; - input->sbtn.buttons &= ~tp->tap.button; + if (evq.evar != NULL && tp != NULL && (tp->tap.state == TAP_LIFTED + || tp->tap.state == TAP_REPEATED || tp->tap.state == TAP_LOCKED)) { + + if (tp->tap.state != TAP_REPEATED + || (input->sbtn.buttons & tp->tap.button)) { + ev = BTN_UP_EV; + input->sbtn.buttons &= ~tp->tap.button; + tp->tap.state = TAP_DETECT; + } else { + ev = BTN_DOWN_EV; + input->sbtn.buttons |= tp->tap.button; + timeout_add_msec(>tap.to, tp->tap.clicktime); + } btn = ffs(tp->tap.button) - 1; evq.put = evq.evar->put; evq.result = EVQ_RESULT_NONE; getnanotime(); - wsmouse_evq_put(, BTN_UP_EV, btn); + wsmouse_evq_put(, ev, btn); wsmouse_evq_put(, SYNC_EV, 0); if (evq.result == EVQ_RESULT_SUCCESS) { if (input->flags & LOG_EVENTS) { @@ -928,15 +945,12 @@ wstpad_click(struct wsmouseinput *input) set_freeze_ts(tp, 0, FREEZE_MS); } -/* - * Translate the "command" bits into the sync-state of wsmouse, or into - * wscons events. - */ +/* Translate the "command" bits into the sync-state of wsmouse. */ void -wstpad_cmds(struct wsmouseinput *input, struct evq_access *evq, u_int cmds) +wstpad_cmds(struct wsmouseinput *input, u_int cmds) { struct wstpad *tp = input->tp; - u_int btn, sbtns_dn = 0, sbtns_up = 0; + u_int sbtns_dn = 0, sbtns_up = 0;
Re: login_fbtab glob
Do we need to worry about symbolic links? I guess not, since they would only be owned by root? joshua stein wrote: > On Fri, 15 Apr 2022 at 12:17:12 -0600, Todd C. Miller wrote: > > On Fri, 15 Apr 2022 13:12:03 -0500, joshua stein wrote: > > > > > Thanks, both applied. > > > > Looks good to me but needs a man page update. > > Anyone want to bikeshed this language? > > > diff --git share/man/man5/fbtab.5 share/man/man5/fbtab.5 > index 13dfb634b67..18eade1fbfa 100644 > --- share/man/man5/fbtab.5 > +++ share/man/man5/fbtab.5 > @@ -51,15 +51,11 @@ An octal permission number (0600), as used by > .It Other devices > The final field is a colon > .Pq Ql \&: > -delimited list of devices (e.g., > -.Dq /dev/console:/dev/fd0a ) . > -All device names are absolute paths. > -A path that ends in > -.Dq /\&* > -refers to all directory entries except > -.Dq \&. > -and > -.Dq \&.\&. . > +delimited list of device paths (e.g., > +.Dq /dev/console:/dev/fd0a:/dev/wskbd* ) . > +All device paths are processed through > +.Xr glob 3 > +to expand any wildcard characters and potentially match multiple devices. > .El > .Pp > The > @@ -84,5 +80,6 @@ the files once again belonging to root. > .Xr login 1 , > .Xr login_fbtab 3 , > .Xr init 8 > +.Xr glob 3 > .Sh AUTHORS > .An Guido van Rooij >
Re: login_fbtab glob
On Fri, 15 Apr 2022 at 12:17:12 -0600, Todd C. Miller wrote: > On Fri, 15 Apr 2022 13:12:03 -0500, joshua stein wrote: > > > Thanks, both applied. > > Looks good to me but needs a man page update. Anyone want to bikeshed this language? diff --git share/man/man5/fbtab.5 share/man/man5/fbtab.5 index 13dfb634b67..18eade1fbfa 100644 --- share/man/man5/fbtab.5 +++ share/man/man5/fbtab.5 @@ -51,15 +51,11 @@ An octal permission number (0600), as used by .It Other devices The final field is a colon .Pq Ql \&: -delimited list of devices (e.g., -.Dq /dev/console:/dev/fd0a ) . -All device names are absolute paths. -A path that ends in -.Dq /\&* -refers to all directory entries except -.Dq \&. -and -.Dq \&.\&. . +delimited list of device paths (e.g., +.Dq /dev/console:/dev/fd0a:/dev/wskbd* ) . +All device paths are processed through +.Xr glob 3 +to expand any wildcard characters and potentially match multiple devices. .El .Pp The @@ -84,5 +80,6 @@ the files once again belonging to root. .Xr login 1 , .Xr login_fbtab 3 , .Xr init 8 +.Xr glob 3 .Sh AUTHORS .An Guido van Rooij
Re: login_fbtab glob
On Fri, 15 Apr 2022 13:12:03 -0500, joshua stein wrote: > Thanks, both applied. Looks good to me but needs a man page update. - todd
Re: login_fbtab glob
On Fri, 15 Apr 2022 at 09:32:46 -0600, Todd C. Miller wrote: > On Thu, 14 Apr 2022 17:52:37 -0500, joshua stein wrote: > > > login_fbtab(3) supports wildcards but only for every file in a > > directory (/path/*). > > > > This makes it use glob(3) so it can also support more specific > > wildcards like /path/file* > > Yes, it is better to use glob(3) than something custom. > Comments inline. Thanks, both applied. diff --git lib/libutil/login_fbtab.c lib/libutil/login_fbtab.c index 5eacf4f65ff..b81bc8e38e4 100644 --- lib/libutil/login_fbtab.c +++ lib/libutil/login_fbtab.c @@ -61,8 +61,8 @@ #include #include -#include #include +#include #include #include #include @@ -134,49 +134,31 @@ login_fbtab(const char *tty, uid_t uid, gid_t gid) static void login_protect(const char *path, mode_t mask, uid_t uid, gid_t gid) { - charbuf[PATH_MAX]; - size_t pathlen = strlen(path); - DIR *dir; - struct dirent *ent; + glob_t g; + size_t n; + char*gpath; - if (pathlen >= sizeof(buf)) { + if (strlen(path) >= PATH_MAX) { errno = ENAMETOOLONG; syslog(LOG_ERR, "%s: %s: %m", _PATH_FBTAB, path); return; } - if (strcmp("/*", path + pathlen - 2) != 0) { - if (chmod(path, mask) && errno != ENOENT) - syslog(LOG_ERR, "%s: chmod(%s): %m", _PATH_FBTAB, path); - if (chown(path, uid, gid) && errno != ENOENT) - syslog(LOG_ERR, "%s: chown(%s): %m", _PATH_FBTAB, path); - } else { - /* -* This is a wildcard directory (/path/to/whatever/ * ). -* Make a copy of path without the trailing '*' (but leave -* the trailing '/' so we can append directory entries.) -*/ - memcpy(buf, path, pathlen - 1); - buf[pathlen - 1] = '\0'; - if ((dir = opendir(buf)) == NULL) { - syslog(LOG_ERR, "%s: opendir(%s): %m", _PATH_FBTAB, - path); - return; - } + if (glob(path, GLOB_NOSORT, NULL, ) != 0) { + if (errno != ENOENT) + syslog(LOG_ERR, "%s: glob(%s): %m", _PATH_FBTAB, path); + globfree(); + return; + } - while ((ent = readdir(dir)) != NULL) { - if (strcmp(ent->d_name, ".") != 0 && - strcmp(ent->d_name, "..") != 0) { - buf[pathlen - 1] = '\0'; - if (strlcat(buf, ent->d_name, sizeof(buf)) - >= sizeof(buf)) { - errno = ENAMETOOLONG; - syslog(LOG_ERR, "%s: %s: %m", - _PATH_FBTAB, path); - } else - login_protect(buf, mask, uid, gid); - } - } - closedir(dir); + for (n = 0; n < g.gl_matchc; n++) { + gpath = g.gl_pathv[n]; + + if (chmod(gpath, mask) && errno != ENOENT) + syslog(LOG_ERR, "%s: chmod(%s): %m", _PATH_FBTAB, gpath); + if (chown(gpath, uid, gid) && errno != ENOENT) + syslog(LOG_ERR, "%s: chown(%s): %m", _PATH_FBTAB, gpath); } + + globfree(); }
Re: login_fbtab glob
On Thu, 14 Apr 2022 17:52:37 -0500, joshua stein wrote: > login_fbtab(3) supports wildcards but only for every file in a > directory (/path/*). > > This makes it use glob(3) so it can also support more specific > wildcards like /path/file* Yes, it is better to use glob(3) than something custom. Comments inline. - todd > diff --git lib/libutil/login_fbtab.c lib/libutil/login_fbtab.c > index 5eacf4f65ff..39621a0cde4 100644 > --- lib/libutil/login_fbtab.c > +++ lib/libutil/login_fbtab.c > @@ -61,8 +61,8 @@ > #include > > #include > -#include > #include > +#include > #include > #include > #include > @@ -134,49 +134,32 @@ login_fbtab(const char *tty, uid_t uid, gid_t gid) > static void > login_protect(const char *path, mode_t mask, uid_t uid, gid_t gid) > { > - charbuf[PATH_MAX]; > - size_t pathlen = strlen(path); > - DIR *dir; > - struct dirent *ent; > + glob_t g; > + size_t n; > + char*gpath; > > - if (pathlen >= sizeof(buf)) { > + if (strlen(path) > PATH_MAX) { The test should be >= PATH_MAX since PATH_MAX includes space for the NUL. > errno = ENAMETOOLONG; > syslog(LOG_ERR, "%s: %s: %m", _PATH_FBTAB, path); > return; > } > > - if (strcmp("/*", path + pathlen - 2) != 0) { > - if (chmod(path, mask) && errno != ENOENT) > - syslog(LOG_ERR, "%s: chmod(%s): %m", _PATH_FBTAB, path) > ; > - if (chown(path, uid, gid) && errno != ENOENT) > - syslog(LOG_ERR, "%s: chown(%s): %m", _PATH_FBTAB, path) > ; > - } else { > - /* > - * This is a wildcard directory (/path/to/whatever/ * ). > - * Make a copy of path without the trailing '*' (but leave > - * the trailing '/' so we can append directory entries.) > - */ > - memcpy(buf, path, pathlen - 1); > - buf[pathlen - 1] = '\0'; > - if ((dir = opendir(buf)) == NULL) { > - syslog(LOG_ERR, "%s: opendir(%s): %m", _PATH_FBTAB, > - path); > - return; > - } > + g.gl_offs = 0; > + if (glob(path, GLOB_DOOFFS, NULL, ) != 0) { I don't think you need GLOB_DOOFFS and g.gl_offs here since you are not reserving slots in gl_pathv. I would just use GLOB_NOSORT unless you need things to be sorted. > + if (errno != ENOENT) > + syslog(LOG_ERR, "%s: glob(%s): %m", _PATH_FBTAB, path); > + globfree(); > + return; > + } > > - while ((ent = readdir(dir)) != NULL) { > - if (strcmp(ent->d_name, ".") != 0 && > - strcmp(ent->d_name, "..") != 0) { > - buf[pathlen - 1] = '\0'; > - if (strlcat(buf, ent->d_name, sizeof(buf)) > - >= sizeof(buf)) { > - errno = ENAMETOOLONG; > - syslog(LOG_ERR, "%s: %s: %m", > - _PATH_FBTAB, path); > - } else > - login_protect(buf, mask, uid, gid); > - } > - } > - closedir(dir); > + for (n = 0; n < g.gl_matchc; n++) { > + gpath = g.gl_pathv[n]; > + > + if (chmod(gpath, mask) && errno != ENOENT) > + syslog(LOG_ERR, "%s: chmod(%s): %m", _PATH_FBTAB, gpath > ); > + if (chown(gpath, uid, gid) && errno != ENOENT) > + syslog(LOG_ERR, "%s: chown(%s): %m", _PATH_FBTAB, gpath > ); > } > + > + globfree(); > }
Re: dwctwo(4) fix panic
On Thu, Apr 14, 2022 at 10:00:35PM +0200, Mark Kettenis wrote: > > Date: Thu, 14 Apr 2022 20:16:13 +0200 > > From: Marcus Glocker > > > > I did hit this panic when trying to stream audio through > > uaudio(4) / dwctwo(4): > > > > panic: _dmamap_sync: ran off map! > > Stopped at panic+0x160:cmp w21, #0x0 > > TIDPIDUID PRFLAGS PFLAGS CPU COMMAND > > *421282 69103 0 0x2 0x4000K ld > > db_enter() at panic+0x15c > > panic() at _dmamap_sync+0x10c > > _dmamap_sync() at dwc2_xfercomp_isoc_split_in+0xe0 > > dwc2_xfercomp_isoc_split_in() at dwc2_hc_xfercomp_intr+0xf4 > > dwc2_hc_xfercomp_intr() at dwc2_hc_n_intr+0x1d4 > > dwc2_hc_n_intr() at dwc2_handle_hcd_intr+0x1e4 > > dwc2_handle_hcd_intr() at dwc2_intr+0xb8 > > > > Obviously usb_syncmem() expects an integer offset as second argument, > > not a memory address. Looks like a typo to me ... > > > > OK? > > Hmm, that doesn't look right. > > > > > > > Index: sys/dev/usb/dwc2/dwc2_hcdintr.c > > === > > RCS file: /cvs/src/sys/dev/usb/dwc2/dwc2_hcdintr.c,v > > retrieving revision 1.13 > > diff -u -p -u -p -r1.13 dwc2_hcdintr.c > > --- sys/dev/usb/dwc2/dwc2_hcdintr.c 4 Sep 2021 10:19:28 - 1.13 > > +++ sys/dev/usb/dwc2/dwc2_hcdintr.c 14 Apr 2022 17:48:08 - > > @@ -975,11 +975,11 @@ STATIC int dwc2_xfercomp_isoc_split_in(s > > > > if (chan->align_buf) { > > dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n", __func__); > > - usb_syncmem(qtd->urb->usbdma, chan->qh->dw_align_buf_dma, > > + usb_syncmem(qtd->urb->usbdma, 0, > > chan->qh->dw_align_buf_size, BUS_DMASYNC_POSTREAD); > > memcpy(qtd->urb->buf + frame_desc->offset + > >qtd->isoc_split_offset, chan->qh->dw_align_buf, len); > > - usb_syncmem(qtd->urb->usbdma, chan->qh->dw_align_buf_dma, > > + usb_syncmem(qtd->urb->usbdma, 0, > > chan->qh->dw_align_buf_size, BUS_DMASYNC_PREREAD); > > } > > When chan->align_buf is set we're doing DMA into dw_align_buf, and I > think that means we need to use dw_align_buf_usbdma as the first > argument to usb_syncmem since we need to make sure that the memory > we're doing DMA into is coherent with the cache. So I think this > should be something like: > > if (chan->align_buf) { > struct usb_dma *ud = >qh->dw_align_buf_usbdma; > > dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n", __func__); > usb_syncmem(ud, 0, chan->qh->dw_align_buf_size, > BUS_DMASYNC_POSTREAD); > memcpy(qtd->urb->buf + frame_desc->offset + > qtd->isoc_split_offset, chan->qh->dw_align_buf, len); > usb_syncmem(qtd->urb->usbdma, chan->qh->dw_align_buf_dma, > usb_syncmem(ud, 0, chan->qh->dw_align_buf_size, > BUS_DMASYNC_PREREAD); > > I think that means the code in dwc2_update_uwb_state() is wrong too. > But then I never fully understood this code... I guess you're right. Also looking at the other usb_syncmem() calls when dw_align_buf is involved. As well the comment for struct dwc2_qh says: * @dw_align_buf_size: Size of dw_align_buf * @dw_align_buf_dma: DMA address for dw_align_buf I've tested the following diff. Panic is gone, sound still crackles, but still partially plays. I think there are other issues related to the ISOC transfers which need to be investigated further. Index: sys/dev/usb/dwc2/dwc2_hcdintr.c === RCS file: /cvs/src/sys/dev/usb/dwc2/dwc2_hcdintr.c,v retrieving revision 1.13 diff -u -p -u -p -r1.13 dwc2_hcdintr.c --- sys/dev/usb/dwc2/dwc2_hcdintr.c 4 Sep 2021 10:19:28 - 1.13 +++ sys/dev/usb/dwc2/dwc2_hcdintr.c 15 Apr 2022 07:25:42 - @@ -493,13 +493,15 @@ STATIC int dwc2_update_urb_state(struct /* Non DWORD-aligned buffer case handling */ if (chan->align_buf && xfer_length) { dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n", __func__); - usb_syncmem(urb->usbdma, 0, chan->qh->dw_align_buf_size, + struct usb_dma *ud = >qh->dw_align_buf_usbdma; + + usb_syncmem(ud, 0, chan->qh->dw_align_buf_size, chan->ep_is_in ? BUS_DMASYNC_POSTREAD : BUS_DMASYNC_POSTWRITE); if (chan->ep_is_in) memcpy(urb->buf + urb->actual_length, chan->qh->dw_align_buf, xfer_length); - usb_syncmem(urb->usbdma, 0, chan->qh->dw_align_buf_size, + usb_syncmem(ud, 0, chan->qh->dw_align_buf_size, chan->ep_is_in ? BUS_DMASYNC_PREREAD : BUS_DMASYNC_PREWRITE); } @@ -975,12 +977,14 @@ STATIC int