[PATCH 1/3] staging: dgap: remove unneeded status variables

[PATCH 1/3] staging: dgap: remove unneeded status variables

[Alexey Khoroshilov]

dgap_driver_start and dgap_Major_Control_Registered are used
to keep status of initialization of the driver as a whole and its "Major Control".
But the code that checks them is executed once on module init/unload.
That makes no sense in these variables as far as their values are predictable
at any time.

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/staging/dgap/dgap_driver.c | 105 ++++++++++++++++---------------------
 1 file changed, 46 insertions(+), 59 deletions(-)

diff --git a/drivers/staging/dgap/dgap_driver.c b/drivers/staging/dgap/dgap_driver.c
index 089d017fc291..d7f1e999aaa4 100644
--- a/drivers/staging/dgap/dgap_driver.c
+++ b/drivers/staging/dgap/dgap_driver.c
@@ -117,9 +117,6 @@ int			dgap_poll_tick = 20;	/* Poll interval - 20 ms */
 /*
  * Static vars.
  */
-static int		dgap_Major_Control_Registered = FALSE;
-static uint		dgap_driver_start = FALSE;
-
 static struct class *	dgap_class;
 
 /*
@@ -283,65 +280,57 @@ static int dgap_start(void)
 	int rc = 0;
 	unsigned long flags;
 
-	if (dgap_driver_start == FALSE) {
-
-		dgap_driver_start = TRUE;
+	/* make sure that the globals are init'd before we do anything else */
+	dgap_init_globals();
 
-	        /* make sure that the globals are init'd before we do anything else */
-	        dgap_init_globals();
+	dgap_NumBoards = 0;
 
-		dgap_NumBoards = 0;
+	APR(("For the tools package or updated drivers please visit http://www.digi.com\n"));
 
-		APR(("For the tools package or updated drivers please visit http://www.digi.com\n"));
-
-		/*
-		 * Register our base character device into the kernel.
-		 * This allows the download daemon to connect to the downld device
-		 * before any of the boards are init'ed.
-		 */
-		if (!dgap_Major_Control_Registered) {
-			/*
-			 * Register management/dpa devices
-			 */
-			rc = register_chrdev(DIGI_DGAP_MAJOR, "dgap", &DgapBoardFops);
-			if (rc < 0) {
-				APR(("Can't register dgap driver device (%d)\n", rc));
-				return (rc);
-			}
+	/*
+	 * Register our base character device into the kernel.
+	 * This allows the download daemon to connect to the downld device
+	 * before any of the boards are init'ed.
+	 */
 
-			dgap_class = class_create(THIS_MODULE, "dgap_mgmt");
-			device_create(dgap_class, NULL,
-				MKDEV(DIGI_DGAP_MAJOR, 0),
-				NULL, "dgap_mgmt");
-			device_create(dgap_class, NULL,
-				MKDEV(DIGI_DGAP_MAJOR, 1),
-				NULL, "dgap_downld");
-			dgap_Major_Control_Registered = TRUE;
-		}
+	/*
+	 * Register management/dpa devices
+	 */
+	rc = register_chrdev(DIGI_DGAP_MAJOR, "dgap", &DgapBoardFops);
+	if (rc < 0) {
+		APR(("Can't register dgap driver device (%d)\n", rc));
+		return (rc);
+	}
 
-		/*
-		 * Init any global tty stuff.
-		 */
-		rc = dgap_tty_preinit();
+	dgap_class = class_create(THIS_MODULE, "dgap_mgmt");
+	device_create(dgap_class, NULL,
+		MKDEV(DIGI_DGAP_MAJOR, 0),
+		NULL, "dgap_mgmt");
+	device_create(dgap_class, NULL,
+		MKDEV(DIGI_DGAP_MAJOR, 1),
+		NULL, "dgap_downld");
 
-		if (rc < 0) {
-			APR(("tty preinit - not enough memory (%d)\n", rc));
-			return(rc); 
-		}
+	/*
+	 * Init any global tty stuff.
+	 */
+	rc = dgap_tty_preinit();
+	if (rc < 0) {
+		APR(("tty preinit - not enough memory (%d)\n", rc));
+		return(rc);
+	}
 
-		/* Start the poller */
-		DGAP_LOCK(dgap_poll_lock, flags);
-		init_timer(&dgap_poll_timer);
-		dgap_poll_timer.function = dgap_poll_handler;
-		dgap_poll_timer.data = 0;
-		dgap_poll_time = jiffies + dgap_jiffies_from_ms(dgap_poll_tick);
-		dgap_poll_timer.expires = dgap_poll_time;
-		DGAP_UNLOCK(dgap_poll_lock, flags);
+	/* Start the poller */
+	DGAP_LOCK(dgap_poll_lock, flags);
+	init_timer(&dgap_poll_timer);
+	dgap_poll_timer.function = dgap_poll_handler;
+	dgap_poll_timer.data = 0;
+	dgap_poll_time = jiffies + dgap_jiffies_from_ms(dgap_poll_tick);
+	dgap_poll_timer.expires = dgap_poll_time;
+	DGAP_UNLOCK(dgap_poll_lock, flags);
 
