From 3a76bbe813ae8fd5957366cf44a23856854ba15d Mon Sep 17 00:00:00 2001 From: Sascha Wildner Date: Thu, 11 Oct 2012 00:06:44 +0200 Subject: [PATCH] usb4bsd: Fixes, fixes, fixes. * Fix a panic when trying to free null pointer in usb_free_device. * Fix a panic due to wrong assignment of locks. * Fix kqueue handling. * Add debug helpers. Submitted-by: Markus Pfeiffer --- sys/bus/u4b/usb_busdma.c | 10 ++-- sys/bus/u4b/usb_core.h | 4 +- sys/bus/u4b/usb_dev.c | 98 +++++++++++++++++++++++++++++++------- sys/bus/u4b/usb_device.c | 26 +++++----- sys/bus/u4b/usb_device.h | 1 - sys/bus/u4b/usb_generic.c | 4 +- sys/bus/u4b/usb_hub.c | 2 +- sys/bus/u4b/usb_process.c | 10 ++-- sys/bus/u4b/usb_request.c | 2 +- sys/bus/u4b/usb_transfer.c | 27 ++++------- sys/bus/u4b/usbdi.h | 1 - 11 files changed, 119 insertions(+), 66 deletions(-) diff --git a/sys/bus/u4b/usb_busdma.c b/sys/bus/u4b/usb_busdma.c index 2fc9f21f4a..c3c7136be8 100644 --- a/sys/bus/u4b/usb_busdma.c +++ b/sys/bus/u4b/usb_busdma.c @@ -372,7 +372,7 @@ usb_dma_tag_create(struct usb_dma_tag *udt, (2 + (size / USB_PAGE_SIZE)) : 1, /* maxsegsz */ (align == 1 && size > USB_PAGE_SIZE) ? USB_PAGE_SIZE : size, - /* flags */ BUS_DMA_ALIGNED | BUS_DMA_KEEP_PG_OFFSET, /* XXX: Find out what this is supposed to do! */ + /* flags */ BUS_DMA_KEEP_PG_OFFSET, /* XXX: Find out what this is supposed to do! */ &tag)) { tag = NULL; } @@ -458,7 +458,7 @@ usb_pc_common_mem_cb(void *arg, bus_dma_segment_t *segs, } done: - owned = (lockstatus(uptag->lock, curthread) == LK_EXCLUSIVE); + owned = lockowned(uptag->lock); if (!owned) lockmgr(uptag->lock, LK_EXCLUSIVE); @@ -611,7 +611,7 @@ usb_pc_load_mem(struct usb_page_cache *pc, usb_size_t size, uint8_t sync) pc->page_offset_end = size; pc->ismultiseg = 1; - KKASSERT(lockstatus(pc->tag_parent->lock, curthread) != 0); + KKASSERT(lockowned(pc->tag_parent->lock)); if (size > 0) { if (sync) { @@ -869,7 +869,7 @@ usb_bdma_work_loop(struct usb_xfer_queue *pq) xfer = pq->curr; info = xfer->xroot; - KKASSERT(lockstatus(info->xfer_lock, curthread) != 0); + KKASSERT(lockowned(info->xfer_lock)); if (xfer->error) { /* some error happened */ @@ -993,7 +993,7 @@ usb_bdma_done_event(struct usb_dma_parent_tag *udpt) info = USB_DMATAG_TO_XROOT(udpt); - KKASSERT(lockstatus(info->xfer_lock, curthread) != 0); + KKASSERT(lockowned(info->xfer_lock)); /* copy error */ info->dma_error = udpt->dma_error; diff --git a/sys/bus/u4b/usb_core.h b/sys/bus/u4b/usb_core.h index a011f19aa3..3f2c69f778 100644 --- a/sys/bus/u4b/usb_core.h +++ b/sys/bus/u4b/usb_core.h @@ -47,8 +47,8 @@ #define USB_BUS_LOCK_ASSERT_NOTOWNED(_b) KKASSERT(!lockowned(&(_b)->bus_lock)) #define USB_XFER_LOCK(_x) lockmgr((_x)->xroot->xfer_lock, LK_EXCLUSIVE) #define USB_XFER_UNLOCK(_x) lockmgr((_x)->xroot->xfer_lock, LK_RELEASE) -#define USB_XFER_LOCK_ASSERT(_x) KKASSERT(lockstatus((_x)->xroot->xfer_lock, curthread) != 0) -#define USB_XFER_LOCK_ASSERT_NOTOWNED(_x) KKASSERT(lockstatus((_x)->xroot->xfer_lock, curthread) == 0) +#define USB_XFER_LOCK_ASSERT(_x) KKASSERT(lockowned((_x)->xroot->xfer_lock)) +#define USB_XFER_LOCK_ASSERT_NOTOWNED(_x) KKASSERT(!lockowned((_x)->xroot->xfer_lock)) /* helper for converting pointers to integers */ #define USB_P2U(ptr) \ diff --git a/sys/bus/u4b/usb_dev.c b/sys/bus/u4b/usb_dev.c index 84b407657a..8728ae34d8 100644 --- a/sys/bus/u4b/usb_dev.c +++ b/sys/bus/u4b/usb_dev.c @@ -33,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -86,12 +87,7 @@ SYSCTL_INT(_hw_usb_dev, OID_AUTO, debug, CTLFLAG_RW, TUNABLE_INT("hw.usb.dev.debug", &usb_fifo_debug); #endif -#if ((__FreeBSD_version >= 700001) || (__FreeBSD_version == 0) || \ - ((__FreeBSD_version >= 600034) && (__FreeBSD_version < 700000))) #define USB_UCRED struct ucred *ucred, -#else -#define USB_UCRED -#endif /* prototypes */ @@ -113,12 +109,18 @@ static usb_error_t usb_ref_device(struct usb_cdev_privdata *, struct usb_cdev_re static usb_error_t usb_usb_ref_device(struct usb_cdev_privdata *, struct usb_cdev_refdata *); static void usb_unref_device(struct usb_cdev_privdata *, struct usb_cdev_refdata *); +static void usb_filter_detach(struct knote *kn); +static int usb_filter_read(struct knote *kn, long hint); +static int usb_filter_write(struct knote *kn, long hint); + static d_open_t usb_open; static d_close_t usb_close; static d_ioctl_t usb_ioctl; static d_read_t usb_read; static d_write_t usb_write; static d_kqfilter_t usb_kqfilter; +static d_open_t usb_static_open; +static d_close_t usb_static_close; static d_ioctl_t usb_static_ioctl; static usb_fifo_open_t usb_fifo_dummy_open; @@ -142,6 +144,8 @@ static struct cdev* usb_dev = NULL; /* character device structure used for /bus/u4b */ static struct dev_ops usb_static_ops = { { "usb", 0, D_MEM }, + .d_open = usb_static_open, + .d_close = usb_static_close, .d_ioctl = usb_static_ioctl, }; @@ -150,6 +154,11 @@ static struct lock usb_sym_lock; struct lock usb_ref_lock; +#if 0 +static int usb_nevents = 0; +static struct kqinfo usb_kqevent; +#endif + /*------------------------------------------------------------------------* * usb_loc_fill * @@ -269,7 +278,7 @@ usb_ref_device(struct usb_cdev_privdata *cpd, DPRINTFN(2, "ref read\n"); crd->rxfifo->refcount++; } - lockmgr(&usb_ref_lock, LK_EXCLUSIVE); + lockmgr(&usb_ref_lock, LK_RELEASE); return (0); @@ -955,7 +964,7 @@ usb_dev_init(void *arg) } /* XXX SI_SUB_KLD? */ -SYSINIT(usb_dev_init, SI_SUB_CONFIGURE, SI_ORDER_FIRST, usb_dev_init, NULL); +SYSINIT(usb_dev_init, SI_SUB_PRE_DRIVERS, SI_ORDER_FIRST, usb_dev_init, NULL); static void usb_dev_init_post(void *arg) @@ -971,7 +980,8 @@ usb_dev_init_post(void *arg) } } -SYSINIT(usb_dev_init_post, SI_SUB_KICK_SCHEDULER, SI_ORDER_FIRST, usb_dev_init_post, NULL); +SYSINIT(usb_dev_init_post, SI_SUB_DRIVERS, SI_ORDER_FIRST, usb_dev_init_post, + NULL); static void usb_dev_uninit(void *arg) @@ -1132,11 +1142,58 @@ done: return (err); } +static struct filterops usb_filtops_read = + { FILTEROP_ISFD, NULL, usb_filter_detach, usb_filter_read }; + +static struct filterops usb_filtops_write = + { FILTEROP_ISFD, NULL, usb_filter_detach, usb_filter_write }; + static int usb_kqfilter(struct dev_kqfilter_args *ap) { - usb_close(NULL); - return 0; + cdev_t dev = ap->a_head.a_dev; + struct knote *kn = ap->a_kn; + + ap->a_result = 0; + + switch(kn->kn_filter) { + case EVFILT_READ: + kn->kn_fop = &usb_filtops_read; + kn->kn_hook = (caddr_t)dev; + break; + case EVFILT_WRITE: + kn->kn_fop = &usb_filtops_write; + kn->kn_hook = (caddr_t)dev; + break; + default: + ap->a_result = EOPNOTSUPP; + return(0); + } + + return(0); +} + +static void +usb_filter_detach(struct knote *kn) +{ +#if 0 + struct klist *klist; + + klist = &usb_kqevent.ki_note; + knote_remove(klist, kn); +#endif +} + +static int +usb_filter_read(struct knote *kn, long hint) +{ + return(0); +} + +static int +usb_filter_write(struct knote *kn, long hint) +{ + return(0); } #if 0 /* XXX implement using kqfilter */ @@ -1531,16 +1588,25 @@ done: return (err); } + +static int +usb_static_open(struct dev_open_args *ap) +{ + return(0); +} + +static int +usb_static_close(struct dev_close_args *ap) +{ + return(0); +} + int usb_static_ioctl(struct dev_ioctl_args *ap) { u_long cmd = ap->a_cmd; caddr_t data = ap->a_data; - /* - * XXX: what is this thread descriptor and where is it - * supposed to come from? - */ - struct thread *td = NULL; + struct thread *td = curthread; /* XXX: curthread the correct choice? */ int fflag = ap->a_fflag; union { struct usb_read_dir *urd; @@ -1601,7 +1667,7 @@ usb_fifo_wait(struct usb_fifo *f) { int err; - KKASSERT(lockstatus(f->priv_lock, curthread) != 0); + KKASSERT(lockowned(f->priv_lock)); if (f->flag_iserror) { /* we are gone */ diff --git a/sys/bus/u4b/usb_device.c b/sys/bus/u4b/usb_device.c index df91771b95..8713ed41cf 100644 --- a/sys/bus/u4b/usb_device.c +++ b/sys/bus/u4b/usb_device.c @@ -680,7 +680,7 @@ usb_config_parse(struct usb_device *udev, uint8_t iface_index, uint8_t cmd) goto cleanup; if (cmd == USB_CFG_INIT) { - KKASSERT(lockstatus(&udev->enum_lock, curthread) == LK_EXCLUSIVE); + KKASSERT(lockowned(&udev->enum_lock)); /* check for in-use endpoints */ @@ -1467,13 +1467,13 @@ usbd_clear_stall_proc(struct usb_proc_msg *_pm) /* Change lock */ USB_BUS_UNLOCK(udev->bus); - lockmgr(&udev->mtx_lock, LK_EXCLUSIVE); + lockmgr(&udev->device_lock, LK_EXCLUSIVE); /* Start clear stall callback */ usbd_transfer_start(udev->ctrl_xfer[1]); /* Change lock */ - lockmgr(&udev->mtx_lock, LK_RELEASE); + lockmgr(&udev->device_lock, LK_RELEASE); USB_BUS_LOCK(udev->bus); } @@ -1541,14 +1541,11 @@ usb_alloc_device(device_t parent_dev, struct usb_bus *bus, #if 0 /* initialise our SX-lock */ sx_init_flags(&udev->ctrl_sx, "USB device SX lock", SX_DUPOK); -#endif - lockinit(&udev->ctrl_lock, "USB device SX lock", 0, 0); - -#if 0 /* initialise our SX-lock */ sx_init_flags(&udev->enum_sx, "USB config SX lock", SX_DUPOK); sx_init_flags(&udev->sr_sx, "USB suspend and resume SX lock", SX_NOWITNESS); #endif + lockinit(&udev->ctrl_lock, "USB device SX lock", 0, 0); lockinit(&udev->enum_lock, "USB config SX lock", 0, 0); lockinit(&udev->sr_lock, "USB suspend and resume SX lock", 0, 0); @@ -1556,7 +1553,7 @@ usb_alloc_device(device_t parent_dev, struct usb_bus *bus, cv_init(&udev->ref_cv, "UGONE"); /* initialise our mutex */ - lockinit(&udev->mtx_lock, "USB device mutex", 0, 0); + lockinit(&udev->device_lock, "USB device mutex", 0, 0); /* initialise generic clear stall */ udev->cs_msg[0].hdr.pm_callback = &usbd_clear_stall_proc; @@ -2120,7 +2117,7 @@ usb_free_device(struct usb_device *udev, uint8_t flag) cv_destroy(&udev->ctrlreq_cv); cv_destroy(&udev->ref_cv); - lockuninit(&udev->mtx_lock); + lockuninit(&udev->device_lock); #if USB_HAVE_UGEN KASSERT(LIST_FIRST(&udev->pd_list) == NULL, ("leaked cdev entries")); #endif @@ -2130,9 +2127,12 @@ usb_free_device(struct usb_device *udev, uint8_t flag) (bus->methods->device_uninit) (udev); /* free device */ - kfree(udev->serial, M_USB); - kfree(udev->manufacturer, M_USB); - kfree(udev->product, M_USB); + if(udev->serial) + kfree(udev->serial, M_USB); + if(udev->manufacturer) + kfree(udev->manufacturer, M_USB); + if(udev->product) + kfree(udev->product, M_USB); kfree(udev, M_USB); } @@ -2715,7 +2715,7 @@ uint8_t usbd_enum_is_locked(struct usb_device *udev) { /* XXX: Make sure that we return a correct value here */ - return (lockstatus(&udev->enum_lock, curthread) == LK_EXCLUSIVE); + return (lockowned(&udev->enum_lock)); } /* diff --git a/sys/bus/u4b/usb_device.h b/sys/bus/u4b/usb_device.h index e9872bc607..8c1ea4e5d3 100644 --- a/sys/bus/u4b/usb_device.h +++ b/sys/bus/u4b/usb_device.h @@ -117,7 +117,6 @@ struct usb_device { struct lock ctrl_lock; struct lock enum_lock; struct lock sr_lock; - struct lock mtx_lock; struct lock device_lock; struct cv ctrlreq_cv; struct cv ref_cv; diff --git a/sys/bus/u4b/usb_generic.c b/sys/bus/u4b/usb_generic.c index 5b3e052622..b585e42c8c 100644 --- a/sys/bus/u4b/usb_generic.c +++ b/sys/bus/u4b/usb_generic.c @@ -232,7 +232,7 @@ ugen_open_pipe_write(struct usb_fifo *f) struct usb_endpoint *ep = usb_fifo_softc(f); struct usb_endpoint_descriptor *ed = ep->edesc; - KKASSERT(lockstatus(f->priv_lock, curthread) != 0); + KKASSERT(lockowned(f->priv_lock)); if (f->xfer[0] || f->xfer[1]) { /* transfers are already opened */ @@ -300,7 +300,7 @@ ugen_open_pipe_read(struct usb_fifo *f) struct usb_endpoint *ep = usb_fifo_softc(f); struct usb_endpoint_descriptor *ed = ep->edesc; - KKASSERT(lockstatus(f->priv_lock, curthread) != 0); + KKASSERT(lockowned(f->priv_lock)); if (f->xfer[0] || f->xfer[1]) { /* transfers are already opened */ diff --git a/sys/bus/u4b/usb_hub.c b/sys/bus/u4b/usb_hub.c index 7b08a35338..54fdcc0db4 100644 --- a/sys/bus/u4b/usb_hub.c +++ b/sys/bus/u4b/usb_hub.c @@ -1923,7 +1923,7 @@ usb_needs_explore(struct usb_bus *bus, uint8_t do_probe) DPRINTF("No root HUB\n"); return; } - if (lockstatus(&bus->bus_lock, curthread) != 0) { + if (lockowned(&bus->bus_lock) != 0) { do_unlock = 0; } else { USB_BUS_LOCK(bus); diff --git a/sys/bus/u4b/usb_process.c b/sys/bus/u4b/usb_process.c index 2ca2d16a35..5093f4236d 100644 --- a/sys/bus/u4b/usb_process.c +++ b/sys/bus/u4b/usb_process.c @@ -269,7 +269,7 @@ usb_proc_msignal(struct usb_process *up, void *_pm0, void *_pm1) if (up->up_gone) return (_pm0); - KKASSERT(lockstatus(up->up_lock, curthread) != 0); + KKASSERT(lockowned(up->up_lock)); t = 0; @@ -350,7 +350,7 @@ usb_proc_is_gone(struct usb_process *up) * structure is initialised. */ if (up->up_lock != NULL) - KKASSERT(lockstatus(up->up_lock, curthread)!=0); + KKASSERT(lockowned(up->up_lock)); return (0); } @@ -371,7 +371,7 @@ usb_proc_mwait(struct usb_process *up, void *_pm0, void *_pm1) if (up->up_gone) return; - KKASSERT(lockstatus(up->up_lock, curthread) != 0); + KKASSERT(lockowned(up->up_lock)); if (up->up_curtd == curthread) { /* Just remove the messages from the queue. */ @@ -414,7 +414,7 @@ usb_proc_drain(struct usb_process *up) if (up->up_mtx != &Giant) mtx_assert(up->up_mtx, MA_NOTOWNED); #else - KKASSERT(lockstatus(up->up_lock, curthread) == 0); + KKASSERT(!lockowned(up->up_lock)); lockmgr(up->up_lock, LK_EXCLUSIVE); #endif @@ -470,7 +470,7 @@ usb_proc_rewakeup(struct usb_process *up) if (up->up_gone) return; - KKASSERT(lockstatus(up->up_lock, curthread) != 0); + KKASSERT(lockowned(up->up_lock)); if (up->up_msleep == 0) { /* re-wakeup */ diff --git a/sys/bus/u4b/usb_request.c b/sys/bus/u4b/usb_request.c index 352053b1ca..c8c43b3ba2 100644 --- a/sys/bus/u4b/usb_request.c +++ b/sys/bus/u4b/usb_request.c @@ -457,7 +457,7 @@ usbd_do_request_flags(struct usb_device *udev, struct lock *lock, #endif if (lock != NULL) { lockmgr(lock, LK_RELEASE); - KKASSERT(lockstatus(lock, curthread) == 0); + KKASSERT(!lockowned(lock)); } /* diff --git a/sys/bus/u4b/usb_transfer.c b/sys/bus/u4b/usb_transfer.c index 6c05dfe30b..cc2be18372 100644 --- a/sys/bus/u4b/usb_transfer.c +++ b/sys/bus/u4b/usb_transfer.c @@ -841,6 +841,7 @@ usbd_transfer_setup(struct usb_device *udev, return (USB_ERR_INVAL); } if (xfer_lock == NULL) { + panic("xfer without lock!\n"); DPRINTFN(6, "using global lock\n"); } /* sanity checks */ @@ -1675,9 +1676,7 @@ usbd_pipe_enter(struct usb_xfer *xfer) { struct usb_endpoint *ep; -#if 0 - USB_XFER_LOCK_ASSERT(xfer, MA_OWNED); -#endif + USB_XFER_LOCK_ASSERT(xfer); USB_BUS_LOCK(xfer->xroot->bus); @@ -1717,9 +1716,7 @@ usbd_transfer_start(struct usb_xfer *xfer) /* transfer is gone */ return; } -#if 0 - USB_XFER_LOCK_ASSERT(xfer, MA_OWNED); -#endif + USB_XFER_LOCK_ASSERT(xfer); /* mark the USB transfer started */ @@ -2161,10 +2158,7 @@ usbd_callback_wrapper(struct usb_xfer_queue *pq) struct usb_xfer_root *info = xfer->xroot; USB_BUS_LOCK_ASSERT(info->bus); -#if 0 /* XXX: This is probably to prevent deadlocks */ - if (!lockstatus(info->xfer_mtx, curthread) && !SCHEDULER_STOPPED()) { -#endif - if (!lockstatus(info->xfer_lock, curthread)) { + if (!lockowned(info->xfer_lock)) { /* * Cases that end up here: * @@ -2241,6 +2235,7 @@ usbd_callback_wrapper(struct usb_xfer_queue *pq) if (xfer->usb_state != USB_ST_SETUP) usbpf_xfertap(xfer, USBPF_XFERTAP_DONE); #endif + USB_XFER_LOCK_ASSERT(xfer); /* call processing routine */ (xfer->callback) (xfer, xfer->error); @@ -2383,7 +2378,7 @@ usbd_transfer_done(struct usb_xfer *xfer, usb_error_t error) usbd_transfer_dequeue(xfer); #if USB_HAVE_BUSDMA - if (lockstatus(xfer->xroot->xfer_lock, curthread)) { + if (lockowned(xfer->xroot->xfer_lock)) { struct usb_xfer_queue *pq; /* @@ -3133,20 +3128,14 @@ usbd_transfer_poll(struct usb_xfer **ppxfer, uint16_t max) /* make sure that the BUS mutex is not locked */ drop_bus = 0; -#if 0 /* XXX */ - while (lockstatus(&xroot->udev->bus->bus_lock, curthread) && !SCHEDULER_STOPPED()) { -#endif - while (lockstatus(&xroot->udev->bus->bus_lock, curthread)) { + while (lockowned(&xroot->udev->bus->bus_lock)) { lockmgr(&xroot->udev->bus->bus_lock, LK_RELEASE); drop_bus++; } /* make sure that the transfer mutex is not locked */ drop_xfer = 0; -#if 0 /* XXX */ - while (lockstatus(xroot->xfer_lock, curthread) && !SCHEDULER_STOPPED()) { -#endif - while (lockstatus(xroot->xfer_lock, curthread)) { + while (lockowned(xroot->xfer_lock)) { lockmgr(xroot->xfer_lock, LK_RELEASE); drop_xfer++; } diff --git a/sys/bus/u4b/usbdi.h b/sys/bus/u4b/usbdi.h index e5b39260c3..451eaba942 100644 --- a/sys/bus/u4b/usbdi.h +++ b/sys/bus/u4b/usbdi.h @@ -393,7 +393,6 @@ struct usb_callout { void *uco_arg; int uco_flags; }; -/* XXX what is supposed to happen with that mutex? */ void usb_callout_timeout_wrapper(void *arg); void usb_callout_init_mtx_dfly(struct usb_callout *uco, struct lock *lock, -- 2.41.0