Skip to contentSkip to conversation

staging: dgap: implement error handling in dgap_tty_register()

PATCH Superseded Daeseok Youn 4 participants 15 messages Apr 25, 2014 – May 17, 2014 2 messages not in the archive Staging

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Daeseok Youn

author
#0 lore raw

PATCH 1/1 staging: dgap: implement error handling in dgap_tty_register()

- alloc_tty_driver() is deprecated so it is changed to tty_alloc_driver()
- Pointers which are allocated by alloc_tty_driver() and kzalloc() can be NULL so it need to check NULL for them.
- If one of those is failed, it need to add proper handler for avoiding memory leak.

Signed-off-by Daeseok Youn <[email protected]>
---
 drivers/staging/dgap/dgap.c |   49 +++++++++++++++++++++++++++++++++++--------
 1 files changed, 40 insertions(+), 9 deletions(-)
drivers/staging/dgap/dgap.c +40 -9
index 3c9278a..5c8823a 100644
@@ -875,7 +875,11 @@ static int dgap_firmware_load(struct pci_dev *pdev, int card_type)
 		return -EINVAL;
 	}
 
-	dgap_tty_register(brd);
+	ret = dgap_tty_register(brd);
+	if (ret) {
+		pr_err("dgap: failed to register tty driver\n");
+		return ret;
+	}
 	dgap_finalize_board_init(brd);
 
 	if (fw_info[card_type].bios_name) {

@@ -1200,7 +1204,9 @@ static int dgap_tty_register(struct board_t *brd)
 {
 	int rc = 0;
 
-	brd->SerialDriver = alloc_tty_driver(MAXPORTS);
+	brd->SerialDriver = tty_alloc_driver(MAXPORTS, 0);
+	if (IS_ERR(brd->SerialDriver))
+		return PTR_ERR(brd->SerialDriver);
 
 	snprintf(brd->SerialName, MAXTTYNAMELEN, "tty_dgap_%d_", brd->boardnum);
 	brd->SerialDriver->name = brd->SerialName;

@@ -1218,8 +1224,10 @@ static int dgap_tty_register(struct board_t *brd)
 	/* The kernel wants space to store pointers to tty_structs */
 	brd->SerialDriver->ttys =
 		kzalloc(MAXPORTS * sizeof(struct tty_struct *), GFP_KERNEL);
-	if (!brd->SerialDriver->ttys)
-		return -ENOMEM;
+	if (!brd->SerialDriver->ttys) {
+		rc = -ENOMEM;
+		goto free_serial_drv;
+	}
 
 	/*
 	 * Entry points for driver.  Called by the kernel from

@@ -1232,7 +1240,11 @@ static int dgap_tty_register(struct board_t *brd)
 	 * again, separately so we don't get the LD confused about what major
 	 * we are when we get into the dgap_tty_open() routine.
 	 */
-	brd->PrintDriver = alloc_tty_driver(MAXPORTS);
+	brd->PrintDriver = tty_alloc_driver(MAXPORTS, 0);
+	if (IS_ERR(brd->PrintDriver)) {
+		rc = PTR_ERR(brd->PrintDriver);
+		goto free_serial_ttys;
+	}
 
 	snprintf(brd->PrintName, MAXTTYNAMELEN, "pr_dgap_%d_", brd->boardnum);
 	brd->PrintDriver->name = brd->PrintName;

@@ -1250,8 +1262,10 @@ static int dgap_tty_register(struct board_t *brd)
 	/* The kernel wants space to store pointers to tty_structs */
 	brd->PrintDriver->ttys =
 		kzalloc(MAXPORTS * sizeof(struct tty_struct *), GFP_KERNEL);
-	if (!brd->PrintDriver->ttys)
-		return -ENOMEM;
+	if (!brd->PrintDriver->ttys) {
+		rc = -ENOMEM;
+		goto free_print_drv;
+	}
 
 	/*
 	 * Entry points for driver.  Called by the kernel from

@@ -1263,7 +1277,8 @@ static int dgap_tty_register(struct board_t *brd)
 		/* Register tty devices */
 		rc = tty_register_driver(brd->SerialDriver);
 		if (rc < 0)
-			return rc;
+			goto free_print_ttys;
+
 		brd->dgap_Major_Serial_Registered = TRUE;
 		dgap_BoardsByMajor[brd->SerialDriver->major] = brd;
 		brd->dgap_Serial_Major = brd->SerialDriver->major;

@@ -1273,13 +1288,29 @@ static int dgap_tty_register(struct board_t *brd)
 		/* Register Transparent Print devices */
 		rc = tty_register_driver(brd->PrintDriver);
 		if (rc < 0)
-			return rc;
+			goto unregister_serial_drv;
+
 		brd->dgap_Major_TransparentPrint_Registered = TRUE;
 		dgap_BoardsByMajor[brd->PrintDriver->major] = brd;
 		brd->dgap_TransparentPrint_Major = brd->PrintDriver->major;
 	}
 
 	return rc;
+
+unregister_serial_drv:
+	tty_unregister_driver(brd->SerialDriver);
+free_print_ttys:
+	kfree(brd->PrintDriver->ttys);
+	brd->PrintDriver->ttys = NULL;
+free_print_drv:
+	put_tty_driver(brd->PrintDriver);
+free_serial_ttys:
+	kfree(brd->SerialDriver->ttys);
+	brd->SerialDriver->ttys = NULL;
+free_serial_drv:
+	put_tty_driver(brd->SerialDriver);
+
+	return rc;
 }
 
 /*
Signature · 1 line
1.7.4.4
#1 lore raw

Mark, maybe you should add yourself to the MAINTAINERS entry for this driver?

Quoted 16 lines · Daeseok Youn
drivers/staging/dgap/dgap.c +3 -2
@@ -1263,7 +1277,8 @@ static int dgap_tty_register(struct board_t *brd)
 		/* Register tty devices */
 		rc = tty_register_driver(brd->SerialDriver);
 		if (rc < 0)