-		add_timer(&dgap_poll_timer);
+	add_timer(&dgap_poll_timer);
 
-		dgap_driver_state = DRIVER_NEED_CONFIG_LOAD;
-	}
+	dgap_driver_state = DRIVER_NEED_CONFIG_LOAD;
 
 	return (rc);
 }
@@ -409,12 +398,10 @@ void dgap_cleanup_module(void)
 	dgap_remove_driver_sysfiles(&dgap_driver);
 
 
-	if (dgap_Major_Control_Registered) {
-		device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 0));
-		device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 1));
-		class_destroy(dgap_class);
-		unregister_chrdev(DIGI_DGAP_MAJOR, "dgap");
-	}
+	device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 0));
+	device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 1));
+	class_destroy(dgap_class);
+	unregister_chrdev(DIGI_DGAP_MAJOR, "dgap");
 
 	kfree(dgap_config_buf);
 
-- 
1.8.3.2

[PATCH 1/3] staging: dgap: remove unneeded status variables

[Alexey Khoroshilov]

dgap_driver_start and dgap_Major_Control_Registered are used
to keep status of initialization of the driver as a whole and its "Major Control".
But the code that checks them is executed once on module init/unload.
That makes no sense in these variables as far as their values are predictable
at any time.

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/staging/dgap/dgap_driver.c | 105 ++++++++++++++++---------------------
 1 file changed, 46 insertions(+), 59 deletions(-)

diff --git a/drivers/staging/dgap/dgap_driver.c b/drivers/staging/dgap/dgap_driver.c
index 089d017fc291..d7f1e999aaa4 100644
--- a/drivers/staging/dgap/dgap_driver.c
+++ b/drivers/staging/dgap/dgap_driver.c
@@ -117,9 +117,6 @@ int			dgap_poll_tick = 20;	/* Poll interval - 20 ms */
 /*
  * Static vars.
  */
-static int		dgap_Major_Control_Registered = FALSE;
-static uint		dgap_driver_start = FALSE;
-
 static struct class *	dgap_class;
 
 /*
@@ -283,65 +280,57 @@ static int dgap_start(void)
 	int rc = 0;
 	unsigned long flags;
 
-	if (dgap_driver_start == FALSE) {
-
-		dgap_driver_start = TRUE;
+	/* make sure that the globals are init'd before we do anything else */
+	dgap_init_globals();
 
-	        /* make sure that the globals are init'd before we do anything else */
-	        dgap_init_globals();
+	dgap_NumBoards = 0;
 
-		dgap_NumBoards = 0;
+	APR(("For the tools package or updated drivers please visit http://www.digi.com\n"));
 
-		APR(("For the tools package or updated drivers please visit http://www.digi.com\n"));
-
-		/*
-		 * Register our base character device into the kernel.
-		 * This allows the download daemon to connect to the downld device
-		 * before any of the boards are init'ed.
-		 */
-		if (!dgap_Major_Control_Registered) {
-			/*
-			 * Register management/dpa devices
-			 */
-			rc = register_chrdev(DIGI_DGAP_MAJOR, "dgap", &DgapBoardFops);
-			if (rc < 0) {
-				APR(("Can't register dgap driver device (%d)\n", rc));
-				return (rc);
-			}
+	/*
+	 * Register our base character device into the kernel.
+	 * This allows the download daemon to connect to the downld device
+	 * before any of the boards are init'ed.
+	 */
 
-			dgap_class = class_create(THIS_MODULE, "dgap_mgmt");
-			device_create(dgap_class, NULL,
-				MKDEV(DIGI_DGAP_MAJOR, 0),
-				NULL, "dgap_mgmt");
-			device_create(dgap_class, NULL,
-				MKDEV(DIGI_DGAP_MAJOR, 1),
-				NULL, "dgap_downld");
-			dgap_Major_Control_Registered = TRUE;
-		}
+	/*
+	 * Register management/dpa devices
+	 */
+	rc = register_chrdev(DIGI_DGAP_MAJOR, "dgap", &DgapBoardFops);
+	if (rc < 0) {
+		APR(("Can't register dgap driver device (%d)\n", rc));
+		return (rc);
+	}
 
-		/*
-		 * Init any global tty stuff.
-		 */
-		rc = dgap_tty_preinit();
+	dgap_class = class_create(THIS_MODULE, "dgap_mgmt");
+	device_create(dgap_class, NULL,
+		MKDEV(DIGI_DGAP_MAJOR, 0),
+		NULL, "dgap_mgmt");
+	device_create(dgap_class, NULL,
+		MKDEV(DIGI_DGAP_MAJOR, 1),
+		NULL, "dgap_downld");
 
-		if (rc < 0) {
-			APR(("tty preinit - not enough memory (%d)\n", rc));
-			return(rc); 
-		}
+	/*
+	 * Init any global tty stuff.
+	 */
+	rc = dgap_tty_preinit();
+	if (rc < 0) {
+		APR(("tty preinit - not enough memory (%d)\n", rc));
+		return(rc);
+	}
 
