[PATCH v2.6.36-rc7 1/2] isdn/capi: unregister capictr notifier after init failure

[PATCH v2.6.36-rc7 1/2] isdn/capi: unregister capictr notifier after init failure

[Tejun Heo]

capidrv_init() could leave capictr notifier dangling after init
failure.  Fix it.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/isdn/capi/capidrv.c |    1 +
 1 file changed, 1 insertion(+)

Index: work/drivers/isdn/capi/capidrv.c
===================================================================
--- work.orig/drivers/isdn/capi/capidrv.c
+++ work/drivers/isdn/capi/capidrv.c
@@ -2288,6 +2288,7 @@ static int __init capidrv_init(void)

 	errcode = capi20_get_profile(0, &profile);
 	if (errcode != CAPI_NOERROR) {
+		unregister_capictr_notifier(&capictr_nb);
 		capi20_release(&global.ap);
 		return -EIO;
 	}

[PATCH v2.6.36-rc7 2/2] isdn/capi: make kcapi use a separate workqueue

[Tejun Heo]

flush_scheduled_work() is deprecated and will be removed.  Because
kcapi uses fire-and-forget type works, it's impossible to flush each
work explicitly.  Create and use a dedicated workqueue instead.

Please note that with recent workqueue changes, each workqueue doesn't
reserve a lot of resources and using it as a flush domain is fine.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/isdn/capi/kcapi.c |   26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

Index: work/drivers/isdn/capi/kcapi.c
===================================================================
--- work.orig/drivers/isdn/capi/kcapi.c
+++ work/drivers/isdn/capi/kcapi.c
@@ -38,6 +38,7 @@
 #include <linux/rcupdate.h>

 static int showcapimsgs = 0;
+static struct workqueue_struct *kcapi_wq;

 MODULE_DESCRIPTION("CAPI4Linux: kernel CAPI layer");
 MODULE_AUTHOR("Carsten Paeth");
@@ -282,7 +283,7 @@ static int notify_push(unsigned int even
 	event->type = event_type;
 	event->controller = controller;

-	schedule_work(&event->work);
+	queue_work(kcapi_wq, &event->work);
 	return 0;
 }

@@ -399,7 +400,7 @@ void capi_ctr_handle_message(struct capi
 		goto error;
 	}
 	skb_queue_tail(&ap->recv_queue, skb);
-	schedule_work(&ap->recv_work);
+	queue_work(kcapi_wq, &ap->recv_work);
 	rcu_read_unlock();

 	return;
