On Thu, 09 Dec 2021 at 16:46:24 -0700, Theo de Raadt wrote:
> Please do not recommend people use hotplugd, for any purpose.
>
> It is on my queue for deletion, because noone has stepped up and
> fixed it.
>
> hotplug is COMPLETELY FLAWED. It reports when a device is attached,
> not when it is ready. As a result everyone uses it wrong.
>
> As a result, your script is broken. There is no assurance that the
> audio device's ioctl routines are ready 1ms after subr_autoconf returns
> from the xxattach() function.
I sent diffs for this in 2018 and nobody wanted them. I wanted
multiple reader support on /dev/hotplug for Xorg and you gave this
same complaint about hotplug being flawed, so I proposed a simple
way for particular drivers to defer their hotplug announcement until
they were ready. From 2018:
I chatted with Theo about that and I proposed that we just have a
way for those specific drivers to tell hotplug to defer their
attachment announcement and then they can manually request
announcement later when they're ready.
However, I just searched the bugs@ archives and all of the mentions
of hotplugd were like 8 years ago, and they were all related to USB
disks. Then some of the bugs were closed after an apparent change
to hotplug.c was made:
https://marc.info/?l=openbsd-bugs&m=127990199813627&w=2
After that, I can't find any reports of panics related to using
hotplugd. Are we sure this is even a problem anymore?
Nobody stepped up with any drivers that actually needed it.
Here's the diff from 2018, and another to implement multiple readers
on /dev/hotplug so things like sndiod could use it. If anyone
actually cares about them this time, I can update them to a current
tree.
diff --git sys/dev/hotplug.c sys/dev/hotplug.c
index fccd5a7c9e3..feeee75e830 100644
--- sys/dev/hotplug.c
+++ sys/dev/hotplug.c
@@ -89,6 +89,12 @@ hotplug_device_detach(enum devclass class, char *name)
hotplug_put_event(&he);
}
+void
+hotplug_defer_attach(struct device *dev)
+{
+ dev->dv_flags |= DVF_HOTPLUG_DEFER;
+}
+
void
hotplug_put_event(struct hotplug_event *he)
{
diff --git sys/kern/subr_autoconf.c sys/kern/subr_autoconf.c
index 3fff153e8ad..a31ca509e35 100644
--- sys/kern/subr_autoconf.c
+++ sys/kern/subr_autoconf.c
@@ -401,7 +401,7 @@ config_attach(struct device *parent, void *match, void
*aux, cfprint_t print)
(*ca->ca_attach)(parent, dev, aux);
config_process_deferred_children(dev);
#if NHOTPLUG > 0
- if (!cold)
+ if (!cold && !(dev->dv_flags & DVF_HOTPLUG_DEFER))
hotplug_device_attach(cd->cd_class, dev->dv_xname);
#endif
diff --git sys/sys/device.h sys/sys/device.h
index 1faa0192a99..596ef1a26d5 100644
--- sys/sys/device.h
+++ sys/sys/device.h
@@ -81,7 +81,8 @@ struct device {
};
/* dv_flags */
-#define DVF_ACTIVE 0x0001 /* device is activated */
+#define DVF_ACTIVE 0x0001 /* device is activated
*/
+#define DVF_HOTPLUG_DEFER 0x0002 /* defer hotplug
announce */
TAILQ_HEAD(devicelist, device);
diff --git sys/sys/hotplug.h sys/sys/hotplug.h
index f2da3f6cfd8..7e68482a428 100644
--- sys/sys/hotplug.h
+++ sys/sys/hotplug.h
@@ -35,6 +35,7 @@ struct hotplug_event {
#ifdef _KERNEL
void hotplug_device_attach(enum devclass, char *);
void hotplug_device_detach(enum devclass, char *);
+void hotplug_defer_attach(struct device *);
#endif
#endif /* _SYS_HOTPLUG_H_ */
Index: sys/dev/hotplug.c
===================================================================
RCS file: /cvs/src/sys/dev/hotplug.c,v
retrieving revision 1.16
diff -u -p -u -p -r1.16 hotplug.c
--- sys/dev/hotplug.c 7 Jun 2016 01:31:54 -0000 1.16
+++ sys/dev/hotplug.c 2 Sep 2018 17:42:50 -0000
@@ -1,5 +1,6 @@
/* $OpenBSD: hotplug.c,v 1.16 2016/06/07 01:31:54 tedu Exp $ */
/*
+ * Copyright (c) 2018 joshua stein <j...@openbsd.org>
* Copyright (c) 2004 Alexander Yurchenko <gra...@openbsd.org>
*
* Permission to use, copy, modify, and distribute this software for any
@@ -25,15 +26,24 @@
#include <sys/fcntl.h>
#include <sys/hotplug.h>
#include <sys/ioctl.h>
+#include <sys/malloc.h>
#include <sys/poll.h>
#include <sys/vnode.h>
#define HOTPLUG_MAXEVENTS 64
-static int opened;
+struct hotplug_dev {
+ int hd_unit;
+ int hd_evqueue_head;
+ struct selinfo hd_sel;
+ LIST_ENTRY(hotplug_dev) hd_list;
+};
+
+LIST_HEAD(, hotplug_dev) hotplug_dev_list;
+struct hotplug_dev *hotplug_lookup(int);
+
static struct hotplug_event evqueue[HOTPLUG_MAXEVENTS];
-static int evqueue_head, evqueue_tail, evqueue_count;
-static struct selinfo hotplug_sel;
+static int evqueue_head, evqueue_count;
void filt_hotplugrdetach(struct knote *);
int filt_hotplugread(struct knote *, long);
@@ -43,19 +53,18 @@ struct filterops hotplugread_filtops =
#define EVQUEUE_NEXT(p) (p == HOTPLUG_MAXEVENTS - 1 ? 0 : p + 1)
-
-int hotplug_put_event(struct hotplug_event *);
-int hotplug_get_event(struct hotplug_event *);
+void hotplug_put_event(struct hotplug_event *);
+int hotplug_get_event(struct hotplug_dev *hd, struct hotplug_event *, int);
void hotplugattach(int);
void
hotplugattach(int count)
{
- opened = 0;
evqueue_head = 0;
- evqueue_tail = 0;
evqueue_count = 0;
+
+ LIST_INIT(&hotplug_dev_list);
}
void
@@ -80,37 +89,42 @@ hotplug_device_detach(enum devclass clas
hotplug_put_event(&he);
}
-int
+void
hotplug_put_event(struct hotplug_event *he)
{
- if (evqueue_count == HOTPLUG_MAXEVENTS && opened) {
- printf("hotplug: event lost, queue full\n");
- return (1);
- }
+ struct hotplug_dev *hd;
evqueue[evqueue_head] = *he;
evqueue_head = EVQUEUE_NEXT(evqueue_head);
- if (evqueue_count == HOTPLUG_MAXEVENTS)
- evqueue_tail = EVQUEUE_NEXT(evqueue_tail);
- else
+ if (evqueue_count < HOTPLUG_MAXEVENTS)
evqueue_count++;
+
+ /*
+ * If any readers are still at the new evqueue_head, they are about to
+ * get lapped.
+ */
+ LIST_FOREACH(hd, &hotplug_dev_list, hd_list) {
+ if (hd->hd_evqueue_head == evqueue_head)
+ hd->hd_evqueue_head = EVQUEUE_NEXT(evqueue_head);
+ selwakeup(&hd->hd_sel);
+ }
+
wakeup(&evqueue);
- selwakeup(&hotplug_sel);
- return (0);
}
int
-hotplug_get_event(struct hotplug_event *he)
+hotplug_get_event(struct hotplug_dev *hd, struct hotplug_event *he, int peek)
{
int s;
- if (evqueue_count == 0)
+ if (evqueue_count == 0 || hd->hd_evqueue_head == evqueue_count ||
+ hd->hd_evqueue_head == evqueue_head)
return (1);
s = splbio();
- *he = evqueue[evqueue_tail];
- evqueue_tail = EVQUEUE_NEXT(evqueue_tail);
- evqueue_count--;
+ *he = evqueue[hd->hd_evqueue_head];
+ if (!peek)
+ hd->hd_evqueue_head = EVQUEUE_NEXT(hd->hd_evqueue_head);
splx(s);
return (0);
}
@@ -118,38 +132,74 @@ hotplug_get_event(struct hotplug_event *
int
hotplugopen(dev_t dev, int flag, int mode, struct proc *p)
{
- if (minor(dev) != 0)
- return (ENXIO);
- if ((flag & FWRITE))
+ struct hotplug_dev *hd;
+ int unit = minor(dev);
+
+ if (flag & FWRITE)
return (EPERM);
- if (opened)
+
+ if ((hd = hotplug_lookup(unit)) != NULL)
return (EBUSY);
- opened = 1;
+
+ hd = malloc(sizeof(*hd), M_DEVBUF, M_WAITOK|M_ZERO);
+ hd->hd_unit = unit;
+
+ /*
+ * Set head as far back as possible so each reader gets all historical
+ * events.
+ */
+ if (evqueue_count < HOTPLUG_MAXEVENTS)
+ hd->hd_evqueue_head = 0;
+ else
+ hd->hd_evqueue_head = EVQUEUE_NEXT(evqueue_head);
+
+ LIST_INSERT_HEAD(&hotplug_dev_list, hd, hd_list);
+
return (0);
}
+struct hotplug_dev *
+hotplug_lookup(int unit)
+{
+ struct hotplug_dev *hd;
+
+ LIST_FOREACH(hd, &hotplug_dev_list, hd_list)
+ if (hd->hd_unit == unit)
+ return (hd);
+ return (NULL);
+}
+
int
hotplugclose(dev_t dev, int flag, int mode, struct proc *p)
{
- struct hotplug_event he;
+ struct hotplug_dev *hd;
+
+ hd = hotplug_lookup(minor(dev));
+ if (hd == NULL)
+ return (EINVAL);
+
+ LIST_REMOVE(hd, hd_list);
+ free(hd, M_DEVBUF, sizeof(*hd));
- while (hotplug_get_event(&he) == 0)
- continue;
- opened = 0;
return (0);
}
int
hotplugread(dev_t dev, struct uio *uio, int flags)
{
+ struct hotplug_dev *hd;
struct hotplug_event he;
int error;
+ hd = hotplug_lookup(minor(dev));
+ if (hd == NULL)
+ return (ENXIO);
+
if (uio->uio_resid != sizeof(he))
return (EINVAL);
again:
- if (hotplug_get_event(&he) == 0)
+ if (hotplug_get_event(hd, &he, 0) == 0)
return (uiomove(&he, sizeof(he), uio));
if (flags & IO_NDELAY)
return (EAGAIN);
@@ -163,6 +213,12 @@ again:
int
hotplugioctl(dev_t dev, u_long cmd, caddr_t data, int flag, struct proc *p)
{
+ struct hotplug_dev *hd;
+
+ hd = hotplug_lookup(minor(dev));
+ if (hd == NULL)
+ return (ENXIO);
+
switch (cmd) {
case FIOASYNC:
/* ignore */
@@ -179,13 +235,19 @@ hotplugioctl(dev_t dev, u_long cmd, cadd
int
hotplugpoll(dev_t dev, int events, struct proc *p)
{
+ struct hotplug_dev *hd;
+ struct hotplug_event he;
int revents = 0;
+ hd = hotplug_lookup(minor(dev));
+ if (hd == NULL)
+ return (POLLERR);
+
if (events & (POLLIN | POLLRDNORM)) {
- if (evqueue_count > 0)
+ if (hotplug_get_event(hd, &he, 1) == 0)
revents |= events & (POLLIN | POLLRDNORM);
else
- selrecord(p, &hotplug_sel);
+ selrecord(p, &hd->hd_sel);
}
return (revents);
@@ -194,18 +256,25 @@ hotplugpoll(dev_t dev, int events, struc
int
hotplugkqfilter(dev_t dev, struct knote *kn)
{
+ struct hotplug_dev *hd;
struct klist *klist;
int s;
+ hd = hotplug_lookup(minor(dev));
+ if (hd == NULL)
+ return (EINVAL);
+
switch (kn->kn_filter) {
case EVFILT_READ:
- klist = &hotplug_sel.si_note;
+ klist = &hd->hd_sel.si_note;
kn->kn_fop = &hotplugread_filtops;
break;
default:
return (EINVAL);
}
+ kn->kn_hook = hd;
+
s = splbio();
SLIST_INSERT_HEAD(klist, kn, kn_selnext);
splx(s);
@@ -215,17 +284,22 @@ hotplugkqfilter(dev_t dev, struct knote
void
filt_hotplugrdetach(struct knote *kn)
{
+ struct hotplug_dev *hd = kn->kn_hook;
int s;
s = splbio();
- SLIST_REMOVE(&hotplug_sel.si_note, kn, knote, kn_selnext);
+ SLIST_REMOVE(&hd->hd_sel.si_note, kn, knote, kn_selnext);
splx(s);
}
int
filt_hotplugread(struct knote *kn, long hint)
{
- kn->kn_data = evqueue_count;
+ struct hotplug_dev *hd = kn->kn_hook;
+ struct hotplug_event he;
+
+ if (hotplug_get_event(hd, &he, 1) != 0)
+ return (0);
- return (evqueue_count > 0);
+ return (1);
}
Index: sys/sys/conf.h
===================================================================
RCS file: /cvs/src/sys/sys/conf.h,v
retrieving revision 1.145
diff -u -p -u -p -r1.145 conf.h
--- sys/sys/conf.h 31 Aug 2018 04:20:37 -0000 1.145
+++ sys/sys/conf.h 2 Sep 2018 17:42:50 -0000
@@ -423,7 +423,7 @@ extern struct cdevsw cdevsw[];
dev_init(c,n,open), dev_init(c,n,close), dev_init(c,n,read), \
(dev_type_write((*))) enodev, dev_init(c,n,ioctl), \
(dev_type_stop((*))) enodev, 0, dev_init(c,n,poll), \
- (dev_type_mmap((*))) enodev, 0, 0, dev_init(c,n,kqfilter) }
+ (dev_type_mmap((*))) enodev, 0, D_CLONE, dev_init(c,n,kqfilter) }
/* open, close, ioctl */
#define cdev_gpio_init(c,n) { \
Index: etc/MAKEDEV.common
===================================================================
RCS file: /cvs/src/etc/MAKEDEV.common,v
retrieving revision 1.101
diff -u -p -u -p -r1.101 MAKEDEV.common
--- etc/MAKEDEV.common 31 Aug 2018 02:32:29 -0000 1.101
+++ etc/MAKEDEV.common 2 Sep 2018 17:42:50 -0000
@@ -512,7 +512,7 @@ __devitem(pdc, pdc, PDC device)dnl
__devitem(gpr, gpr*, GPR400 smartcard reader)dnl
_mcdev(gpr, gpr*, gpr, {-major_gpr_c-})dnl
__devitem(hotplug, hotplug, devices hot plugging)dnl
-_mkdev(hotplug, hotplug, {-M hotplug c major_hotplug_c $U 400-})dnl
+_mkdev(hotplug, hotplug, {-M hotplug c major_hotplug_c $U 444-})dnl
__devitem(ipmi, ipmi*, IPMI BMC access)dnl
_mkdev(ipmi, ipmi*, {-M ipmi$U c major_ipmi_c $U 600-})dnl
__devitem(gpio, gpio*, General Purpose Input/Output)dnl
Index: share/man/man4/hotplug.4
===================================================================
RCS file: /cvs/src/share/man/man4/hotplug.4,v
retrieving revision 1.6
diff -u -p -u -p -r1.6 hotplug.4
--- share/man/man4/hotplug.4 14 Sep 2015 17:09:26 -0000 1.6
+++ share/man/man4/hotplug.4 2 Sep 2018 17:42:50 -0000
@@ -37,9 +37,9 @@ The events can then be obtained from the
call on the
.Pa /dev/hotplug
device file.
-Once an event has been read, it's deleted from the queue.
-The event queue has a limited size and if it's full all new events will be
-dropped.
+Once an event has been read, it's deleted from the queue for that reader.
+The event queue has a limited size and if it's full, the oldest events
+will be dropped.
Each event is described with the following structure declared in the
.In sys/hotplug.h
header file:
@@ -84,10 +84,6 @@ Only one structure can be read per call.
If there are no events in the queue, the
.Xr read 2
call will block until an event appears.
-.Sh DIAGNOSTICS
-.Bl -diag
-.It "hotplug: event lost, queue full"
-New events will be dropped until all pending events have been read.
.El
.Sh SEE ALSO
.Xr read 2 ,