-			return rc;
+			goto free_print_ttys;
+
 		brd->dgap_Major_Serial_Registered = TRUE;
 		dgap_BoardsByMajor[brd->SerialDriver->major] = brd;
 		brd->dgap_Serial_Major = brd->SerialDriver->major;

@@ -1273,13 +1288,29 @@ static int dgap_tty_register(struct board_t *brd)
 		/* Register Transparent Print devices */
 		rc = tty_register_driver(brd->PrintDriver);
 		if (rc < 0)
-			return rc;
+			goto unregister_serial_drv;

drivers/staging/dgap/dgap.c static int dgap_tty_register(struct board_t *brd) · line 1301

Quoted 10 lines
drivers/staging/dgap/dgap.c +4 -0
+
 		brd->dgap_Major_TransparentPrint_Registered = TRUE;
 		dgap_BoardsByMajor[brd->PrintDriver->major] = brd;
 		brd->dgap_TransparentPrint_Major = brd->PrintDriver->major;
 	}
 
 	return rc;
+
+unregister_serial_drv:
+	tty_unregister_driver(brd->SerialDriver);

We only register the ->SerialDriver if someone else hasn't registered it first? So this should be:

	if (we_were_the_ones_who_registered_the_serial_driver)
		tty_unregister_driver(brd->SerialDriver);

I haven't followed looked at this. Who else is registering the serial driver? You have looked at this, what do you think? Or Mark.

regards,
dan carpenter

author
#2 lore raw

Hi, Dan.

2014-04-25 18:26 GMT+09:00 Dan Carpenter <[email protected]>:

Quoted 34 lines

Mark, maybe you should add yourself to the MAINTAINERS entry for this driver?

Quoted 16 lines · Daeseok Youn
drivers/staging/dgap/dgap.c +3 -2
@@ -1263,7 +1277,8 @@ static int dgap_tty_register(struct board_t *brd)
              /* Register tty devices */
              rc = tty_register_driver(brd->SerialDriver);
              if (rc < 0)
-                     return rc;
+                     goto free_print_ttys;
+
              brd->dgap_Major_Serial_Registered = TRUE;
              dgap_BoardsByMajor[brd->SerialDriver->major] = brd;
              brd->dgap_Serial_Major = brd->SerialDriver->major;

@@ -1273,13 +1288,29 @@ static int dgap_tty_register(struct board_t *brd)
              /* Register Transparent Print devices */
              rc = tty_register_driver(brd->PrintDriver);
              if (rc < 0)
-                     return rc;
+                     goto unregister_serial_drv;

drivers/staging/dgap/dgap.c static int dgap_tty_register(struct board_t *brd) · line 1301

Quoted 10 lines
drivers/staging/dgap/dgap.c +4 -0
+
              brd->dgap_Major_TransparentPrint_Registered = TRUE;
              dgap_BoardsByMajor[brd->PrintDriver->major] = brd;
              brd->dgap_TransparentPrint_Major = brd->PrintDriver->major;
      }
 
      return rc;
+
+unregister_serial_drv:
+     tty_unregister_driver(brd->SerialDriver);

We only register the ->SerialDriver if someone else hasn't registered it first? So this should be:

Quoted 5 lines
        if (we_were_the_ones_who_registered_the_serial_driver)
                tty_unregister_driver(brd->SerialDriver);

I haven't followed looked at this. Who else is registering the serial driver? You have looked at this, what do you think? Or Mark.

I think brd struct is from dgap_Board array as global static variable when this function is
called. So brd->dgap_Major_Serial_Registered is always "false". If dgap_NumBoards is less than MAXBOARDS, brd->SerialDriver should be registered.

I'm not sure..

Thanks.

Regards,
Daeseok Youn.

Quoted 4 lines

regards,
dan carpenter

#4 lore raw
In reply to a message not in the archive
Quoted 6 lines · DaeSeok Youn

Hi, Dan.

2014-04-25 18:26 GMT+09:00 Dan Carpenter <[email protected]>:

Quoted 3 lines

Mark, maybe you should add yourself to the MAINTAINERS entry for this driver?

I'll look into this. I am clueless on what that would actually mean.

Quoted 31 lines
Quoted 31 lines · Daeseok Youn
Quoted 16 lines
drivers/staging/dgap/dgap.c +3 -2
@@ -1263,7 +1277,8 @@ static int dgap_tty_register(struct board_t *brd)
              /* Register tty devices */
              rc = tty_register_driver(brd->SerialDriver);
              if (rc < 0)
-                     return rc;
+                     goto free_print_ttys;
+
              brd->dgap_Major_Serial_Registered = TRUE;
              dgap_BoardsByMajor[brd->SerialDriver->major] = brd;
              brd->dgap_Serial_Major = brd->SerialDriver->major;

@@ -1273,13 +1288,29 @@ static int dgap_tty_register(struct board_t *brd)
              /* Register Transparent Print devices */
              rc = tty_register_driver(brd->PrintDriver);
              if (rc < 0)
-                     return rc;
+                     goto unregister_serial_drv;

drivers/staging/dgap/dgap.c static int dgap_tty_register(struct board_t *brd) · line 1301