@@ -734,7 +735,7 @@ u16 capi20_release(struct capi20_appl *a

 	mutex_unlock(&capi_controller_lock);

-	flush_scheduled_work();
+	flush_workqueue(kcapi_wq);
 	skb_queue_purge(&ap->recv_queue);

 	if (showcapimsgs & 1) {
@@ -1276,21 +1277,30 @@ static int __init kcapi_init(void)
 {
 	int err;

+	kcapi_wq = alloc_workqueue("kcapi", 0, 0);
+	if (!kcapi_wq)
+		return -ENOMEM;
+
 	register_capictr_notifier(&capictr_nb);

 	err = cdebug_init();
-	if (!err)
-		kcapi_proc_init();
-	return err;
+	if (err) {
+		unregister_capictr_notifier(&capictr_nb);
+		destroy_workqueue(kcapi_wq);
+		return err;
+	}
+
+	kcapi_proc_init();
+	return 0;
 }

 static void __exit kcapi_exit(void)
 {
         kcapi_proc_exit();

-	/* make sure all notifiers are finished */
-	flush_scheduled_work();
+	unregister_capictr_notifier(&capictr_nb);
 	cdebug_exit();
+	destroy_workqueue(kcapi_wq);
 }

 module_init(kcapi_init);

Re: [PATCH v2.6.36-rc7 2/2] isdn/capi: make kcapi use a separate workqueue

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2780 bytes --]

Am 15.10.2010 14:43, Tejun Heo wrote:
> flush_scheduled_work() is deprecated and will be removed.  Because
> kcapi uses fire-and-forget type works, it's impossible to flush each
> work explicitly.  Create and use a dedicated workqueue instead.
> 
> Please note that with recent workqueue changes, each workqueue doesn't
> reserve a lot of resources and using it as a flush domain is fine.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  drivers/isdn/capi/kcapi.c |   26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> Index: work/drivers/isdn/capi/kcapi.c
> ===================================================================
> --- work.orig/drivers/isdn/capi/kcapi.c
> +++ work/drivers/isdn/capi/kcapi.c
> @@ -38,6 +38,7 @@
>  #include <linux/rcupdate.h>
> 
>  static int showcapimsgs = 0;
> +static struct workqueue_struct *kcapi_wq;
> 
>  MODULE_DESCRIPTION("CAPI4Linux: kernel CAPI layer");
>  MODULE_AUTHOR("Carsten Paeth");
> @@ -282,7 +283,7 @@ static int notify_push(unsigned int even
>  	event->type = event_type;
>  	event->controller = controller;
> 
> -	schedule_work(&event->work);
> +	queue_work(kcapi_wq, &event->work);
>  	return 0;
>  }
> 
> @@ -399,7 +400,7 @@ void capi_ctr_handle_message(struct capi
>  		goto error;
>  	}
>  	skb_queue_tail(&ap->recv_queue, skb);
> -	schedule_work(&ap->recv_work);
> +	queue_work(kcapi_wq, &ap->recv_work);
>  	rcu_read_unlock();
> 
>  	return;
> @@ -734,7 +735,7 @@ u16 capi20_release(struct capi20_appl *a
> 
>  	mutex_unlock(&capi_controller_lock);
> 
> -	flush_scheduled_work();
> +	flush_workqueue(kcapi_wq);
>  	skb_queue_purge(&ap->recv_queue);
> 
>  	if (showcapimsgs & 1) {
> @@ -1276,21 +1277,30 @@ static int __init kcapi_init(void)
>  {
>  	int err;
> 
> +	kcapi_wq = alloc_workqueue("kcapi", 0, 0);
> +	if (!kcapi_wq)
> +		return -ENOMEM;
> +
>  	register_capictr_notifier(&capictr_nb);
> 
>  	err = cdebug_init();
> -	if (!err)
> -		kcapi_proc_init();
> -	return err;
> +	if (err) {
> +		unregister_capictr_notifier(&capictr_nb);

Actually, this and the one on exit are unneeded as the notifier is
driven by this module itself. Anyway, they are also harmless. So let's
leave them in to avoid future confusions.

> +		destroy_workqueue(kcapi_wq);
> +		return err;
> +	}
> +
> +	kcapi_proc_init();
> +	return 0;
>  }
> 
>  static void __exit kcapi_exit(void)
>  {
>          kcapi_proc_exit();
> 
> -	/* make sure all notifiers are finished */
> -	flush_scheduled_work();
> +	unregister_capictr_notifier(&capictr_nb);
>  	cdebug_exit();
> +	destroy_workqueue(kcapi_wq);
>  }
> 
>  module_init(kcapi_init);

Looks and works fine.

Acked-by: Jan Kiszka <jan.kiszka@web.de>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH v2.6.36-rc7 1/2] isdn/capi: unregister capictr notifier after init failure

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 857 bytes --]

Am 15.10.2010 14:42, Tejun Heo wrote:
> capidrv_init() could leave capictr notifier dangling after init
> failure.  Fix it.

Good catch!

I assume David will pick these two up for net-next. Adding him to CC.

Acked-by: Jan Kiszka <jan.kiszka@web.de>

> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  drivers/isdn/capi/capidrv.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> Index: work/drivers/isdn/capi/capidrv.c
> ===================================================================
> --- work.orig/drivers/isdn/capi/capidrv.c
> +++ work/drivers/isdn/capi/capidrv.c
> @@ -2288,6 +2288,7 @@ static int __init capidrv_init(void)
> 
>  	errcode = capi20_get_profile(0, &profile);
>  	if (errcode != CAPI_NOERROR) {
> +		unregister_capictr_notifier(&capictr_nb);
>  		capi20_release(&global.ap);
>  		return -EIO;
>  	}



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]