-		/* Start the poller */
-		DGAP_LOCK(dgap_poll_lock, flags);
-		init_timer(&dgap_poll_timer);
-		dgap_poll_timer.function = dgap_poll_handler;
-		dgap_poll_timer.data = 0;
-		dgap_poll_time = jiffies + dgap_jiffies_from_ms(dgap_poll_tick);
-		dgap_poll_timer.expires = dgap_poll_time;
-		DGAP_UNLOCK(dgap_poll_lock, flags);
+	/* Start the poller */
+	DGAP_LOCK(dgap_poll_lock, flags);
+	init_timer(&dgap_poll_timer);
+	dgap_poll_timer.function = dgap_poll_handler;
+	dgap_poll_timer.data = 0;
+	dgap_poll_time = jiffies + dgap_jiffies_from_ms(dgap_poll_tick);
+	dgap_poll_timer.expires = dgap_poll_time;
+	DGAP_UNLOCK(dgap_poll_lock, flags);
 
-		add_timer(&dgap_poll_timer);
+	add_timer(&dgap_poll_timer);
 
-		dgap_driver_state = DRIVER_NEED_CONFIG_LOAD;
-	}
+	dgap_driver_state = DRIVER_NEED_CONFIG_LOAD;
 
 	return (rc);
 }
@@ -409,12 +398,10 @@ void dgap_cleanup_module(void)
 	dgap_remove_driver_sysfiles(&dgap_driver);
 
 
-	if (dgap_Major_Control_Registered) {
-		device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 0));
-		device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 1));
-		class_destroy(dgap_class);
-		unregister_chrdev(DIGI_DGAP_MAJOR, "dgap");
-	}
+	device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 0));
+	device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 1));
+	class_destroy(dgap_class);
+	unregister_chrdev(DIGI_DGAP_MAJOR, "dgap");
 
 	kfree(dgap_config_buf);
 
-- 
1.8.3.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/3] staging: dgap: implement proper error handling in dgap_start()

[Alexey Khoroshilov]

dgap_start() ignored errors in class_create() and device_create().
The patch implements proper error handling.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/staging/dgap/dgap_driver.c | 42 +++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/dgap/dgap_driver.c b/drivers/staging/dgap/dgap_driver.c
index d7f1e999aaa4..136c879b560c 100644
--- a/drivers/staging/dgap/dgap_driver.c
+++ b/drivers/staging/dgap/dgap_driver.c
@@ -279,6 +279,7 @@ static int dgap_start(void)
 {
 	int rc = 0;
 	unsigned long flags;
+	struct device *device;
 
         /* make sure that the globals are init'd before we do anything else */
         dgap_init_globals();
@@ -303,12 +304,29 @@ static int dgap_start(void)
 	}
 
 	dgap_class = class_create(THIS_MODULE, "dgap_mgmt");
-	device_create(dgap_class, NULL,
-		MKDEV(DIGI_DGAP_MAJOR, 0),
-		NULL, "dgap_mgmt");
-	device_create(dgap_class, NULL,
-		MKDEV(DIGI_DGAP_MAJOR, 1),
-		NULL, "dgap_downld");
+	if (IS_ERR(dgap_class)) {
+		rc = PTR_ERR(dgap_class);
+		APR(("Can't create dgap_mgmt class (%d)\n", rc));
+		goto failed_class;
+	}
+
+	device = device_create(dgap_class, NULL,
+			MKDEV(DIGI_DGAP_MAJOR, 0),
+			NULL, "dgap_mgmt");
+	if (IS_ERR(device)) {
+		rc = PTR_ERR(device);
+		APR(("Can't create device (%d)\n", rc));
+		goto failed_device0;
+	}
+
+	device = device_create(dgap_class, NULL,
+			MKDEV(DIGI_DGAP_MAJOR, 1),
+			NULL, "dgap_downld");
+	if (IS_ERR(device)) {
+		rc = PTR_ERR(device);
+		APR(("Can't create device (%d)\n", rc));
+		goto failed_device1;
+	}
 
 	/*
 	 * Init any global tty stuff.
@@ -316,7 +334,7 @@ static int dgap_start(void)
 	rc = dgap_tty_preinit();
 	if (rc < 0) {
 		APR(("tty preinit - not enough memory (%d)\n", rc));
-		return(rc); 
+		goto failed_tty;
 	}
 
 	/* Start the poller */
@@ -333,6 +351,16 @@ static int dgap_start(void)
 	dgap_driver_state = DRIVER_NEED_CONFIG_LOAD;
 
 	return (rc);
+
+failed_tty:
+	device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 1));
+failed_device1:
+	device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 0));
+failed_device0:
+	class_destroy(dgap_class);
+failed_class:
+	unregister_chrdev(DIGI_DGAP_MAJOR, "dgap");
+	return (rc);
 }
 
 
-- 
1.8.3.2

[PATCH 3/3] staging: dgap: fix error handling in dgap_init_module()

[Alexey Khoroshilov]