Quoted 10 lines
drivers/staging/dgap/dgap.c +4 -0
+
              brd->dgap_Major_TransparentPrint_Registered = TRUE;
              dgap_BoardsByMajor[brd->PrintDriver->major] = brd;
              brd->dgap_TransparentPrint_Major = brd->PrintDriver->major;
      }
 
      return rc;
+
+unregister_serial_drv:
+     tty_unregister_driver(brd->SerialDriver);

We only register the ->SerialDriver if someone else hasn't registered it first? So this should be:

Quoted 6 lines
Quoted 5 lines
        if (we_were_the_ones_who_registered_the_serial_driver)
                tty_unregister_driver(brd->SerialDriver);

I haven't followed looked at this. Who else is registering the serial driver? You have looked at this, what do you think? Or Mark.

registering the brd->XxxxxDriver is only done when a board is detected and only during the firmware_load process. If we fail to tty_register_driver do we _need_ to tty_unregister_driver? Isn't that like freeing after an alloc failure?

Quoted 8 lines

I think brd struct is from dgap_Board array as global static variable when this function is
called. So brd->dgap_Major_Serial_Registered is always "false". If dgap_NumBoards is less than MAXBOARDS, brd->SerialDriver should be registered.

I'm not sure..

I don't see any check for (dgap_NumBoards < MAXBOARDS), which I think I probably should, but I do see we are calling dgap_tty_register, which can fail, without actually checking the return value. Also, yes, dgap_Major_Xxxx_Registered seems to be always "false" until registered, and it looks like dgap_Major_Xxxxx_Registered flags could be removed because the only places we can unregister is at module_cleanup or "after" it is already registered.

What is the driver _supposed_ to do if we fail something on the second or later board? Is the driver supposed to cleanup and exit or are we supposed to stay loaded for the board/boards that are usable?

Regards
mark

#5 lore raw
Quoted 10 lines · Mark Hounschell
Quoted 6 lines · DaeSeok Youn

Hi, Dan.

2014-04-25 18:26 GMT+09:00 Dan Carpenter <[email protected]>:

Quoted 3 lines

Mark, maybe you should add yourself to the MAINTAINERS entry for this driver?

I'll look into this. I am clueless on what that would actually mean.

Just add your name with Lidza in the MAINTAINERS file so that people will CC you on all the patches.

DIGI EPCA PCI PRODUCTS

M:      Lidza Louina <[email protected]>
L:      [email protected]
S:      Maintained
F:      drivers/staging/dgap/

You don't have to do it if you don't want to, but you seem to be working on this driver and I'm going to refer questions to you either way. :P

Quoted 34 lines
Quoted 34 lines
Quoted 34 lines · Daeseok Youn
Quoted 16 lines
drivers/staging/dgap/dgap.c +3 -2
@@ -1263,7 +1277,8 @@ static int dgap_tty_register(struct board_t *brd)
              /* Register tty devices */
              rc = tty_register_driver(brd->SerialDriver);
              if (rc < 0)
-                     return rc;
+                     goto free_print_ttys;
+
              brd->dgap_Major_Serial_Registered = TRUE;
              dgap_BoardsByMajor[brd->SerialDriver->major] = brd;
              brd->dgap_Serial_Major = brd->SerialDriver->major;

@@ -1273,13 +1288,29 @@ static int dgap_tty_register(struct board_t *brd)
              /* Register Transparent Print devices */
              rc = tty_register_driver(brd->PrintDriver);
              if (rc < 0)
-                     return rc;
+                     goto unregister_serial_drv;

drivers/staging/dgap/dgap.c static int dgap_tty_register(struct board_t *brd) · line 1301

Quoted 10 lines
drivers/staging/dgap/dgap.c +4 -0
+
              brd->dgap_Major_TransparentPrint_Registered = TRUE;
              dgap_BoardsByMajor[brd->PrintDriver->major] = brd;
              brd->dgap_TransparentPrint_Major = brd->PrintDriver->major;
      }
 
      return rc;
+
+unregister_serial_drv:
+     tty_unregister_driver(brd->SerialDriver);

We only register the ->SerialDriver if someone else hasn't registered it first? So this should be:

        if (we_were_the_ones_who_registered_the_serial_driver)
                tty_unregister_driver(brd->SerialDriver);
Quoted 8 lines
Quoted 3 lines
Quoted 2 lines

I haven't followed looked at this. Who else is registering the serial driver? You have looked at this, what do you think? Or Mark.

registering the brd->XxxxxDriver is only done when a board is detected and only during the firmware_load process. If we fail to tty_register_driver do we _need_ to tty_unregister_driver? Isn't that like freeing after an alloc failure?

The allocation is conditional so the free should be conditional. If we didn't allocate it, then we shouldn't free it.

It wouldn't have even been a question except I'm not sure the allocation is *really* conditional because brd->dgap_Major_Serial_Registered might always be "false" like you guys seem to be saying.

Quoted 18 lines
Quoted 9 lines

I think brd struct is from dgap_Board array as global static variable when this function is
called. So brd->dgap_Major_Serial_Registered is always "false". If dgap_NumBoards is less than MAXBOARDS, brd->SerialDriver should be registered.

I'm not sure..

I don't see any check for (dgap_NumBoards < MAXBOARDS), which I think I probably should, but I do see we are calling dgap_tty_register, which can fail, without actually checking the return value. Also, yes, dgap_Major_Xxxx_Registered seems to be always "false" until registered, and it looks like dgap_Major_Xxxxx_Registered flags could be removed because the only places we can unregister is at module_cleanup or "after" it is already registered.

Quoted 3 lines

