On 10/09/2010 07:09 AM, Jiri Slaby wrote:
> On 10/08/2010 11:44 PM, Jason Cooper wrote:
>> Replaced kernel_thread() with kthread_run(). Used kthread_should_stop()
>> in place of watchdog_exited completion. Replaced watchdog_pid with
>> struct task_struct.
>>
>> watchdog_tsk is NULL when not in use, -ENOMEM for error, otherwise, in use.
>>
>> Signed-off-by: Jason Cooper <jason@xxxxxxxxxxxxxx>
>> ---
>> drivers/staging/brcm80211/brcmfmac/dhd_linux.c | 29 ++++++++++++++----------
>> 1 files changed, 17 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/staging/brcm80211/brcmfmac/dhd_linux.c b/drivers/staging/brcm80211/brcmfmac/dhd_linux.c
>> index 0088d8a..7e24f8a 100644
>> --- a/drivers/staging/brcm80211/brcmfmac/dhd_linux.c
>> +++ b/drivers/staging/brcm80211/brcmfmac/dhd_linux.c
>> @@ -2017,10 +2017,15 @@ dhd_pub_t *dhd_attach(osl_t *osh, struct dhd_bus *bus, uint bus_hdrlen)
>> if (dhd_dpc_prio >= 0) {
>> /* Initialize watchdog thread */
>> sema_init(&dhd->watchdog_sem, 0);
>> - init_completion(&dhd->watchdog_exited);
>> - dhd->watchdog_pid = kernel_thread(dhd_watchdog_thread, dhd, 0);
>> + dhd->watchdog_tsk = kthread_run(dhd_watchdog_thread, dhd,
>> + "dhd_watchdog");
>> + if (IS_ERR(dhd->watchdog_tsk)) {
>> + printk(KERN_WARNING
>> + "dhd_watchdog thread failed to start\n");
>> + dhd->watchdog_tsk = NULL;
>> + }
>> } else {
>> - dhd->watchdog_pid = -1;
>> + dhd->watchdog_tsk = NULL;
>> }
>
> Yep, ACK, this looks good(TM). You write in the changelog above, that
> you set -ENOMEM in the error case, but you don't. And I think it is
> better the way you have it now.
>
Sorry, I wasn't clear. I was enumerating the possible values, -ENOMEM being a possible return value from kthread_run(). I'll reword to make it clear.
>> @@ -2351,9 +2356,9 @@ void dhd_detach(dhd_pub_t *dhdp)
>> unregister_netdev(ifp->net);
>> }
>>
>> - if (dhd->watchdog_pid >= 0) {
>> - KILL_PROC(dhd->watchdog_pid, SIGTERM);
>> - wait_for_completion(&dhd->watchdog_exited);
>> + if (dhd->watchdog_tsk && !IS_ERR(dhd->watchdog_tsk)) {
>> + kthread_stop(dhd->watchdog_tsk);
>> + dhd->watchdog_tsk = NULL;
>
> Then, you don't need the IS_ERR checks at all.
Yep, I was overthinking the problem. Trying to account for a running thread dying unexpectedly (OOM, etc). IS_ERR() isn't going to account for that anyway. I'll respin only using IS_ERR() to check the return value of kthread_run(), and setting to NULL when not running.
Thanks for all the help.
Jason.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