No need to call pci_unregister_driver() if pci_register_driver() failed.

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/staging/dgap/dgap_driver.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/dgap/dgap_driver.c b/drivers/staging/dgap/dgap_driver.c
index 136c879b560c..cc6933db6feb 100644
--- a/drivers/staging/dgap/dgap_driver.c
+++ b/drivers/staging/dgap/dgap_driver.c
@@ -254,18 +254,10 @@ int dgap_init_module(void)
 	/*
 	 * If something went wrong in the scan, bail out of driver.
 	 */
-	if (rc < 0) {
-		/* Only unregister the pci driver if it was actually registered. */
-		if (dgap_NumBoards)
-			pci_unregister_driver(&dgap_driver);
-		else
-			printk("WARNING: dgap driver load failed.  No DGAP boards found.\n");
-
+	if (rc < 0)
 		dgap_cleanup_module();
-	}
-	else {
+	else
 		dgap_create_driver_sysfiles(&dgap_driver);
-	}
   
 	DPR_INIT(("Finished init_module. Returning %d\n", rc));
 	return (rc);
-- 
1.8.3.2

[PATCH 3/3] staging: dgap: fix error handling in dgap_init_module()

[Alexey Khoroshilov]

No need to call pci_unregister_driver() if pci_register_driver() failed.

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/staging/dgap/dgap_driver.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/dgap/dgap_driver.c b/drivers/staging/dgap/dgap_driver.c
index 136c879b560c..cc6933db6feb 100644
--- a/drivers/staging/dgap/dgap_driver.c
+++ b/drivers/staging/dgap/dgap_driver.c
@@ -254,18 +254,10 @@ int dgap_init_module(void)
 	/*
 	 * If something went wrong in the scan, bail out of driver.
 	 */
-	if (rc < 0) {
-		/* Only unregister the pci driver if it was actually registered. */
-		if (dgap_NumBoards)
-			pci_unregister_driver(&dgap_driver);
-		else
-			printk("WARNING: dgap driver load failed.  No DGAP boards found.\n");
-
+	if (rc < 0)
 		dgap_cleanup_module();
-	}
-	else {
+	else
 		dgap_create_driver_sysfiles(&dgap_driver);
-	}
   
 	DPR_INIT(("Finished init_module. Returning %d\n", rc));
 	return (rc);
-- 
1.8.3.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/3] staging: dgap: remove unneeded status variables

[Greg Kroah-Hartman]

On Mon, Feb 24, 2014 at 12:21:51AM +0400, Alexey Khoroshilov wrote:
> dgap_driver_start and dgap_Major_Control_Registered are used
> to keep status of initialization of the driver as a whole and its "Major Control".
> But the code that checks them is executed once on module init/unload.
> That makes no sense in these variables as far as their values are predictable
> at any time.
> 
> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> ---
>  drivers/staging/dgap/dgap_driver.c | 105 ++++++++++++++++---------------------
>  1 file changed, 46 insertions(+), 59 deletions(-)

This file is no longer in my tree, as the driver has been merged to one
file.  Can you please redo it against my latest staging-next branch of
my staging.git tree and resend the series so that I can apply them?

thanks,

greg k-h

Re: [PATCH 1/3] staging: dgap: remove unneeded status variables

[Greg Kroah-Hartman]

On Mon, Feb 24, 2014 at 12:21:51AM +0400, Alexey Khoroshilov wrote:
> dgap_driver_start and dgap_Major_Control_Registered are used
> to keep status of initialization of the driver as a whole and its "Major Control".
> But the code that checks them is executed once on module init/unload.
> That makes no sense in these variables as far as their values are predictable
> at any time.
> 
> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> ---
>  drivers/staging/dgap/dgap_driver.c | 105 ++++++++++++++++---------------------
>  1 file changed, 46 insertions(+), 59 deletions(-)

This file is no longer in my tree, as the driver has been merged to one
file.  Can you please redo it against my latest staging-next branch of
my staging.git tree and resend the series so that I can apply them?

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/3] staging: dgap: remove unneeded status variables

[Alexey Khoroshilov]

dgap_driver_start and dgap_Major_Control_Registered are used
to keep status of initialization of the driver as a whole and its "Major Control".
But the code that checks them is executed once on module init/unload.
That makes no sense in these variables as far as their values are predictable
at any time.

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/staging/dgap/dgap.c | 97 ++++++++++++++++++++-------------------------
 1 file changed, 42 insertions(+), 55 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index cbce457..5271856 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -254,9 +254,6 @@ static int dgap_poll_tick = 20;	/* Poll interval - 20 ms */
 /*
  * Static vars.
  */
-static int dgap_Major_Control_Registered = FALSE;
-static uint dgap_driver_start = FALSE;
-
 static struct class *dgap_class;
 
 static struct board_t *dgap_BoardsByMajor[256];
@@ -561,61 +558,54 @@ static int dgap_start(void)
 	int rc = 0;
 	unsigned long flags;
 