What is the driver _supposed_ to do if we fail something on the second or later board? Is the driver supposed to cleanup and exit or are we supposed to stay loaded for the board/boards that are usable?

Stay loaded.

regards,
dan carpenter

#7 lore raw
In reply to a message not in the archive
Quoted 92 lines · Dan Carpenter
Quoted 10 lines · Mark Hounschell
Quoted 6 lines · DaeSeok Youn

Hi, Dan.

2014-04-25 18:26 GMT+09:00 Dan Carpenter <[email protected]>:

Quoted 3 lines

Mark, maybe you should add yourself to the MAINTAINERS entry for this driver?

I'll look into this. I am clueless on what that would actually mean.

Just add your name with Lidza in the MAINTAINERS file so that people will CC you on all the patches.

DIGI EPCA PCI PRODUCTS

M:      Lidza Louina <[email protected]>
L:      [email protected]
S:      Maintained
F:      drivers/staging/dgap/

You don't have to do it if you don't want to, but you seem to be working on this driver and I'm going to refer questions to you either way. :P

Quoted 34 lines
Quoted 34 lines
Quoted 34 lines · Daeseok Youn
Quoted 16 lines
drivers/staging/dgap/dgap.c +3 -2
@@ -1263,7 +1277,8 @@ static int dgap_tty_register(struct board_t *brd)
              /* Register tty devices */
              rc = tty_register_driver(brd->SerialDriver);
              if (rc < 0)
-                     return rc;
+                     goto free_print_ttys;
+
              brd->dgap_Major_Serial_Registered = TRUE;
              dgap_BoardsByMajor[brd->SerialDriver->major] = brd;
              brd->dgap_Serial_Major = brd->SerialDriver->major;

@@ -1273,13 +1288,29 @@ static int dgap_tty_register(struct board_t *brd)
              /* Register Transparent Print devices */
              rc = tty_register_driver(brd->PrintDriver);
              if (rc < 0)
-                     return rc;
+                     goto unregister_serial_drv;

drivers/staging/dgap/dgap.c static int dgap_tty_register(struct board_t *brd) · line 1301

Quoted 10 lines
drivers/staging/dgap/dgap.c +4 -0
+
              brd->dgap_Major_TransparentPrint_Registered = TRUE;
              dgap_BoardsByMajor[brd->PrintDriver->major] = brd;
              brd->dgap_TransparentPrint_Major = brd->PrintDriver->major;
      }
 
      return rc;
+
+unregister_serial_drv:
+     tty_unregister_driver(brd->SerialDriver);

We only register the ->SerialDriver if someone else hasn't registered it first? So this should be:

        if (we_were_the_ones_who_registered_the_serial_driver)
                tty_unregister_driver(brd->SerialDriver);
Quoted 8 lines
Quoted 3 lines
Quoted 2 lines

I haven't followed looked at this. Who else is registering the serial driver? You have looked at this, what do you think? Or Mark.

registering the brd->XxxxxDriver is only done when a board is detected and only during the firmware_load process. If we fail to tty_register_driver do we _need_ to tty_unregister_driver? Isn't that like freeing after an alloc failure?

The allocation is conditional so the free should be conditional. If we didn't allocate it, then we shouldn't free it.

It wouldn't have even been a question except I'm not sure the allocation is *really* conditional because brd->dgap_Major_Serial_Registered might always be "false" like you guys seem to be saying.

Quoted 18 lines
Quoted 9 lines

I think brd struct is from dgap_Board array as global static variable when this function is
called. So brd->dgap_Major_Serial_Registered is always "false". If dgap_NumBoards is less than MAXBOARDS, brd->SerialDriver should be registered.

I'm not sure..

I don't see any check for (dgap_NumBoards < MAXBOARDS), which I think I probably should, but I do see we are calling dgap_tty_register, which can fail, without actually checking the return value. Also, yes, dgap_Major_Xxxx_Registered seems to be always "false" until registered, and it looks like dgap_Major_Xxxxx_Registered flags could be removed because the only places we can unregister is at module_cleanup or "after" it is already registered.

Quoted 6 lines
Quoted 3 lines

What is the driver _supposed_ to do if we fail something on the second or later board? Is the driver supposed to cleanup and exit or are we supposed to stay loaded for the board/boards that are usable?

Stay loaded.

Then these tests on brd->dgap_Major_Serial_Registered need to stay in there. If I have 3 boards and the second fails in some way, if I rmmod the driver they will protect from unregistering a never registered one. At least in the unregister code path. There is probably no need for them in the register code path. I'll work up a patch for this.

I need to look at the (dgap_NumBoards < MAXBOARDS) thing too.

Mark

author
#8 lore raw

Hi,

please check below my comments.

2014-04-25 23:41 GMT+09:00 Mark Hounschell <[email protected]>:

Quoted 97 lines · Dan Carpenter
Quoted 96 lines · Mark Hounschell
Quoted 10 lines · DaeSeok Youn
Quoted 6 lines

Hi, Dan.

2014-04-25 18:26 GMT+09:00 Dan Carpenter <[email protected]>:

Quoted 3 lines

Mark, maybe you should add yourself to the MAINTAINERS entry for this driver?

I'll look into this. I am clueless on what that would actually mean.

Just add your name with Lidza in the MAINTAINERS file so that people will CC you on all the patches.

DIGI EPCA PCI PRODUCTS

M:      Lidza Louina <[email protected]>
L:      [email protected]
S:      Maintained
F:      drivers/staging/dgap/

You don't have to do it if you don't want to, but you seem to be working on this driver and I'm going to refer questions to you either way. :P

