[PATCH 1/4] staging: dgap: fix error path

[PATCH 1/4] staging: dgap: fix error path

[Sudip Mukherjee]

The code in dgap_stop() is almost a duplicate of the code that will be
executed on pci_unregister_driver(). So the error code was stopping and
unregistering everything twice.

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

we had a discussion about the init and error path few months back when
another patch was sent. I am not able to find that discussion in
lkml.org

 drivers/staging/dgap/dgap.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index 26b0446..3a783bf 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -7133,8 +7133,10 @@ static int dgap_init_module(void)
 		return rc;
 
 	rc = pci_register_driver(&dgap_driver);
-	if (rc)
-		goto err_stop;
+	if (rc) {
+		dgap_stop();
+		return rc;
+	}
 
 	rc = dgap_create_driver_sysfiles(&dgap_driver);
 	if (rc)
@@ -7146,9 +7148,6 @@ static int dgap_init_module(void)
 
 err_unregister:
 	pci_unregister_driver(&dgap_driver);
-err_stop:
-	dgap_stop();
-
 	return rc;
 }
 
-- 
1.8.1.2

[PATCH 2/4] staging: dgap: rearrange function

[Sudip Mukherjee]

Relocate the function dgap_stop() so that in a later patch we can remove
the duplicate codes between dgap_stop() and dgap_remove_one().

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/staging/dgap/dgap.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index 3a783bf..8956228 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -7004,6 +7004,21 @@ static void dgap_cleanup_board(struct board_t *brd)
 	kfree(brd);
 }
 