-	if (dgap_driver_start == FALSE) {
+	/*
+	 * make sure that the globals are
+	 * init'd before we do anything else
+	 */
+	dgap_init_globals();
 
-		dgap_driver_start = TRUE;
+	dgap_NumBoards = 0;
 
-		/*
-		 * make sure that the globals are
-		 * init'd before we do anything else
-		 */
-		dgap_init_globals();
+	pr_info("For the tools package please visit http://www.digi.com\n");
 
-		dgap_NumBoards = 0;
+	/*
+	 * Register our base character device into the kernel.
+	 * This allows the download daemon to connect to the downld device
+	 * before any of the boards are init'ed.
+	 */
 
-		pr_info("For the tools package please visit http://www.digi.com\n");
+	/*
+	 * Register management/dpa devices
+	 */
+	rc = register_chrdev(DIGI_DGAP_MAJOR, "dgap", &DgapBoardFops);
+	if (rc < 0)
+		return rc;
 
-		/*
-		 * Register our base character device into the kernel.
-		 * This allows the download daemon to connect to the downld device
-		 * before any of the boards are init'ed.
-		 */
-		if (!dgap_Major_Control_Registered) {
-			/*
-			 * Register management/dpa devices
-			 */
-			rc = register_chrdev(DIGI_DGAP_MAJOR, "dgap", &DgapBoardFops);
-			if (rc < 0)
-				return rc;
-
-			dgap_class = class_create(THIS_MODULE, "dgap_mgmt");
-			device_create(dgap_class, NULL,
-				MKDEV(DIGI_DGAP_MAJOR, 0),
-				NULL, "dgap_mgmt");
-			dgap_Major_Control_Registered = TRUE;
-		}
+	dgap_class = class_create(THIS_MODULE, "dgap_mgmt");
+	device_create(dgap_class, NULL,
+		MKDEV(DIGI_DGAP_MAJOR, 0),
+		NULL, "dgap_mgmt");
 
-		/*
-		 * Init any global tty stuff.
-		 */
-		rc = dgap_tty_preinit();
+	/*
+	 * Init any global tty stuff.
+	 */
+	rc = dgap_tty_preinit();
 
-		if (rc < 0)
-			return rc;
+	if (rc < 0)
+		return rc;
 
-		/* Start the poller */
-		DGAP_LOCK(dgap_poll_lock, flags);
-		init_timer(&dgap_poll_timer);
-		dgap_poll_timer.function = dgap_poll_handler;
-		dgap_poll_timer.data = 0;
-		dgap_poll_time = jiffies + dgap_jiffies_from_ms(dgap_poll_tick);
-		dgap_poll_timer.expires = dgap_poll_time;
-		DGAP_UNLOCK(dgap_poll_lock, flags);
+	/* Start the poller */
+	DGAP_LOCK(dgap_poll_lock, flags);
+	init_timer(&dgap_poll_timer);
+	dgap_poll_timer.function = dgap_poll_handler;
+	dgap_poll_timer.data = 0;
+	dgap_poll_time = jiffies + dgap_jiffies_from_ms(dgap_poll_tick);
+	dgap_poll_timer.expires = dgap_poll_time;
+	DGAP_UNLOCK(dgap_poll_lock, flags);
 
-		add_timer(&dgap_poll_timer);
+	add_timer(&dgap_poll_timer);
 
-		dgap_driver_state = DRIVER_NEED_CONFIG_LOAD;
-	}
+	dgap_driver_state = DRIVER_NEED_CONFIG_LOAD;
 
 	return rc;
 }
@@ -677,13 +667,10 @@ void dgap_cleanup_module(void)
 
 	dgap_remove_driver_sysfiles(&dgap_driver);
 
-
-	if (dgap_Major_Control_Registered) {
-		device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 0));
-		device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 1));
-		class_destroy(dgap_class);
-		unregister_chrdev(DIGI_DGAP_MAJOR, "dgap");
-	}
+	device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 0));
+	device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 1));
+	class_destroy(dgap_class);
+	unregister_chrdev(DIGI_DGAP_MAJOR, "dgap");
 
 	kfree(dgap_config_buf);
 
-- 
1.8.3.2

[PATCH 2/3] staging: dgap: implement proper error handling in dgap_start()

[Alexey Khoroshilov]