Quoted 34 lines
Quoted 34 lines
Quoted 34 lines · Daeseok Youn
Quoted 16 lines
drivers/staging/dgap/dgap.c +3 -2
@@ -1263,7 +1277,8 @@ static int dgap_tty_register(struct board_t *brd)
              /* Register tty devices */
              rc = tty_register_driver(brd->SerialDriver);
              if (rc < 0)
-                     return rc;
+                     goto free_print_ttys;
+
              brd->dgap_Major_Serial_Registered = TRUE;
              dgap_BoardsByMajor[brd->SerialDriver->major] = brd;
              brd->dgap_Serial_Major = brd->SerialDriver->major;

@@ -1273,13 +1288,29 @@ static int dgap_tty_register(struct board_t *brd)
              /* Register Transparent Print devices */
              rc = tty_register_driver(brd->PrintDriver);
              if (rc < 0)
-                     return rc;
+                     goto unregister_serial_drv;

drivers/staging/dgap/dgap.c static int dgap_tty_register(struct board_t *brd) · line 1301

Quoted 10 lines
drivers/staging/dgap/dgap.c +4 -0
+
              brd->dgap_Major_TransparentPrint_Registered = TRUE;
              dgap_BoardsByMajor[brd->PrintDriver->major] = brd;
              brd->dgap_TransparentPrint_Major = brd->PrintDriver->major;
      }
 
      return rc;
+
+unregister_serial_drv:
+     tty_unregister_driver(brd->SerialDriver);

We only register the ->SerialDriver if someone else hasn't registered it first? So this should be:

        if (we_were_the_ones_who_registered_the_serial_driver)
                tty_unregister_driver(brd->SerialDriver);
Quoted 8 lines
Quoted 3 lines
Quoted 2 lines

I haven't followed looked at this. Who else is registering the serial driver? You have looked at this, what do you think? Or Mark.

registering the brd->XxxxxDriver is only done when a board is detected and only during the firmware_load process. If we fail to tty_register_driver do we _need_ to tty_unregister_driver? Isn't that like freeing after an alloc failure?

The allocation is conditional so the free should be conditional. If we didn't allocate it, then we shouldn't free it.

It wouldn't have even been a question except I'm not sure the allocation is *really* conditional because brd->dgap_Major_Serial_Registered might always be "false" like you guys seem to be saying.

Quoted 21 lines
Quoted 9 lines

I think brd struct is from dgap_Board array as global static variable when this function is
called. So brd->dgap_Major_Serial_Registered is always "false". If dgap_NumBoards is less than MAXBOARDS, brd->SerialDriver should be registered.

I'm not sure..

I don't see any check for (dgap_NumBoards < MAXBOARDS), which I think I probably should, but I do see we are calling dgap_tty_register, which can fail, without actually checking the return value. Also, yes, dgap_Major_Xxxx_Registered seems to be always "false" until registered, and it looks like dgap_Major_Xxxxx_Registered flags could be removed because the only places we can unregister is at module_cleanup or "after" it is already registered.

What is the driver _supposed_ to do if we fail something on the second or later board? Is the driver supposed to cleanup and exit or are we supposed to stay loaded for the board/boards that are usable?

Quoted 8 lines
Quoted 2 lines

Stay loaded.

Then these tests on brd->dgap_Major_Serial_Registered need to stay in there. If I have 3 boards and the second fails in some way, if I rmmod the driver they will protect from unregistering a never registered one. At least in the unregister code path. There is probably no need for them in the register code path. I'll work up a patch for this.

Should I update my patch?

I think "if (!brd->dgap_Major_XXX_Registered)" line can be removed in this function, because if tty_register_driver() is failed just set "false" to "dgap_Major_XXX_Registered".
dgap_Major_XXX_Registered only is used in cleaup function, right? So code like below

               rc = tty_register_driver(brd->PrintDriver);
               if (rc < 0)
                     goto unregister_serial_drv;
               brd->dgap_Major_TransparentPrint_Registered = TRUE;
...
unregister_serial_drv;
               brd->dgap_Major_Serial_Registered = false;
                tty_unregister_driver(brd->SerialDriver);

Thanks.

Regards,
Daeseok Youn.

Quoted 6 lines

I need to look at the (dgap_NumBoards < MAXBOARDS) thing too.

Mark

#9 lore raw
Quoted 111 lines · DaeSeok Youn

Hi,

please check below my comments.

2014-04-25 23:41 GMT+09:00 Mark Hounschell <[email protected]>:

Quoted 97 lines · Dan Carpenter
Quoted 96 lines · Mark Hounschell
Quoted 10 lines · DaeSeok Youn
Quoted 6 lines

Hi, Dan.

2014-04-25 18:26 GMT+09:00 Dan Carpenter <[email protected]>:

Quoted 3 lines

Mark, maybe you should add yourself to the MAINTAINERS entry for this driver?

I'll look into this. I am clueless on what that would actually mean.

Just add your name with Lidza in the MAINTAINERS file so that people will CC you on all the patches.

DIGI EPCA PCI PRODUCTS

M:      Lidza Louina <[email protected]>
L:      [email protected]
S:      Maintained
F:      drivers/staging/dgap/

You don't have to do it if you don't want to, but you seem to be working on this driver and I'm going to refer questions to you either way. :P

Quoted 34 lines
Quoted 34 lines
Quoted 34 lines · Daeseok Youn
Quoted 16 lines
(quoted diff) +3 -2
@@ -1263,7 +1277,8 @@ static int dgap_tty_register(struct board_t *brd)
              /* Register tty devices */
              rc = tty_register_driver(brd->SerialDriver);
              if (rc < 0)