+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 void dgap_remove_one(struct pci_dev *dev)
 {
 	unsigned int i;
@@ -7096,21 +7111,6 @@ 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");
-}
-
 /************************************************************************
  *
  * Driver load/unload functions
-- 
1.8.1.2

[PATCH 3/4] staging: dgap: new arguments to dgap_stop

[Sudip Mukherjee]

In a later patch we will remove the duplicate codes. But the code also
needs to execute dgap_remove_driver_sysfiles() if it is being called
from dgap_remove_one() but if being called fron the error path of the
dgap_init_module() then the sysfiles should not be removed.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/staging/dgap/dgap.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index 8956228..4f07a0c 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -7004,7 +7004,7 @@ static void dgap_cleanup_board(struct board_t *brd)
 	kfree(brd);
 }
 
-static void dgap_stop(void)
+static void dgap_stop(bool removesys, struct pci_driver *drv)
 {
 	unsigned long lock_flags;
 
@@ -7013,6 +7013,8 @@ static void dgap_stop(void)
 	spin_unlock_irqrestore(&dgap_poll_lock, lock_flags);
 
 	del_timer_sync(&dgap_poll_timer);
+	if (removesys)
+		dgap_remove_driver_sysfiles(drv);
 
 	device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 0));
 	class_destroy(dgap_class);
@@ -7134,7 +7136,7 @@ static int dgap_init_module(void)
 
 	rc = pci_register_driver(&dgap_driver);
 	if (rc) {
-		dgap_stop();
+		dgap_stop(false, NULL);
 		return rc;
 	}
 
-- 
1.8.1.2

[PATCH 4/4] staging: dgap: remove duplicate code

[Sudip Mukherjee]

Remove the duplicate code of dgap_remove_one() and dgap_stop().

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/staging/dgap/dgap.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index 4f07a0c..b344e03 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -7024,22 +7024,9 @@ static void dgap_stop(bool removesys, struct pci_driver *drv)
 static void dgap_remove_one(struct pci_dev *dev)
 {
 	unsigned int i;
-	ulong lock_flags;
 	struct pci_driver *drv = to_pci_driver(dev->dev.driver);
 
-	spin_lock_irqsave(&dgap_poll_lock, lock_flags);
-	dgap_poll_stop = 1;
-	spin_unlock_irqrestore(&dgap_poll_lock, lock_flags);
-
-	/* Turn off poller right away. */
-	del_timer_sync(&dgap_poll_timer);
-
-	dgap_remove_driver_sysfiles(drv);
-
-	device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 0));
-	class_destroy(dgap_class);
-	unregister_chrdev(DIGI_DGAP_MAJOR, "dgap");
-
+	dgap_stop(true, drv);
 	for (i = 0; i < dgap_numboards; ++i) {
 		dgap_remove_ports_sysfiles(dgap_board[i]);
 		dgap_cleanup_tty(dgap_board[i]);
-- 
1.8.1.2

Re: [PATCH 1/4] staging: dgap: fix error path

[Dan Carpenter]

I don't think I like these at all.  remove_one has always been buggy in
that it removes everything.  We should fix it to only remove one instead
of formalizing the currect terrible behavior.

regards,
dan carpenter

Re: [PATCH 1/4] staging: dgap: fix error path

[Sudip Mukherjee]

On Fri, Jul 17, 2015 at 12:30:03PM +0300, Dan Carpenter wrote:
> I don't think I like these at all.  remove_one has always been buggy in
> that it removes everything.  We should fix it to only remove one instead
> of formalizing the currect terrible behavior.
Its already applied.
I thought after the full series the code became a little better than the
original one.
Now dgap_stop() is being called from dgap_remove_one(). How do you suggest
it should be?

regards
sudip

Re: [PATCH 1/4] staging: dgap: fix error path

[Dan Carpenter]

On Fri, Jul 17, 2015 at 03:21:28PM +0530, Sudip Mukherjee wrote:
> On Fri, Jul 17, 2015 at 12:30:03PM +0300, Dan Carpenter wrote:
> > I don't think I like these at all.  remove_one has always been buggy in
> > that it removes everything.  We should fix it to only remove one instead
> > of formalizing the currect terrible behavior.
> Its already applied.
> I thought after the full series the code became a little better than the
> original one.

It looks nicer but it's wrong.


> Now dgap_stop() is being called from dgap_remove_one(). How do you suggest
> it should be?

dgap_remove_one() should mirror dgap_init_one().  dgap_stop() should
only be called from dgap_cleanup_module().  dgap_cleanup_module() should
mirror dgap_init_module().

regards,
dan carpenter

Re: [PATCH 1/4] staging: dgap: fix error path

[Sudip Mukherjee]

On Fri, Jul 17, 2015 at 01:05:55PM +0300, Dan Carpenter wrote:
> On Fri, Jul 17, 2015 at 03:21:28PM +0530, Sudip Mukherjee wrote:
> > On Fri, Jul 17, 2015 at 12:30:03PM +0300, Dan Carpenter wrote:
> > > I don't think I like these at all.  remove_one has always been buggy in
> > > that it removes everything.  We should fix it to only remove one instead
> > > of formalizing the currect terrible behavior.
> > Its already applied.
> > I thought after the full series the code became a little better than the
> > original one.
> 
> It looks nicer but it's wrong.
> 
> 
> > Now dgap_stop() is being called from dgap_remove_one(). How do you suggest
> > it should be?
> 
> dgap_remove_one() should mirror dgap_init_one().  dgap_stop() should
> only be called from dgap_cleanup_module().  dgap_cleanup_module() should
> mirror dgap_init_module().
But if dgap_stop() is only called from dgap_cleanup_module() then what
will happen if the pci device is suddenly removed?
Currently if the pci device is removed then the remove callback will be
executed and it will stop and unregister everything properly.

regards
sudip

Re: [PATCH 1/4] staging: dgap: fix error path

[Dan Carpenter]

On Fri, Jul 17, 2015 at 03:57:21PM +0530, Sudip Mukherjee wrote:
> On Fri, Jul 17, 2015 at 01:05:55PM +0300, Dan Carpenter wrote:
> > On Fri, Jul 17, 2015 at 03:21:28PM +0530, Sudip Mukherjee wrote:
> > > On Fri, Jul 17, 2015 at 12:30:03PM +0300, Dan Carpenter wrote:
> > > > I don't think I like these at all.  remove_one has always been buggy in
> > > > that it removes everything.  We should fix it to only remove one instead
> > > > of formalizing the currect terrible behavior.
> > > Its already applied.
> > > I thought after the full series the code became a little better than the
> > > original one.
> > 
> > It looks nicer but it's wrong.
> > 
> > 
> > > Now dgap_stop() is being called from dgap_remove_one(). How do you suggest
> > > it should be?
> > 
> > dgap_remove_one() should mirror dgap_init_one().  dgap_stop() should
> > only be called from dgap_cleanup_module().  dgap_cleanup_module() should
> > mirror dgap_init_module().
> But if dgap_stop() is only called from dgap_cleanup_module() then what
> will happen if the pci device is suddenly removed?
> Currently if the pci device is removed then the remove callback will be
> executed and it will stop and unregister everything properly.

It shouldn't be unregistering everything when one thing is removed, it
should only unregister the stuff that is removed.

regards,
dan carpenter