Re: [PATCH 3/3] staging: dgap: introduce dgap_stop()
Hi, Dan 2014-10-08 20:37 GMT+09:00 Dan Carpenter : > All three of these patches are good and a nice improvement. This one is > a good bugfix. I have some notes for later, though below. > > On Wed, Oct 08, 2014 at 08:13:56PM +0900, Daeseok Youn wrote: >> diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c >> index 7c79fe6..00f34b5 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); > > These kinds of forward declarations are annoying. The whole file needs > to be re-arranged so that we don't have to deal with them. OK. I will try to re-arrange and remove forward declarations. > >> @@ -561,6 +563,21 @@ failed_class: >> return rc; >> } >> >> +static void dgap_stop(void) >> +{ >> + ulong lock_flags; OK. I will send this patch again after changing ulong to "unsigned long". > > This is non-standard. Traditionally it would be: > > unsigned long flags; > > regards, > dan carpenter Thanks regards, Daeseok Youn > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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 3/3] staging: dgap: introduce dgap_stop()
All three of these patches are good and a nice improvement. This one is a good bugfix. I have some notes for later, though below. On Wed, Oct 08, 2014 at 08:13:56PM +0900, Daeseok Youn wrote: > diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c > index 7c79fe6..00f34b5 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); These kinds of forward declarations are annoying. The whole file needs to be re-arranged so that we don't have to deal with them. > @@ -561,6 +563,21 @@ failed_class: > return rc; > } > > +static void dgap_stop(void) > +{ > + ulong lock_flags; This is non-standard. Traditionally it would be: unsigned long flags; regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/3] staging: dgap: introduce dgap_stop()
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 --- 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..00f34b5 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(_driver); if (rc) - goto err_cleanup; + goto err_stop; rc = dgap_create_driver_sysfiles(_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(_driver); +err_stop: + dgap_stop(); return rc; } @@ -561,6 +563,21 @@ failed_class: return rc; } +static void dgap_stop(void) +{ + ulong lock_flags; + + spin_lock_irqsave(_poll_lock, lock_flags); + dgap_poll_stop = 1; + spin_unlock_irqrestore(_poll_lock, lock_flags); + + del_timer_sync(_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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/3] staging: dgap: introduce dgap_stop()
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.y...@gmail.com --- 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..00f34b5 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) +{ + ulong 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 majord...@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 3/3] staging: dgap: introduce dgap_stop()
All three of these patches are good and a nice improvement. This one is a good bugfix. I have some notes for later, though below. On Wed, Oct 08, 2014 at 08:13:56PM +0900, Daeseok Youn wrote: diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c index 7c79fe6..00f34b5 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); These kinds of forward declarations are annoying. The whole file needs to be re-arranged so that we don't have to deal with them. @@ -561,6 +563,21 @@ failed_class: return rc; } +static void dgap_stop(void) +{ + ulong lock_flags; This is non-standard. Traditionally it would be: unsigned long flags; regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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 3/3] staging: dgap: introduce dgap_stop()
Hi, Dan 2014-10-08 20:37 GMT+09:00 Dan Carpenter dan.carpen...@oracle.com: All three of these patches are good and a nice improvement. This one is a good bugfix. I have some notes for later, though below. On Wed, Oct 08, 2014 at 08:13:56PM +0900, Daeseok Youn wrote: diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c index 7c79fe6..00f34b5 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); These kinds of forward declarations are annoying. The whole file needs to be re-arranged so that we don't have to deal with them. OK. I will try to re-arrange and remove forward declarations. @@ -561,6 +563,21 @@ failed_class: return rc; } +static void dgap_stop(void) +{ + ulong lock_flags; OK. I will send this patch again after changing ulong to unsigned long. This is non-standard. Traditionally it would be: unsigned long flags; regards, dan carpenter Thanks regards, Daeseok Youn -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/