-                     return rc;
+                     goto free_print_ttys;
+
              brd->dgap_Major_Serial_Registered = TRUE;
              dgap_BoardsByMajor[brd->SerialDriver->major] = brd;
              brd->dgap_Serial_Major = brd->SerialDriver->major;

@@ -1273,13 +1288,29 @@ static int dgap_tty_register(struct board_t *brd)
              /* Register Transparent Print devices */
              rc = tty_register_driver(brd->PrintDriver);
              if (rc < 0)
-                     return rc;
+                     goto unregister_serial_drv;

Review static int dgap_tty_register(struct board_t *brd) · line 1301

Quoted 10 lines
(quoted diff) +4 -0
+
              brd->dgap_Major_TransparentPrint_Registered = TRUE;
              dgap_BoardsByMajor[brd->PrintDriver->major] = brd;
              brd->dgap_TransparentPrint_Major = brd->PrintDriver->major;
      }
 
      return rc;
+
+unregister_serial_drv:
+     tty_unregister_driver(brd->SerialDriver);

We only register the ->SerialDriver if someone else hasn't registered it first? So this should be:

        if (we_were_the_ones_who_registered_the_serial_driver)
                tty_unregister_driver(brd->SerialDriver);
Quoted 8 lines
Quoted 3 lines
Quoted 2 lines

I haven't followed looked at this. Who else is registering the serial driver? You have looked at this, what do you think? Or Mark.

registering the brd->XxxxxDriver is only done when a board is detected and only during the firmware_load process. If we fail to tty_register_driver do we _need_ to tty_unregister_driver? Isn't that like freeing after an alloc failure?

The allocation is conditional so the free should be conditional. If we didn't allocate it, then we shouldn't free it.

It wouldn't have even been a question except I'm not sure the allocation is *really* conditional because brd->dgap_Major_Serial_Registered might always be "false" like you guys seem to be saying.

Quoted 21 lines
Quoted 9 lines

I think brd struct is from dgap_Board array as global static variable when this function is
called. So brd->dgap_Major_Serial_Registered is always "false". If dgap_NumBoards is less than MAXBOARDS, brd->SerialDriver should be registered.

I'm not sure..

I don't see any check for (dgap_NumBoards < MAXBOARDS), which I think I probably should, but I do see we are calling dgap_tty_register, which can fail, without actually checking the return value. Also, yes, dgap_Major_Xxxx_Registered seems to be always "false" until registered, and it looks like dgap_Major_Xxxxx_Registered flags could be removed because the only places we can unregister is at module_cleanup or "after" it is already registered.

What is the driver _supposed_ to do if we fail something on the second or later board? Is the driver supposed to cleanup and exit or are we supposed to stay loaded for the board/boards that are usable?

Quoted 8 lines
Quoted 2 lines

Stay loaded.

Then these tests on brd->dgap_Major_Serial_Registered need to stay in there. If I have 3 boards and the second fails in some way, if I rmmod the driver they will protect from unregistering a never registered one. At least in the unregister code path. There is probably no need for them in the register code path. I'll work up a patch for this.

Quoted 5 lines

Should I update my patch?

I think "if (!brd->dgap_Major_XXX_Registered)" line can be removed in this function, because if tty_register_driver() is failed just set "false" to "dgap_Major_XXX_Registered".

Mark sent a patch to remove the check. Could you redo your patch based on his?

regards,
dan carpenter

author
#10 lore raw

OK. I'll make my patch based on Mark's patch.
Thanks.

Daeseok Youn.

2014-04-27 3:48 GMT+09:00 Dan Carpenter <[email protected]>:

Quoted 124 lines · DaeSeok Youn
Quoted 111 lines

Hi,

please check below my comments.

2014-04-25 23:41 GMT+09:00 Mark Hounschell <[email protected]>:

Quoted 97 lines · Dan Carpenter
Quoted 96 lines · Mark Hounschell
Quoted 10 lines · DaeSeok Youn
Quoted 6 lines

Hi, Dan.

2014-04-25 18:26 GMT+09:00 Dan Carpenter <[email protected]>:

Quoted 3 lines

Mark, maybe you should add yourself to the MAINTAINERS entry for this driver?

I'll look into this. I am clueless on what that would actually mean.

Just add your name with Lidza in the MAINTAINERS file so that people will CC you on all the patches.

DIGI EPCA PCI PRODUCTS

M:      Lidza Louina <[email protected]>
L:      [email protected]
S:      Maintained
F:      drivers/staging/dgap/

You don't have to do it if you don't want to, but you seem to be working on this driver and I'm going to refer questions to you either way. :P

Quoted 34 lines
Quoted 34 lines
Quoted 34 lines · Daeseok Youn
Quoted 16 lines
(quoted diff) +3 -2
@@ -1263,7 +1277,8 @@ static int dgap_tty_register(struct board_t *brd)
              /* Register tty devices */
              rc = tty_register_driver(brd->SerialDriver);
              if (rc < 0)
-                     return rc;
+                     goto free_print_ttys;
+
              brd->dgap_Major_Serial_Registered = TRUE;
              dgap_BoardsByMajor[brd->SerialDriver->major] = brd;
              brd->dgap_Serial_Major = brd->SerialDriver->major;

@@ -1273,13 +1288,29 @@ static int dgap_tty_register(struct board_t *brd)
              /* Register Transparent Print devices */
              rc = tty_register_driver(brd->PrintDriver);
              if (rc < 0)
-                     return rc;
+                     goto unregister_serial_drv;

Review static int dgap_tty_register(struct board_t *brd) · line 1301

