From: Mike Isely Date: Mon, 7 Apr 2008 05:22:04 +0000 (-0300) Subject: V4L/DVB (7711): pvrusb2: Fix race on module unload X-Git-Tag: v2.6.26-rc1~1084^2~18 X-Git-Url: https://err.no/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e5be15c63804e05b5a94197524023702a259e308;p=linux-2.6 V4L/DVB (7711): pvrusb2: Fix race on module unload The pvrusb2 driver - for basically forever - was not enforcing a proper module tear-down. Kernel threads are used inside the driver and all must be gone before the module can be safely removed. This changeset reimplements a chunk of pvrusb2-context.c to enforce this correctly. Unfortunately this is not a simple fix. The new implementation also cuts back on kernel thread usage; instead of there being 1 control thread per instance now it's just 1 control thread shared by all instances. (By dropping to a single thread then the module exit function can block on its shutdown and the thread itself can monitor and cleanly shut down all of the other instances first.) Signed-off-by: Mike Isely Signed-off-by: Mauro Carvalho Chehab --- diff --git a/drivers/media/video/pvrusb2/pvrusb2-context.c b/drivers/media/video/pvrusb2/pvrusb2-context.c index 22bd8302c4..e7a2ed58bd 100644 --- a/drivers/media/video/pvrusb2/pvrusb2-context.c +++ b/drivers/media/video/pvrusb2/pvrusb2-context.c @@ -23,100 +23,206 @@ #include "pvrusb2-ioread.h" #include "pvrusb2-hdw.h" #include "pvrusb2-debug.h" +#include #include #include #include #include +static struct pvr2_context *pvr2_context_exist_first; +static struct pvr2_context *pvr2_context_exist_last; +static struct pvr2_context *pvr2_context_notify_first; +static struct pvr2_context *pvr2_context_notify_last; +static DEFINE_MUTEX(pvr2_context_mutex); +static DECLARE_WAIT_QUEUE_HEAD(pvr2_context_sync_data); +static struct task_struct *pvr2_context_thread_ptr; + + +static void pvr2_context_set_notify(struct pvr2_context *mp, int fl) +{ + int signal_flag = 0; + mutex_lock(&pvr2_context_mutex); + if (fl) { + if (!mp->notify_flag) { + signal_flag = (pvr2_context_notify_first == NULL); + mp->notify_prev = pvr2_context_notify_last; + mp->notify_next = NULL; + pvr2_context_notify_last = mp; + if (mp->notify_prev) { + mp->notify_prev->notify_next = mp; + } else { + pvr2_context_notify_first = mp; + } + mp->notify_flag = !0; + } + } else { + if (mp->notify_flag) { + mp->notify_flag = 0; + if (mp->notify_next) { + mp->notify_next->notify_prev = mp->notify_prev; + } else { + pvr2_context_notify_last = mp->notify_prev; + } + if (mp->notify_prev) { + mp->notify_prev->notify_next = mp->notify_next; + } else { + pvr2_context_notify_first = mp->notify_next; + } + } + } + mutex_unlock(&pvr2_context_mutex); + if (signal_flag) wake_up(&pvr2_context_sync_data); +} + static void pvr2_context_destroy(struct pvr2_context *mp) { pvr2_trace(PVR2_TRACE_CTXT,"pvr2_context %p (destroy)",mp); if (mp->hdw) pvr2_hdw_destroy(mp->hdw); + pvr2_context_set_notify(mp, 0); + mutex_lock(&pvr2_context_mutex); + if (mp->exist_next) { + mp->exist_next->exist_prev = mp->exist_prev; + } else { + pvr2_context_exist_last = mp->exist_prev; + } + if (mp->exist_prev) { + mp->exist_prev->exist_next = mp->exist_next; + } else { + pvr2_context_exist_first = mp->exist_next; + } + if (!pvr2_context_exist_first) { + /* Trigger wakeup on control thread in case it is waiting + for an exit condition. */ + wake_up(&pvr2_context_sync_data); + } + mutex_unlock(&pvr2_context_mutex); kfree(mp); } static void pvr2_context_notify(struct pvr2_context *mp) { - mp->notify_flag = !0; - wake_up(&mp->wait_data); + pvr2_context_set_notify(mp,!0); } -static int pvr2_context_thread(void *_mp) +static void pvr2_context_check(struct pvr2_context *mp) { - struct pvr2_channel *ch1,*ch2; - struct pvr2_context *mp = _mp; - pvr2_trace(PVR2_TRACE_CTXT,"pvr2_context %p (thread start)",mp); - - /* Finish hardware initialization */ - if (pvr2_hdw_initialize(mp->hdw, - (void (*)(void *))pvr2_context_notify,mp)) { - mp->video_stream.stream = - pvr2_hdw_get_video_stream(mp->hdw); - /* Trigger interface initialization. By doing this here - initialization runs in our own safe and cozy thread - context. */ - if (mp->setup_func) mp->setup_func(mp); - } else { + struct pvr2_channel *ch1, *ch2; + pvr2_trace(PVR2_TRACE_CTXT, + "pvr2_context %p (notify)", mp); + if (!mp->initialized_flag && !mp->disconnect_flag) { + mp->initialized_flag = !0; pvr2_trace(PVR2_TRACE_CTXT, - "pvr2_context %p (thread skipping setup)",mp); - /* Even though initialization did not succeed, we're still - going to enter the wait loop anyway. We need to do this - in order to await the expected disconnect (which we will - detect in the normal course of operation). */ - } - - /* Now just issue callbacks whenever hardware state changes or if - there is a disconnect. If there is a disconnect and there are - no channels left, then there's no reason to stick around anymore - so we'll self-destruct - tearing down the rest of this driver - instance along the way. */ - pvr2_trace(PVR2_TRACE_CTXT,"pvr2_context %p (thread enter loop)",mp); - while (!mp->disconnect_flag || mp->mc_first) { - if (mp->notify_flag) { - mp->notify_flag = 0; + "pvr2_context %p (initialize)", mp); + /* Finish hardware initialization */ + if (pvr2_hdw_initialize(mp->hdw, + (void (*)(void *))pvr2_context_notify, + mp)) { + mp->video_stream.stream = + pvr2_hdw_get_video_stream(mp->hdw); + /* Trigger interface initialization. By doing this + here initialization runs in our own safe and + cozy thread context. */ + if (mp->setup_func) mp->setup_func(mp); + } else { pvr2_trace(PVR2_TRACE_CTXT, - "pvr2_context %p (thread notify)",mp); - for (ch1 = mp->mc_first; ch1; ch1 = ch2) { - ch2 = ch1->mc_next; - if (ch1->check_func) ch1->check_func(ch1); - } + "pvr2_context %p (thread skipping setup)", + mp); + /* Even though initialization did not succeed, + we're still going to continue anyway. We need + to do this in order to await the expected + disconnect (which we will detect in the normal + course of operation). */ } - wait_event_interruptible(mp->wait_data, mp->notify_flag); } - pvr2_trace(PVR2_TRACE_CTXT,"pvr2_context %p (thread end)",mp); - pvr2_context_destroy(mp); + + for (ch1 = mp->mc_first; ch1; ch1 = ch2) { + ch2 = ch1->mc_next; + if (ch1->check_func) ch1->check_func(ch1); + } + + if (mp->disconnect_flag && !mp->mc_first) { + /* Go away... */ + pvr2_context_destroy(mp); + return; + } +} + + +static int pvr2_context_shutok(void) +{ + return kthread_should_stop() && (pvr2_context_exist_first == NULL); +} + + +static int pvr2_context_thread_func(void *foo) +{ + struct pvr2_context *mp; + + pvr2_trace(PVR2_TRACE_CTXT,"pvr2_context thread start"); + + do { + while ((mp = pvr2_context_notify_first) != NULL) { + pvr2_context_set_notify(mp, 0); + pvr2_context_check(mp); + } + wait_event_interruptible( + pvr2_context_sync_data, + ((pvr2_context_notify_first != NULL) || + pvr2_context_shutok())); + } while (!pvr2_context_shutok()); + + pvr2_trace(PVR2_TRACE_CTXT,"pvr2_context thread end"); + return 0; } +int pvr2_context_global_init(void) +{ + pvr2_context_thread_ptr = kthread_run(pvr2_context_thread_func, + 0, + "pvrusb2-context"); + return (pvr2_context_thread_ptr ? 0 : -ENOMEM); +} + + +void pvr2_context_global_done(void) +{ + kthread_stop(pvr2_context_thread_ptr); +} + + struct pvr2_context *pvr2_context_create( struct usb_interface *intf, const struct usb_device_id *devid, void (*setup_func)(struct pvr2_context *)) { - struct task_struct *thread; struct pvr2_context *mp = NULL; mp = kzalloc(sizeof(*mp),GFP_KERNEL); if (!mp) goto done; pvr2_trace(PVR2_TRACE_CTXT,"pvr2_context %p (create)",mp); - init_waitqueue_head(&mp->wait_data); mp->setup_func = setup_func; mutex_init(&mp->mutex); + mutex_lock(&pvr2_context_mutex); + mp->exist_prev = pvr2_context_exist_last; + mp->exist_next = NULL; + pvr2_context_exist_last = mp; + if (mp->exist_prev) { + mp->exist_prev->exist_next = mp; + } else { + pvr2_context_exist_first = mp; + } + mutex_unlock(&pvr2_context_mutex); mp->hdw = pvr2_hdw_create(intf,devid); if (!mp->hdw) { pvr2_context_destroy(mp); mp = NULL; goto done; } - thread = kthread_run(pvr2_context_thread, mp, "pvrusb2-context"); - if (!thread) { - pvr2_context_destroy(mp); - mp = NULL; - goto done; - } + pvr2_context_set_notify(mp, !0); done: return mp; } diff --git a/drivers/media/video/pvrusb2/pvrusb2-context.h b/drivers/media/video/pvrusb2/pvrusb2-context.h index 127ec53e09..6fb6ab0228 100644 --- a/drivers/media/video/pvrusb2/pvrusb2-context.h +++ b/drivers/media/video/pvrusb2/pvrusb2-context.h @@ -40,14 +40,17 @@ struct pvr2_context_stream { struct pvr2_context { struct pvr2_channel *mc_first; struct pvr2_channel *mc_last; + struct pvr2_context *exist_next; + struct pvr2_context *exist_prev; + struct pvr2_context *notify_next; + struct pvr2_context *notify_prev; struct pvr2_hdw *hdw; struct pvr2_context_stream video_stream; struct mutex mutex; int notify_flag; + int initialized_flag; int disconnect_flag; - wait_queue_head_t wait_data; - /* Called after pvr2_context initialization is complete */ void (*setup_func)(struct pvr2_context *); @@ -74,6 +77,8 @@ int pvr2_channel_claim_stream(struct pvr2_channel *, struct pvr2_ioread *pvr2_channel_create_mpeg_stream( struct pvr2_context_stream *); +int pvr2_context_global_init(void); +void pvr2_context_global_done(void); #endif /* __PVRUSB2_CONTEXT_H */ /* diff --git a/drivers/media/video/pvrusb2/pvrusb2-main.c b/drivers/media/video/pvrusb2/pvrusb2-main.c index 54d9f168d7..332aced8a5 100644 --- a/drivers/media/video/pvrusb2/pvrusb2-main.c +++ b/drivers/media/video/pvrusb2/pvrusb2-main.c @@ -125,6 +125,12 @@ static int __init pvr_init(void) pvr2_trace(PVR2_TRACE_INIT,"pvr_init"); + ret = pvr2_context_global_init(); + if (ret != 0) { + pvr2_trace(PVR2_TRACE_INIT,"pvr_init failure code=%d",ret); + return ret; + } + #ifdef CONFIG_VIDEO_PVRUSB2_SYSFS class_ptr = pvr2_sysfs_class_create(); #endif /* CONFIG_VIDEO_PVRUSB2_SYSFS */ @@ -136,6 +142,8 @@ static int __init pvr_init(void) if (pvrusb2_debug) info("Debug mask is %d (0x%x)", pvrusb2_debug,pvrusb2_debug); + pvr2_trace(PVR2_TRACE_INIT,"pvr_init complete"); + return ret; } @@ -148,6 +156,10 @@ static void __exit pvr_exit(void) #ifdef CONFIG_VIDEO_PVRUSB2_SYSFS pvr2_sysfs_class_destroy(class_ptr); #endif /* CONFIG_VIDEO_PVRUSB2_SYSFS */ + + pvr2_context_global_done(); + + pvr2_trace(PVR2_TRACE_INIT,"pvr_exit complete"); } module_init(pvr_init);