[PATCH v2] staging: panel: fix stackdump

[PATCH v2] staging: panel: fix stackdump

[Sudip Mukherjee]

if we load the module, unload and then again try to load the module, we
will get a stackdump. In the module_exit function we are unregistering
the device and releasing the parport. So when we reach the detach
function parport is already null and the unregister_reboot_notifier()
is never called. When we again try to load the module it again tries
register_reboot_notifier() and gives us a stackdump as its earlier
registration is still not removed. It was caused by the commit
<bb046fef9668baf1c4744a3c7aba05e15e44c9ac>.
Fix this by moving all the unregistering and releasing in the detach
function, which should be the ideal case as the detach will be called if
we try to unregister the driver or if the parport is removed.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---

v2: addded previous commit id to commit message

Faced this problem while working on the device-model code of
parallelport.

 drivers/staging/panel/panel.c | 44 +++++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 1d8ed8b..0046ee0 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -2250,8 +2250,28 @@ static void panel_detach(struct parport *port)
 		       __func__, port->number, parport);
 		return;
 	}
+	if (scan_timer.function != NULL)
+		del_timer_sync(&scan_timer);
 
-	unregister_reboot_notifier(&panel_notifier);
+	if (pprt != NULL) {
+		if (keypad.enabled) {
+			misc_deregister(&keypad_dev);
+			keypad_initialized = 0;
+		}
+
+		if (lcd.enabled) {
+			panel_lcd_print("\x0cLCD driver " PANEL_VERSION
+					"\nunloaded.\x1b[Lc\x1b[Lb\x1b[L-");
+			misc_deregister(&lcd_dev);
+			lcd.initialized = false;
+		}
+
+		/* TODO: free all input signals */
+		parport_release(pprt);
+		parport_unregister_device(pprt);
+		pprt = NULL;
+		unregister_reboot_notifier(&panel_notifier);
+	}
 }
 
 static struct parport_driver panel_driver = {
@@ -2384,28 +2404,6 @@ static int __init panel_init_module(void)
 
 static void __exit panel_cleanup_module(void)
 {
-
-	if (scan_timer.function != NULL)
-		del_timer_sync(&scan_timer);
-
-	if (pprt != NULL) {
-		if (keypad.enabled) {
-			misc_deregister(&keypad_dev);
-			keypad_initialized = 0;
-		}
-
-		if (lcd.enabled) {
-			panel_lcd_print("\x0cLCD driver " PANEL_VERSION
-					"\nunloaded.\x1b[Lc\x1b[Lb\x1b[L-");
-			misc_deregister(&lcd_dev);
-			lcd.initialized = false;
-		}
-
-		/* TODO: free all input signals */
-		parport_release(pprt);
-		parport_unregister_device(pprt);
-		pprt = NULL;
-	}
 	parport_unregister_driver(&panel_driver);
 }
 
-- 
1.8.1.2

Re: [PATCH v2] staging: panel: fix stackdump

[Dan Carpenter]

On Tue, May 12, 2015 at 04:45:44PM +0530, Sudip Mukherjee wrote:
> if we load the module, unload and then again try to load the module, we
> will get a stackdump. In the module_exit function we are unregistering
> the device and releasing the parport. So when we reach the detach
> function parport is already null and the unregister_reboot_notifier()
> is never called. When we again try to load the module it again tries
> register_reboot_notifier() and gives us a stackdump as its earlier
> registration is still not removed. It was caused by the commit
> <bb046fef9668baf1c4744a3c7aba05e15e44c9ac>.

The right format is:

commit bb046fef9668 ('staging: panel: register reboot')

It is in case human beings reading your description don't remember which
patch bb046fef9668 was.

> Fix this by moving all the unregistering and releasing in the detach
> function, which should be the ideal case as the detach will be called if
> we try to unregister the driver or if the parport is removed.
> 

We also have the Fixes: tag.

Fixes: bb046fef9668 ('staging: panel: register reboot')

> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>

regards,
dan carpenter

Re: [PATCH v2] staging: panel: fix stackdump

[Sudip Mukherjee]

On Tue, May 12, 2015 at 02:27:13PM +0300, Dan Carpenter wrote:
> On Tue, May 12, 2015 at 04:45:44PM +0530, Sudip Mukherjee wrote:
> > if we load the module, unload and then again try to load the module, we
> > will get a stackdump. In the module_exit function we are unregistering
> > the device and releasing the parport. So when we reach the detach
> > function parport is already null and the unregister_reboot_notifier()
> > is never called. When we again try to load the module it again tries
> > register_reboot_notifier() and gives us a stackdump as its earlier
> > registration is still not removed. It was caused by the commit
> > <bb046fef9668baf1c4744a3c7aba05e15e44c9ac>.
> 
> The right format is:
> 
> commit bb046fef9668 ('staging: panel: register reboot')
> 
> It is in case human beings reading your description don't remember which
> patch bb046fef9668 was.
> 
> > Fix this by moving all the unregistering and releasing in the detach
> > function, which should be the ideal case as the detach will be called if
> > we try to unregister the driver or if the parport is removed.
> > 
> 
> We also have the Fixes: tag.
> 
> Fixes: bb046fef9668 ('staging: panel: register reboot')

ok, thanks.
I will send the v3 with the commit in correct format.

regards
sudip
> 
> > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> 
> regards,
> dan carpenter
>