Quoted 10 lines
(quoted diff) +4 -0
+
              brd->dgap_Major_TransparentPrint_Registered = TRUE;
              dgap_BoardsByMajor[brd->PrintDriver->major] = brd;
              brd->dgap_TransparentPrint_Major = brd->PrintDriver->major;
      }
 
      return rc;
+
+unregister_serial_drv:
+     tty_unregister_driver(brd->SerialDriver);

We only register the ->SerialDriver if someone else hasn't registered it first? So this should be:

        if (we_were_the_ones_who_registered_the_serial_driver)
                tty_unregister_driver(brd->SerialDriver);
Quoted 8 lines
Quoted 3 lines
Quoted 2 lines

I haven't followed looked at this. Who else is registering the serial driver? You have looked at this, what do you think? Or Mark.

registering the brd->XxxxxDriver is only done when a board is detected and only during the firmware_load process. If we fail to tty_register_driver do we _need_ to tty_unregister_driver? Isn't that like freeing after an alloc failure?

The allocation is conditional so the free should be conditional. If we didn't allocate it, then we shouldn't free it.

It wouldn't have even been a question except I'm not sure the allocation is *really* conditional because brd->dgap_Major_Serial_Registered might always be "false" like you guys seem to be saying.

Quoted 21 lines
Quoted 9 lines

I think brd struct is from dgap_Board array as global static variable when this function is
called. So brd->dgap_Major_Serial_Registered is always "false". If dgap_NumBoards is less than MAXBOARDS, brd->SerialDriver should be registered.

I'm not sure..

I don't see any check for (dgap_NumBoards < MAXBOARDS), which I think I probably should, but I do see we are calling dgap_tty_register, which can fail, without actually checking the return value. Also, yes, dgap_Major_Xxxx_Registered seems to be always "false" until registered, and it looks like dgap_Major_Xxxxx_Registered flags could be removed because the only places we can unregister is at module_cleanup or "after" it is already registered.

What is the driver _supposed_ to do if we fail something on the second or later board? Is the driver supposed to cleanup and exit or are we supposed to stay loaded for the board/boards that are usable?

Quoted 8 lines
Quoted 2 lines

Stay loaded.

Then these tests on brd->dgap_Major_Serial_Registered need to stay in there. If I have 3 boards and the second fails in some way, if I rmmod the driver they will protect from unregistering a never registered one. At least in the unregister code path. There is probably no need for them in the register code path. I'll work up a patch for this.

Quoted 5 lines

Should I update my patch?

I think "if (!brd->dgap_Major_XXX_Registered)" line can be removed in this function, because if tty_register_driver() is failed just set "false" to "dgap_Major_XXX_Registered".

Mark sent a patch to remove the check. Could you redo your patch based on his?

regards,
dan carpenter

author
#11 lore raw

Hi,

This patch haven't been updated over the past 3 weeks.
Will you check for me?

Regards,
Daeseok Youn.

2014-04-28 8:21 GMT+09:00 DaeSeok Youn <[email protected]>:

Quoted 130 lines

OK. I'll make my patch based on Mark's patch.
Thanks.

Daeseok Youn.

2014-04-27 3:48 GMT+09:00 Dan Carpenter <[email protected]>:

Quoted 124 lines · DaeSeok Youn
Quoted 111 lines

Hi,

please check below my comments.

2014-04-25 23:41 GMT+09:00 Mark Hounschell <[email protected]>:

Quoted 97 lines · Dan Carpenter
Quoted 96 lines · Mark Hounschell
Quoted 10 lines · DaeSeok Youn
Quoted 6 lines

Hi, Dan.

2014-04-25 18:26 GMT+09:00 Dan Carpenter <[email protected]>:

Quoted 3 lines

Mark, maybe you should add yourself to the MAINTAINERS entry for this driver?

I'll look into this. I am clueless on what that would actually mean.

Just add your name with Lidza in the MAINTAINERS file so that people will CC you on all the patches.

DIGI EPCA PCI PRODUCTS

M:      Lidza Louina <[email protected]>
L:      [email protected]
S:      Maintained
F:      drivers/staging/dgap/

You don't have to do it if you don't want to, but you seem to be working on this driver and I'm going to refer questions to you either way. :P

Quoted 34 lines
Quoted 34 lines
Quoted 34 lines · Daeseok Youn
Quoted 16 lines
(quoted diff) +3 -2
@@ -1263,7 +1277,8 @@ static int dgap_tty_register(struct board_t *brd)
              /* Register tty devices */
              rc = tty_register_driver(brd->SerialDriver);
              if (rc < 0)
-                     return rc;
+                     goto free_print_ttys;
+
              brd->dgap_Major_Serial_Registered = TRUE;
              dgap_BoardsByMajor[brd->SerialDriver->major] = brd;
              brd->dgap_Serial_Major = brd->SerialDriver->major;

@@ -1273,13 +1288,29 @@ static int dgap_tty_register(struct board_t *brd)
              /* Register Transparent Print devices */
              rc = tty_register_driver(brd->PrintDriver);
              if (rc < 0)
-                     return rc;
+                     goto unregister_serial_drv;

Review static int dgap_tty_register(struct board_t *brd) · line 1301

Quoted 10 lines
(quoted diff) +4 -0
+
              brd->dgap_Major_TransparentPrint_Registered = TRUE;
              dgap_BoardsByMajor[brd->PrintDriver->major] = brd;
              brd->dgap_TransparentPrint_Major = brd->PrintDriver->major;
      }
 
      return rc;
+
+unregister_serial_drv:
+     tty_unregister_driver(brd->SerialDriver);

