Date: Thu, 27 Feb 2014 19:59:56 +0900 (JST) From: Kohji Okuno <okuno.kohji@jp.panasonic.com> To: jmg@funkthat.com Cc: freebsd-current@freebsd.org, okuno.kohji@jp.panasonic.com Subject: Re: kqueue for KBD. Message-ID: <20140227.195956.1191893338998586416.okuno.kohji@jp.panasonic.com> In-Reply-To: <20140227061840.GB47921@funkthat.com> References: <20140227.142445.47188371497615592.okuno.kohji@jp.panasonic.com> <20140227061840.GB47921@funkthat.com>
next in thread | previous in thread | raw e-mail | index | archive | help
----Next_Part(Thu_Feb_27_19_59_56_2014_781)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Hi John-Mark,
Thank you for your comment.
I added knote_clear() and knote_destroy() in kbd_detach(). I attached
patch, again. But, maybe this patch can not resolve all cases you
pointed.
Regards,
Kohji Okuno
> Kohji Okuno wrote this message on Thu, Feb 27, 2014 at 14:24 +0900:
>> I tried to add kqueue I/F to kbd.c. I attached patch.
>> What do you think about my patch?
>
> So, knlist_destroy is missing in this patch too..
>
> It also needs some style(9) loving in that some blank lines are missing
> and there are some extra curly braces...
>
> So, knlist_clear is usually used for something like close where it
> cannot be used again... You use knlist_clear when the kbd goes away,
> but this also means that the user will never be notified that the kbd
> has gone, and could possibly end up leaking resources...
>
> I guess I should maybe write a function knlist_clearerr or something
> that detaches all the knotes from the knlist and sets the proper flag
> so that they can be reaped by userland... I believe your usb patch
> had a similar issue and some of the other drivers have this issue too..
>
> Otherwise looks good...
>
> --
> John-Mark Gurney Voice: +1 415 225 5579
>
> "All that I will do, has been done, All that I have, has not."
----Next_Part(Thu_Feb_27_19_59_56_2014_781)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline; filename="kbd_kqueue-2.patch"
diff --git a/sys/dev/kbd/kbd.c b/sys/dev/kbd/kbd.c
index 8036762..26dcaad 100644
--- a/sys/dev/kbd/kbd.c
+++ b/sys/dev/kbd/kbd.c
@@ -59,6 +59,7 @@ typedef struct genkbd_softc {
char gkb_q[KB_QSIZE]; /* input queue */
unsigned int gkb_q_start;
unsigned int gkb_q_length;
+ unsigned int gkb_index;
} genkbd_softc_t;
static SLIST_HEAD(, keyboard_driver) keyboard_drivers =
@@ -472,6 +473,7 @@ static d_read_t genkbdread;
static d_write_t genkbdwrite;
static d_ioctl_t genkbdioctl;
static d_poll_t genkbdpoll;
+static d_kqfilter_t genkbdkqfilter;
static struct cdevsw kbd_cdevsw = {
@@ -483,12 +485,14 @@ static struct cdevsw kbd_cdevsw = {
.d_write = genkbdwrite,
.d_ioctl = genkbdioctl,
.d_poll = genkbdpoll,
+ .d_kqfilter = genkbdkqfilter,
.d_name = "kbd",
};
int
kbd_attach(keyboard_t *kbd)
{
+ genkbd_softc_t *sc;
if (kbd->kb_index >= keyboards)
return (EINVAL);
@@ -498,8 +502,11 @@ kbd_attach(keyboard_t *kbd)
kbd->kb_dev = make_dev(&kbd_cdevsw, kbd->kb_index, UID_ROOT, GID_WHEEL,
0600, "%s%r", kbd->kb_name, kbd->kb_unit);
make_dev_alias(kbd->kb_dev, "kbd%r", kbd->kb_index);
- kbd->kb_dev->si_drv1 = malloc(sizeof(genkbd_softc_t), M_DEVBUF,
+ sc = malloc(sizeof(genkbd_softc_t), M_DEVBUF,
M_WAITOK | M_ZERO);
+ kbd->kb_dev->si_drv1 = sc;
+ sc->gkb_index = KBD_INDEX(kbd->kb_dev);
+ knlist_init_mtx(&sc->gkb_rsel.si_note, NULL);
printf("kbd%d at %s%d\n", kbd->kb_index, kbd->kb_name, kbd->kb_unit);
return (0);
}
@@ -507,12 +514,17 @@ kbd_attach(keyboard_t *kbd)
int
kbd_detach(keyboard_t *kbd)
{
+ genkbd_softc_t *sc;
if (kbd->kb_index >= keyboards)
return (EINVAL);
if (keyboard[kbd->kb_index] != kbd)
return (EINVAL);
+ sc = kbd->kb_dev->si_drv1;
+ knlist_clear(&sc->gkb_rsel.si_note, 0);
+ knlist_destroy(&sc->gkb_rsel.si_note);
+
free(kbd->kb_dev->si_drv1, M_DEVBUF);
destroy_dev(kbd->kb_dev);
@@ -697,6 +709,71 @@ genkbdioctl(struct cdev *dev, u_long cmd, caddr_t arg, int flag, struct thread *
return (error);
}
+static void
+genkbd_kqops_detach(struct knote *kn)
+{
+ genkbd_softc_t *sc;
+ sc = kn->kn_hook;
+ knlist_remove(&sc->gkb_rsel.si_note, kn, 0);
+}
+
+static int
+genkbd_kqops_event(struct knote *kn, long hint)
+{
+ keyboard_t *kbd;
+ genkbd_softc_t *sc;
+
+ sc = kn->kn_hook;
+ kbd = kbd_get_keyboard(sc->gkb_index);
+
+ if ((kbd == NULL) || !KBD_IS_VALID(kbd)) {
+ return 1; /* the keyboard has gone */
+ }
+ else {
+ if (sc->gkb_q_length > 0)
+ return 1;
+ else
+ return 0;
+ }
+
+}
+static struct filterops genkbd_kqops =
+{
+ .f_isfd = 1,
+ .f_attach = NULL,
+ .f_detach = genkbd_kqops_detach,
+ .f_event = genkbd_kqops_event,
+};
+static int
+genkbdkqfilter(struct cdev *dev, struct knote *kn)
+{
+ keyboard_t *kbd;
+ genkbd_softc_t *sc;
+ int error = 0;
+ int s;
+
+ s = spltty();
+ sc = dev->si_drv1;
+ kbd = kbd_get_keyboard(KBD_INDEX(dev));
+ if ((sc == NULL) || (kbd == NULL) || !KBD_IS_VALID(kbd)) {
+ error = EIO;
+ }
+ else {
+ switch (kn->kn_filter) {
+ case EVFILT_READ:
+ kn->kn_fop = &genkbd_kqops;
+ kn->kn_hook = (void *)sc;
+ knlist_add(&sc->gkb_rsel.si_note, kn, 0);
+ break;
+ default:
+ error = EOPNOTSUPP;
+ break;
+ }
+ }
+ splx(s);
+ return (error);
+}
+
static int
genkbdpoll(struct cdev *dev, int events, struct thread *td)
{
@@ -744,6 +821,7 @@ genkbd_event(keyboard_t *kbd, int event, void *arg)
wakeup(sc);
}
selwakeuppri(&sc->gkb_rsel, PZERO);
+ KNOTE_UNLOCKED(&sc->gkb_rsel.si_note, 0);
return (0);
default:
return (EINVAL);
@@ -814,6 +892,7 @@ genkbd_event(keyboard_t *kbd, int event, void *arg)
wakeup(sc);
}
selwakeuppri(&sc->gkb_rsel, PZERO);
+ KNOTE_UNLOCKED(&sc->gkb_rsel.si_note, 0);
}
return (0);
----Next_Part(Thu_Feb_27_19_59_56_2014_781)----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140227.195956.1191893338998586416.okuno.kohji>