dgap_start() ignored errors in class_create() and device_create().
The patch implements proper error handling.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/staging/dgap/dgap.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index 5271856..b4157d7 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -557,6 +557,7 @@ static int dgap_start(void)
 {
 	int rc = 0;
 	unsigned long flags;
+	struct device *device;
 
 	/*
 	 * make sure that the globals are
@@ -582,9 +583,18 @@ static int dgap_start(void)
 		return rc;
 
 	dgap_class = class_create(THIS_MODULE, "dgap_mgmt");
-	device_create(dgap_class, NULL,
+	if (IS_ERR(dgap_class)) {
+		rc = PTR_ERR(dgap_class);
+		goto failed_class;
+	}
+
+	device = device_create(dgap_class, NULL,
 		MKDEV(DIGI_DGAP_MAJOR, 0),
 		NULL, "dgap_mgmt");
+	if (IS_ERR(device)) {
+		rc = PTR_ERR(device);
+		goto failed_device;
+	}
 
 	/*
 	 * Init any global tty stuff.
@@ -592,7 +602,7 @@ static int dgap_start(void)
 	rc = dgap_tty_preinit();
 
 	if (rc < 0)
-		return rc;
+		goto failed_tty;
 
 	/* Start the poller */
 	DGAP_LOCK(dgap_poll_lock, flags);
@@ -608,6 +618,14 @@ static int dgap_start(void)
 	dgap_driver_state = DRIVER_NEED_CONFIG_LOAD;
 
 	return rc;
+
+failed_tty:
+	device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 0));
+failed_device:
+	class_destroy(dgap_class);
+failed_class:
+	unregister_chrdev(DIGI_DGAP_MAJOR, "dgap");
+	return rc;
 }
 
 /*
-- 
1.8.3.2

[PATCH 3/3] staging: dgap: fix error handling in dgap_init_module()

[Alexey Khoroshilov]

No need to call pci_unregister_driver() if pci_register_driver() failed.

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/staging/dgap/dgap.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index b4157d7..510e8b3 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -535,18 +535,13 @@ int dgap_init_module(void)
 	 * If something went wrong in the scan, bail out of driver.
 	 */
 	if (rc < 0) {
-		/* Only unregister the pci driver if it was actually registered. */
-		if (dgap_NumBoards)
-			pci_unregister_driver(&dgap_driver);
-		else
-			printk("WARNING: dgap driver load failed.  No DGAP boards found.\n");
-
 		dgap_cleanup_module();
-	} else {
-		dgap_create_driver_sysfiles(&dgap_driver);
-		dgap_driver_state = DRIVER_READY;
+		return rc;
 	}
 
+	dgap_create_driver_sysfiles(&dgap_driver);
+	dgap_driver_state = DRIVER_READY;
+
 	return rc;
 }
 
-- 
1.8.3.2

Re: [PATCH 1/3] staging: dgap: remove unneeded status variables

[Greg Kroah-Hartman]

On Mon, Mar 03, 2014 at 01:08:06AM +0400, Alexey Khoroshilov wrote:
> dgap_driver_start and dgap_Major_Control_Registered are used
> to keep status of initialization of the driver as a whole and its "Major Control".
> But the code that checks them is executed once on module init/unload.
> That makes no sense in these variables as far as their values are predictable
> at any time.
> 
> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> ---
>  drivers/staging/dgap/dgap.c | 97 ++++++++++++++++++++-------------------------
>  1 file changed, 42 insertions(+), 55 deletions(-)

Due to other recent changes in this driver, none of these patches apply
anymore :(

Can you please redo them against my latest tree so that I can apply
them?

thanks,

greg k-h

[PATCH 1/2] staging: dgap: remove unneeded status variables

[Alexey Khoroshilov]

dgap_driver_start and dgap_Major_Control_Registered are used
to keep status of initialization of the driver as a whole and its "Major Control".
But the code that checks them is executed once on module init/unload.
That makes no sense in these variables as far as their values are predictable
at any time.

Also "dgap_downld" device was removed, while
device_destroy(MKDEV(DIGI_DGAP_MAJOR, 1)) is still in dgap_cleanup_module().
The patch removes it by the way.

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/staging/dgap/dgap.c | 81 ++++++++++++++++++---------------------------
 1 file changed, 33 insertions(+), 48 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index d00283a..ad5afbc 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -254,9 +254,6 @@ static int dgap_poll_tick = 20;	/* Poll interval - 20 ms */
 /*
  * Static vars.
  */
-static int dgap_Major_Control_Registered = FALSE;
-static uint dgap_driver_start = FALSE;
-
 static struct class *dgap_class;
 
 static struct board_t *dgap_BoardsByMajor[256];
@@ -551,52 +548,44 @@ static int dgap_start(void)
 	int rc = 0;
 	unsigned long flags;
 
-	if (dgap_driver_start == FALSE) {
+	/*
+	 * make sure that the globals are
+	 * init'd before we do anything else
+	 */
+	dgap_init_globals();
 
-		dgap_driver_start = TRUE;
+	dgap_NumBoards = 0;
 
-		/*
-		 * make sure that the globals are
-		 * init'd before we do anything else
-		 */
-		dgap_init_globals();
+	pr_info("For the tools package please visit http://www.digi.com\n");
 
-		dgap_NumBoards = 0;
+	/*
+	 * Register our base character device into the kernel.
+	 */
 
-		pr_info("For the tools package please visit http://www.digi.com\n");
+	/*
+	 * Register management/dpa devices
+	 */
+	rc = register_chrdev(DIGI_DGAP_MAJOR, "dgap", &DgapBoardFops);
+	if (rc < 0)
+		return rc;
 
-		/*
-		 * Register our base character device into the kernel.
-		 */
-		if (!dgap_Major_Control_Registered) {
-			/*
-			 * Register management/dpa devices
-			 */
-			rc = register_chrdev(DIGI_DGAP_MAJOR, "dgap",
-						 &DgapBoardFops);
-			if (rc < 0)
-				return rc;
-
-			dgap_class = class_create(THIS_MODULE, "dgap_mgmt");
-			device_create(dgap_class, NULL,
-				MKDEV(DIGI_DGAP_MAJOR, 0),
-				NULL, "dgap_mgmt");
-			dgap_Major_Control_Registered = TRUE;
-		}
+	dgap_class = class_create(THIS_MODULE, "dgap_mgmt");
+	device_create(dgap_class, NULL,
+		MKDEV(DIGI_DGAP_MAJOR, 0),
+		NULL, "dgap_mgmt");
 
-		/* Start the poller */
-		DGAP_LOCK(dgap_poll_lock, flags);
-		init_timer(&dgap_poll_timer);
-		dgap_poll_timer.function = dgap_poll_handler;
-		dgap_poll_timer.data = 0;
-		dgap_poll_time = jiffies + dgap_jiffies_from_ms(dgap_poll_tick);
-		dgap_poll_timer.expires = dgap_poll_time;
-		DGAP_UNLOCK(dgap_poll_lock, flags);
+	/* Start the poller */
+	DGAP_LOCK(dgap_poll_lock, flags);
+	init_timer(&dgap_poll_timer);
+	dgap_poll_timer.function = dgap_poll_handler;
+	dgap_poll_timer.data = 0;
+	dgap_poll_time = jiffies + dgap_jiffies_from_ms(dgap_poll_tick);
+	dgap_poll_timer.expires = dgap_poll_time;
+	DGAP_UNLOCK(dgap_poll_lock, flags);
 
-		add_timer(&dgap_poll_timer);
+	add_timer(&dgap_poll_timer);
 
-		dgap_driver_state = DRIVER_NEED_CONFIG_LOAD;
-	}
+	dgap_driver_state = DRIVER_NEED_CONFIG_LOAD;
 
 	return rc;
 }