We only register the ->SerialDriver if someone else hasn't registered it first? So this should be:

        if (we_were_the_ones_who_registered_the_serial_driver)
                tty_unregister_driver(brd->SerialDriver);
Quoted 8 lines
Quoted 3 lines
Quoted 2 lines

I haven't followed looked at this. Who else is registering the serial driver? You have looked at this, what do you think? Or Mark.

registering the brd->XxxxxDriver is only done when a board is detected and only during the firmware_load process. If we fail to tty_register_driver do we _need_ to tty_unregister_driver? Isn't that like freeing after an alloc failure?

The allocation is conditional so the free should be conditional. If we didn't allocate it, then we shouldn't free it.

It wouldn't have even been a question except I'm not sure the allocation is *really* conditional because brd->dgap_Major_Serial_Registered might always be "false" like you guys seem to be saying.

Quoted 21 lines
Quoted 9 lines

I think brd struct is from dgap_Board array as global static variable when this function is
called. So brd->dgap_Major_Serial_Registered is always "false". If dgap_NumBoards is less than MAXBOARDS, brd->SerialDriver should be registered.

I'm not sure..

I don't see any check for (dgap_NumBoards < MAXBOARDS), which I think I probably should, but I do see we are calling dgap_tty_register, which can fail, without actually checking the return value. Also, yes, dgap_Major_Xxxx_Registered seems to be always "false" until registered, and it looks like dgap_Major_Xxxxx_Registered flags could be removed because the only places we can unregister is at module_cleanup or "after" it is already registered.

What is the driver _supposed_ to do if we fail something on the second or later board? Is the driver supposed to cleanup and exit or are we supposed to stay loaded for the board/boards that are usable?

Quoted 8 lines
Quoted 2 lines

Stay loaded.

Then these tests on brd->dgap_Major_Serial_Registered need to stay in there. If I have 3 boards and the second fails in some way, if I rmmod the driver they will protect from unregistering a never registered one. At least in the unregister code path. There is probably no need for them in the register code path. I'll work up a patch for this.

Quoted 5 lines

Should I update my patch?

I think "if (!brd->dgap_Major_XXX_Registered)" line can be removed in this function, because if tty_register_driver() is failed just set "false" to "dgap_Major_XXX_Registered".

Mark sent a patch to remove the check. Could you redo your patch based on his?

regards,
dan carpenter

#12 lore raw
Quoted 4 lines · DaeSeok Youn

Hi,

This patch haven't been updated over the past 3 weeks.
Will you check for me?

Greg was trying to travel around the world in one month like Jackie Chan. http://www.imdb.com/title/tt0327437/ He is back to work since yesterday.

regards,
dan carpenter

author
#13 lore raw

2014-05-16 18:52 GMT+09:00 Dan Carpenter <[email protected]>:

Quoted 9 lines · DaeSeok Youn
Quoted 4 lines

Hi,

This patch haven't been updated over the past 3 weeks.
Will you check for me?

Greg was trying to travel around the world in one month like Jackie Chan. http://www.imdb.com/title/tt0327437/ He is back to work since yesterday.

Oh, good notice :-), but I'm not asking why my patch is not pushed. I knew this patch should be re-based on Mark's patch. You said that I waited his patch and it need to redo based on his.
And I checked staging-next branch and linux-next branch but I didn't found his patch.

He did already send that patch, let me know.

Thanks.
Daeseok Youn.

Quoted 4 lines

regards,
dan carpenter

maintainer
#14 lore raw
In reply to Dan Carpenter
Quoted 9 lines · Dan Carpenter
Quoted 4 lines · DaeSeok Youn

Hi,

This patch haven't been updated over the past 3 weeks.
Will you check for me?

Greg was trying to travel around the world in one month like Jackie Chan. http://www.imdb.com/title/tt0327437/ He is back to work since yesterday.

Well, I'm home for a day or so before I leave again for a week, so I'm having to prioritize things (patches or laundry) and dealing with a pet vet emergency, so my ability to get to cleanup patches is way down the list...

greg k-h

maintainer
#15 lore raw
In reply to Daeseok Youn
Quoted 11 lines · Daeseok Youn

- alloc_tty_driver() is deprecated so it is changed to tty_alloc_driver()
- Pointers which are allocated by alloc_tty_driver() and kzalloc() can be NULL so it need to check NULL for them.
- If one of those is failed, it need to add proper handler for avoiding memory leak.

Signed-off-by Daeseok Youn <[email protected]>
---
 drivers/staging/dgap/dgap.c |   49 +++++++++++++++++++++++++++++++++++--------
 1 files changed, 40 insertions(+), 9 deletions(-)

This doesn't apply at all to my tree anymore, please refresh it and resend.

thanks,

greg k-h

author
#16 lore raw

2014-05-17 8:09 GMT+09:00, Greg KH <[email protected]>:

Quoted 8 lines · Daeseok Youn
Quoted 7 lines

- alloc_tty_driver() is deprecated so it is changed to tty_alloc_driver()
- Pointers which are allocated by alloc_tty_driver() and kzalloc() can be NULL so it need to check NULL for them.
- If one of those is failed, it need to add proper handler for avoiding memory leak.

Quoted 9 lines
Quoted 5 lines
Signed-off-by Daeseok Youn <[email protected]>
---
 drivers/staging/dgap/dgap.c |   49
+++++++++++++++++++++++++++++++++++--------
 1 files changed, 40 insertions(+), 9 deletions(-)

This doesn't apply at all to my tree anymore, please refresh it and resend.

Yes. It need to rebase on staging-next branch. I will rebase and resend this. Thanks
Daeseok Youn.

Quoted 4 lines

thanks,

greg k-h