[PATCH V2] staging: dgap: introduce dgap_stop()

[PATCH V2] staging: dgap: introduce dgap_stop()

[Daeseok Youn]

The dgap_init_module() need to unwind for cleanup variables properly.
Because dgap_init_module() calls dgap_cleanup_module() for freeing
variables but this function is possible to free variables
which are not allocated.

Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
---
V2: change ulong which is non-standard to "unsigned long".

 drivers/staging/dgap/dgap.c |   27 ++++++++++++++++++++++-----
 1 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index 7c79fe6..290ca3b 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -71,6 +71,7 @@ MODULE_DESCRIPTION("Driver for the Digi International EPCA PCI based product lin
 MODULE_SUPPORTED_DEVICE("dgap");
 
 static int dgap_start(void);
+static void dgap_stop(void);
 static void dgap_init_globals(void);
 static struct board_t *dgap_found_board(struct pci_dev *pdev, int id,
 					int boardnum);
@@ -479,19 +480,20 @@ static int dgap_init_module(void)
 
 	rc = pci_register_driver(&dgap_driver);
 	if (rc)
-		goto err_cleanup;
+		goto err_stop;
 
 	rc = dgap_create_driver_sysfiles(&dgap_driver);
 	if (rc)
-		goto err_cleanup;
+		goto err_unregister;
 
 	dgap_driver_state = DRIVER_READY;
 
 	return 0;
 
-err_cleanup:
-
-	dgap_cleanup_module();
+err_unregister:
+	pci_unregister_driver(&dgap_driver);
+err_stop:
+	dgap_stop();
 
 	return rc;
 }
@@ -561,6 +563,21 @@ failed_class:
 	return rc;
 }
 
+static void dgap_stop(void)
+{
+	unsigned long lock_flags;
+
+	spin_lock_irqsave(&dgap_poll_lock, lock_flags);
+	dgap_poll_stop = 1;
+	spin_unlock_irqrestore(&dgap_poll_lock, lock_flags);
+
+	del_timer_sync(&dgap_poll_timer);
+
+	device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 0));
+	class_destroy(dgap_class);
+	unregister_chrdev(DIGI_DGAP_MAJOR, "dgap");
+}
+
 static int dgap_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	int rc;
-- 
1.7.1

Re: [PATCH V2] staging: dgap: introduce dgap_stop()

[Sudip Mukherjee]

On Thu, Oct 09, 2014 at 01:40:11PM +0900, Daeseok Youn wrote:
> The dgap_init_module() need to unwind for cleanup variables properly.
> Because dgap_init_module() calls dgap_cleanup_module() for freeing
> variables but this function is possible to free variables
> which are not allocated.
> 
> Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
> ---
> V2: change ulong which is non-standard to "unsigned long".
I think , Dan, in his review of your v1 , asked you to rearrange the functions and get rid of the forward declarations.

thanks
sudip

> 
>  drivers/staging/dgap/dgap.c |   27 ++++++++++++++++++++++-----
>  1 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
> index 7c79fe6..290ca3b 100644
> --- a/drivers/staging/dgap/dgap.c
> +++ b/drivers/staging/dgap/dgap.c
> @@ -71,6 +71,7 @@ MODULE_DESCRIPTION("Driver for the Digi International EPCA PCI based product lin
>  MODULE_SUPPORTED_DEVICE("dgap");
>  
>  static int dgap_start(void);
> +static void dgap_stop(void);
>  static void dgap_init_globals(void);
>  static struct board_t *dgap_found_board(struct pci_dev *pdev, int id,
>  					int boardnum);
> @@ -479,19 +480,20 @@ static int dgap_init_module(void)
>  
>  	rc = pci_register_driver(&dgap_driver);
>  	if (rc)
> -		goto err_cleanup;
> +		goto err_stop;
>  
>  	rc = dgap_create_driver_sysfiles(&dgap_driver);
>  	if (rc)
> -		goto err_cleanup;
> +		goto err_unregister;
>  
>  	dgap_driver_state = DRIVER_READY;
>  
>  	return 0;
>  
> -err_cleanup:
> -
> -	dgap_cleanup_module();
> +err_unregister:
> +	pci_unregister_driver(&dgap_driver);
> +err_stop:
> +	dgap_stop();
>  
>  	return rc;
>  }
> @@ -561,6 +563,21 @@ failed_class:
>  	return rc;
>  }
>  
> +static void dgap_stop(void)
> +{
> +	unsigned long lock_flags;
> +
> +	spin_lock_irqsave(&dgap_poll_lock, lock_flags);
> +	dgap_poll_stop = 1;
> +	spin_unlock_irqrestore(&dgap_poll_lock, lock_flags);
> +
> +	del_timer_sync(&dgap_poll_timer);
> +
> +	device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 0));
> +	class_destroy(dgap_class);
> +	unregister_chrdev(DIGI_DGAP_MAJOR, "dgap");
> +}
> +
>  static int dgap_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
>  	int rc;
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2] staging: dgap: introduce dgap_stop()

[Dan Carpenter]

On Thu, Oct 09, 2014 at 03:19:03PM +0530, Sudip Mukherjee wrote:
> On Thu, Oct 09, 2014 at 01:40:11PM +0900, Daeseok Youn wrote:
> > The dgap_init_module() need to unwind for cleanup variables properly.
> > Because dgap_init_module() calls dgap_cleanup_module() for freeing
> > variables but this function is possible to free variables
> > which are not allocated.
> > 
> > Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
> > ---
> > V2: change ulong which is non-standard to "unsigned long".
> I think , Dan, in his review of your v1 , asked you to rearrange the functions and get rid of the forward declarations.
> 

I was fine with v1 actually...  It's a bug fix, and this is staging and
there are tons of forward declarations already.  Eventually we need to
get rid of them but it can be done later.

regards,
dan carpenter

Re: [PATCH V2] staging: dgap: introduce dgap_stop()

[DaeSeok Youn]

Hi,

2014-10-09 19:23 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> On Thu, Oct 09, 2014 at 03:19:03PM +0530, Sudip Mukherjee wrote:
>> On Thu, Oct 09, 2014 at 01:40:11PM +0900, Daeseok Youn wrote:
>> > The dgap_init_module() need to unwind for cleanup variables properly.
>> > Because dgap_init_module() calls dgap_cleanup_module() for freeing
>> > variables but this function is possible to free variables
>> > which are not allocated.
>> >
>> > Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
>> > ---
>> > V2: change ulong which is non-standard to "unsigned long".
>> I think , Dan, in his review of your v1 , asked you to rearrange the functions and get rid of the forward declarations.
>>
>
> I was fine with v1 actually...  It's a bug fix, and this is staging and
> there are tons of forward declarations already.  Eventually we need to
> get rid of them but it can be done later.
Yes, I will cleanup as your review.

thanks.

regards,
Daeseok Youn.

>
> regards,
> dan carpenter
>