@@ -658,13 +647,9 @@ static void dgap_cleanup_module(void)
 
 	dgap_remove_driver_sysfiles(&dgap_driver);
 
-
-	if (dgap_Major_Control_Registered) {
-		device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 0));
-		device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 1));
-		class_destroy(dgap_class);
-		unregister_chrdev(DIGI_DGAP_MAJOR, "dgap");
-	}
+	device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 0));
+	class_destroy(dgap_class);
+	unregister_chrdev(DIGI_DGAP_MAJOR, "dgap");
 
 	kfree(dgap_config_buf);
 
-- 
1.8.3.2

[PATCH 1/2] staging: dgap: remove unneeded status variables

[Alexey Khoroshilov]

dgap_driver_start and dgap_Major_Control_Registered are used
to keep status of initialization of the driver as a whole and its "Major Control".
But the code that checks them is executed once on module init/unload.
That makes no sense in these variables as far as their values are predictable
at any time.

Also "dgap_downld" device was removed, while
device_destroy(MKDEV(DIGI_DGAP_MAJOR, 1)) is still in dgap_cleanup_module().
The patch removes it by the way.

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/staging/dgap/dgap.c | 81 ++++++++++++++++++---------------------------
 1 file changed, 33 insertions(+), 48 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index d00283a..ad5afbc 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -254,9 +254,6 @@ static int dgap_poll_tick = 20;	/* Poll interval - 20 ms */
 /*
  * Static vars.
  */
-static int dgap_Major_Control_Registered = FALSE;
-static uint dgap_driver_start = FALSE;
-
 static struct class *dgap_class;
 
 static struct board_t *dgap_BoardsByMajor[256];
@@ -551,52 +548,44 @@ static int dgap_start(void)
 	int rc = 0;
 	unsigned long flags;
 
-	if (dgap_driver_start == FALSE) {
+	/*
+	 * make sure that the globals are
+	 * init'd before we do anything else
+	 */
+	dgap_init_globals();
 
-		dgap_driver_start = TRUE;
+	dgap_NumBoards = 0;
 
-		/*
-		 * make sure that the globals are
-		 * init'd before we do anything else
-		 */
-		dgap_init_globals();
+	pr_info("For the tools package please visit http://www.digi.com\n");
 
-		dgap_NumBoards = 0;
+	/*
+	 * Register our base character device into the kernel.
+	 */
 
-		pr_info("For the tools package please visit http://www.digi.com\n");
+	/*
+	 * Register management/dpa devices
+	 */
+	rc = register_chrdev(DIGI_DGAP_MAJOR, "dgap", &DgapBoardFops);
+	if (rc < 0)
+		return rc;
 
-		/*
-		 * Register our base character device into the kernel.
-		 */
-		if (!dgap_Major_Control_Registered) {
-			/*
-			 * Register management/dpa devices
-			 */
-			rc = register_chrdev(DIGI_DGAP_MAJOR, "dgap",
-						 &DgapBoardFops);
-			if (rc < 0)
-				return rc;
-
-			dgap_class = class_create(THIS_MODULE, "dgap_mgmt");
-			device_create(dgap_class, NULL,
-				MKDEV(DIGI_DGAP_MAJOR, 0),
-				NULL, "dgap_mgmt");
-			dgap_Major_Control_Registered = TRUE;
-		}
+	dgap_class = class_create(THIS_MODULE, "dgap_mgmt");
+	device_create(dgap_class, NULL,
+		MKDEV(DIGI_DGAP_MAJOR, 0),
+		NULL, "dgap_mgmt");
 
-		/* Start the poller */
-		DGAP_LOCK(dgap_poll_lock, flags);
-		init_timer(&dgap_poll_timer);
-		dgap_poll_timer.function = dgap_poll_handler;
-		dgap_poll_timer.data = 0;
-		dgap_poll_time = jiffies + dgap_jiffies_from_ms(dgap_poll_tick);
-		dgap_poll_timer.expires = dgap_poll_time;
-		DGAP_UNLOCK(dgap_poll_lock, flags);
+	/* Start the poller */
+	DGAP_LOCK(dgap_poll_lock, flags);
+	init_timer(&dgap_poll_timer);
+	dgap_poll_timer.function = dgap_poll_handler;
+	dgap_poll_timer.data = 0;
+	dgap_poll_time = jiffies + dgap_jiffies_from_ms(dgap_poll_tick);
+	dgap_poll_timer.expires = dgap_poll_time;
+	DGAP_UNLOCK(dgap_poll_lock, flags);
 
-		add_timer(&dgap_poll_timer);
+	add_timer(&dgap_poll_timer);
 
-		dgap_driver_state = DRIVER_NEED_CONFIG_LOAD;
-	}
+	dgap_driver_state = DRIVER_NEED_CONFIG_LOAD;
 
 	return rc;
 }
@@ -658,13 +647,9 @@ static void dgap_cleanup_module(void)
 
 	dgap_remove_driver_sysfiles(&dgap_driver);
 
-
-	if (dgap_Major_Control_Registered) {
-		device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 0));
-		device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 1));
-		class_destroy(dgap_class);
-		unregister_chrdev(DIGI_DGAP_MAJOR, "dgap");
-	}
+	device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 0));
+	class_destroy(dgap_class);
+	unregister_chrdev(DIGI_DGAP_MAJOR, "dgap");
 
 	kfree(dgap_config_buf);
 
-- 
1.8.3.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/2] staging: dgap: implement proper error handling in dgap_start()

[Alexey Khoroshilov]

dgap_start() ignored errors in class_create() and device_create().
The patch implements proper error handling.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/staging/dgap/dgap.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index ad5afbc..bfafe7e 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -547,6 +547,7 @@ static int dgap_start(void)
 {
 	int rc = 0;
 	unsigned long flags;
+	struct device *device;
 
 	/*
 	 * make sure that the globals are
@@ -570,9 +571,18 @@ static int dgap_start(void)
 		return rc;
 
 	dgap_class = class_create(THIS_MODULE, "dgap_mgmt");
-	device_create(dgap_class, NULL,
+	if (IS_ERR(dgap_class)) {
+		rc = PTR_ERR(dgap_class);
+		goto failed_class;
+	}
+
+	device = device_create(dgap_class, NULL,
 		MKDEV(DIGI_DGAP_MAJOR, 0),
 		NULL, "dgap_mgmt");
+	if (IS_ERR(device)) {
+		rc = PTR_ERR(device);
+		goto failed_device;
+	}
 
 	/* Start the poller */
 	DGAP_LOCK(dgap_poll_lock, flags);
@@ -588,6 +598,12 @@ static int dgap_start(void)
 	dgap_driver_state = DRIVER_NEED_CONFIG_LOAD;
 
 	return rc;
+
+failed_device:
+	class_destroy(dgap_class);
+failed_class:
+	unregister_chrdev(DIGI_DGAP_MAJOR, "dgap");
+	return rc;
 }
 
 /*
-- 
1.8.3.2

[PATCH 2/2] staging: dgap: implement proper error handling in dgap_start()

[Alexey Khoroshilov]

dgap_start() ignored errors in class_create() and device_create().
The patch implements proper error handling.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/staging/dgap/dgap.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index ad5afbc..bfafe7e 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -547,6 +547,7 @@ static int dgap_start(void)
 {
 	int rc = 0;
 	unsigned long flags;
+	struct device *device;
 
 	/*
 	 * make sure that the globals are
@@ -570,9 +571,18 @@ static int dgap_start(void)
 		return rc;
 
 	dgap_class = class_create(THIS_MODULE, "dgap_mgmt");
-	device_create(dgap_class, NULL,
+	if (IS_ERR(dgap_class)) {
+		rc = PTR_ERR(dgap_class);
+		goto failed_class;
+	}
+
+	device = device_create(dgap_class, NULL,
 		MKDEV(DIGI_DGAP_MAJOR, 0),
 		NULL, "dgap_mgmt");
+	if (IS_ERR(device)) {
+		rc = PTR_ERR(device);
+		goto failed_device;
+	}
 
 	/* Start the poller */
 	DGAP_LOCK(dgap_poll_lock, flags);
@@ -588,6 +598,12 @@ static int dgap_start(void)
 	dgap_driver_state = DRIVER_NEED_CONFIG_LOAD;
 
 	return rc;
+
+failed_device:
+	class_destroy(dgap_class);
+failed_class:
+	unregister_chrdev(DIGI_DGAP_MAJOR, "dgap");
+	return rc;
 }
 
 /*
-- 
1.8.3.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel