[PATCH] staging: dgap: implement error handling in dgap_tty_register()

[PATCH] staging: dgap: implement error handling in dgap_tty_register()

[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 <daeseok.youn@gmail.com>
---
 drivers/staging/dgap/dgap.c |   49 +++++++++++++++++++++++++++++++++++--------
 1 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index 3c9278a..5c8823a 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -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;
 }
 
 /*
-- 
1.7.4.4

[PATCH] staging: dgap: implement error handling in dgap_tty_register()

[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 <daeseok.youn@gmail.com>
---
 drivers/staging/dgap/dgap.c |   49 +++++++++++++++++++++++++++++++++++--------
 1 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index 3c9278a..5c8823a 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -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;
 }
 
 /*
-- 
1.7.4.4

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

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[Dan Carpenter]

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

On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
> @@ -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);

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

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[Dan Carpenter]

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

On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
> @@ -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);

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

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

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[DaeSeok Youn]

Hi, Dan.

2014-04-25 18:26 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> Mark, maybe you should add yourself to the MAINTAINERS entry for this
> driver?
>
> On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
>> @@ -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);
>
> 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.

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.

>
> regards,
> dan carpenter
>

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[DaeSeok Youn]

Hi, Dan.

2014-04-25 18:26 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> Mark, maybe you should add yourself to the MAINTAINERS entry for this
> driver?
>
> On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
>> @@ -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);
>
> 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.

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.

>
> regards,
> dan carpenter
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[Mark Hounschell]

On 04/25/2014 07:02 AM, DaeSeok Youn wrote:
> Hi, Dan.
> 
> 2014-04-25 18:26 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
>> 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.

>> On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
>>> @@ -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);
>>
>> 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.
> 

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?

> 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

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[Mark Hounschell]

On 04/25/2014 07:02 AM, DaeSeok Youn wrote:
> Hi, Dan.
> 
> 2014-04-25 18:26 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
>> 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.

>> On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
>>> @@ -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);
>>
>> 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.
> 

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?

> 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


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

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[Dan Carpenter]

On Fri, Apr 25, 2014 at 08:29:41AM -0400, Mark Hounschell wrote:
> On 04/25/2014 07:02 AM, DaeSeok Youn wrote:
> > Hi, Dan.
> > 
> > 2014-04-25 18:26 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> >> 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 <lidza.louina@gmail.com>
L:      driverdev-devel@linuxdriverproject.org
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

> >> On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
> >>> @@ -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);
> >>
> >> 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.
> > 
> 
> 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.

> 
> > 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?

Stay loaded.

regards,
dan carpenter

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[Dan Carpenter]

On Fri, Apr 25, 2014 at 08:29:41AM -0400, Mark Hounschell wrote:
> On 04/25/2014 07:02 AM, DaeSeok Youn wrote:
> > Hi, Dan.
> > 
> > 2014-04-25 18:26 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> >> 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 <lidza.louina@gmail.com>
L:      driverdev-devel@linuxdriverproject.org
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

> >> On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
> >>> @@ -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);
> >>
> >> 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.
> > 
> 
> 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.

> 
> > 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?

Stay loaded.

regards,
dan carpenter

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

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[Mark Hounschell]

On 04/25/2014 08:59 AM, Dan Carpenter wrote:
> On Fri, Apr 25, 2014 at 08:29:41AM -0400, Mark Hounschell wrote:
>> On 04/25/2014 07:02 AM, DaeSeok Youn wrote:
>>> Hi, Dan.
>>>
>>> 2014-04-25 18:26 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
>>>> 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 <lidza.louina@gmail.com>
> L:      driverdev-devel@linuxdriverproject.org
> 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
> 
>>>> On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
>>>>> @@ -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);
>>>>
>>>> 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.
>>>
>>
>> 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.
> 
>>
>>> 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?
> 
> 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

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[Mark Hounschell]

On 04/25/2014 08:59 AM, Dan Carpenter wrote:
> On Fri, Apr 25, 2014 at 08:29:41AM -0400, Mark Hounschell wrote:
>> On 04/25/2014 07:02 AM, DaeSeok Youn wrote:
>>> Hi, Dan.
>>>
>>> 2014-04-25 18:26 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
>>>> 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 <lidza.louina@gmail.com>
> L:      driverdev-devel@linuxdriverproject.org
> 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
> 
>>>> On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
>>>>> @@ -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);
>>>>
>>>> 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.
>>>
>>
>> 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.
> 
>>
>>> 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?
> 
> 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


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

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[DaeSeok Youn]

Hi,

please check below my comments.

2014-04-25 23:41 GMT+09:00 Mark Hounschell <markh@compro.net>:
> On 04/25/2014 08:59 AM, Dan Carpenter wrote:
>> On Fri, Apr 25, 2014 at 08:29:41AM -0400, Mark Hounschell wrote:
>>> On 04/25/2014 07:02 AM, DaeSeok Youn wrote:
>>>> Hi, Dan.
>>>>
>>>> 2014-04-25 18:26 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
>>>>> 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 <lidza.louina@gmail.com>
>> L:      driverdev-devel@linuxdriverproject.org
>> 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
>>
>>>>> On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
>>>>>> @@ -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);
>>>>>
>>>>> 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.
>>>>
>>>
>>> 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.
>>
>>>
>>>> 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?
>>
>> 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.

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

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[DaeSeok Youn]

Hi,

please check below my comments.

2014-04-25 23:41 GMT+09:00 Mark Hounschell <markh@compro.net>:
> On 04/25/2014 08:59 AM, Dan Carpenter wrote:
>> On Fri, Apr 25, 2014 at 08:29:41AM -0400, Mark Hounschell wrote:
>>> On 04/25/2014 07:02 AM, DaeSeok Youn wrote:
>>>> Hi, Dan.
>>>>
>>>> 2014-04-25 18:26 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
>>>>> 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 <lidza.louina@gmail.com>
>> L:      driverdev-devel@linuxdriverproject.org
>> 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
>>
>>>>> On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
>>>>>> @@ -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);
>>>>>
>>>>> 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.
>>>>
>>>
>>> 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.
>>
>>>
>>>> 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?
>>
>> 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.

>
> I need to look at the (dgap_NumBoards <  MAXBOARDS) thing too.
>
> Mark
>
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[Dan Carpenter]

On Sat, Apr 26, 2014 at 11:39:38AM +0900, DaeSeok Youn wrote:
> Hi,
> 
> please check below my comments.
> 
> 2014-04-25 23:41 GMT+09:00 Mark Hounschell <markh@compro.net>:
> > On 04/25/2014 08:59 AM, Dan Carpenter wrote:
> >> On Fri, Apr 25, 2014 at 08:29:41AM -0400, Mark Hounschell wrote:
> >>> On 04/25/2014 07:02 AM, DaeSeok Youn wrote:
> >>>> Hi, Dan.
> >>>>
> >>>> 2014-04-25 18:26 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> >>>>> 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 <lidza.louina@gmail.com>
> >> L:      driverdev-devel@linuxdriverproject.org
> >> 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
> >>
> >>>>> On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
> >>>>>> @@ -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);
> >>>>>
> >>>>> 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.
> >>>>
> >>>
> >>> 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.
> >>
> >>>
> >>>> 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?
> >>
> >> 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".

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

regards,
dan carpenter

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[Dan Carpenter]

On Sat, Apr 26, 2014 at 11:39:38AM +0900, DaeSeok Youn wrote:
> Hi,
> 
> please check below my comments.
> 
> 2014-04-25 23:41 GMT+09:00 Mark Hounschell <markh@compro.net>:
> > On 04/25/2014 08:59 AM, Dan Carpenter wrote:
> >> On Fri, Apr 25, 2014 at 08:29:41AM -0400, Mark Hounschell wrote:
> >>> On 04/25/2014 07:02 AM, DaeSeok Youn wrote:
> >>>> Hi, Dan.
> >>>>
> >>>> 2014-04-25 18:26 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> >>>>> 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 <lidza.louina@gmail.com>
> >> L:      driverdev-devel@linuxdriverproject.org
> >> 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
> >>
> >>>>> On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
> >>>>>> @@ -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);
> >>>>>
> >>>>> 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.
> >>>>
> >>>
> >>> 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.
> >>
> >>>
> >>>> 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?
> >>
> >> 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".

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

regards,
dan carpenter

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

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[DaeSeok Youn]

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

Daeseok Youn.

2014-04-27 3:48 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> On Sat, Apr 26, 2014 at 11:39:38AM +0900, DaeSeok Youn wrote:
>> Hi,
>>
>> please check below my comments.
>>
>> 2014-04-25 23:41 GMT+09:00 Mark Hounschell <markh@compro.net>:
>> > On 04/25/2014 08:59 AM, Dan Carpenter wrote:
>> >> On Fri, Apr 25, 2014 at 08:29:41AM -0400, Mark Hounschell wrote:
>> >>> On 04/25/2014 07:02 AM, DaeSeok Youn wrote:
>> >>>> Hi, Dan.
>> >>>>
>> >>>> 2014-04-25 18:26 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
>> >>>>> 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 <lidza.louina@gmail.com>
>> >> L:      driverdev-devel@linuxdriverproject.org
>> >> 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
>> >>
>> >>>>> On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
>> >>>>>> @@ -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);
>> >>>>>
>> >>>>> 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.
>> >>>>
>> >>>
>> >>> 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.
>> >>
>> >>>
>> >>>> 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?
>> >>
>> >> 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".
>
> Mark sent a patch to remove the check.  Could you redo your patch based
> on his?
>
> regards,
> dan carpenter
>

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[DaeSeok Youn]

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

Daeseok Youn.

2014-04-27 3:48 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> On Sat, Apr 26, 2014 at 11:39:38AM +0900, DaeSeok Youn wrote:
>> Hi,
>>
>> please check below my comments.
>>
>> 2014-04-25 23:41 GMT+09:00 Mark Hounschell <markh@compro.net>:
>> > On 04/25/2014 08:59 AM, Dan Carpenter wrote:
>> >> On Fri, Apr 25, 2014 at 08:29:41AM -0400, Mark Hounschell wrote:
>> >>> On 04/25/2014 07:02 AM, DaeSeok Youn wrote:
>> >>>> Hi, Dan.
>> >>>>
>> >>>> 2014-04-25 18:26 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
>> >>>>> 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 <lidza.louina@gmail.com>
>> >> L:      driverdev-devel@linuxdriverproject.org
>> >> 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
>> >>
>> >>>>> On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
>> >>>>>> @@ -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);
>> >>>>>
>> >>>>> 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.
>> >>>>
>> >>>
>> >>> 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.
>> >>
>> >>>
>> >>>> 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?
>> >>
>> >> 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".
>
> Mark sent a patch to remove the check.  Could you redo your patch based
> on his?
>
> regards,
> dan carpenter
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[DaeSeok Youn]

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 <daeseok.youn@gmail.com>:
> OK. I'll make my patch based on Mark's patch.
> Thanks.
>
> Daeseok Youn.
>
> 2014-04-27 3:48 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
>> On Sat, Apr 26, 2014 at 11:39:38AM +0900, DaeSeok Youn wrote:
>>> Hi,
>>>
>>> please check below my comments.
>>>
>>> 2014-04-25 23:41 GMT+09:00 Mark Hounschell <markh@compro.net>:
>>> > On 04/25/2014 08:59 AM, Dan Carpenter wrote:
>>> >> On Fri, Apr 25, 2014 at 08:29:41AM -0400, Mark Hounschell wrote:
>>> >>> On 04/25/2014 07:02 AM, DaeSeok Youn wrote:
>>> >>>> Hi, Dan.
>>> >>>>
>>> >>>> 2014-04-25 18:26 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
>>> >>>>> 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 <lidza.louina@gmail.com>
>>> >> L:      driverdev-devel@linuxdriverproject.org
>>> >> 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
>>> >>
>>> >>>>> On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
>>> >>>>>> @@ -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);
>>> >>>>>
>>> >>>>> 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.
>>> >>>>
>>> >>>
>>> >>> 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.
>>> >>
>>> >>>
>>> >>>> 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?
>>> >>
>>> >> 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".
>>
>> Mark sent a patch to remove the check.  Could you redo your patch based
>> on his?
>>
>> regards,
>> dan carpenter
>>

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[DaeSeok Youn]

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 <daeseok.youn@gmail.com>:
> OK. I'll make my patch based on Mark's patch.
> Thanks.
>
> Daeseok Youn.
>
> 2014-04-27 3:48 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
>> On Sat, Apr 26, 2014 at 11:39:38AM +0900, DaeSeok Youn wrote:
>>> Hi,
>>>
>>> please check below my comments.
>>>
>>> 2014-04-25 23:41 GMT+09:00 Mark Hounschell <markh@compro.net>:
>>> > On 04/25/2014 08:59 AM, Dan Carpenter wrote:
>>> >> On Fri, Apr 25, 2014 at 08:29:41AM -0400, Mark Hounschell wrote:
>>> >>> On 04/25/2014 07:02 AM, DaeSeok Youn wrote:
>>> >>>> Hi, Dan.
>>> >>>>
>>> >>>> 2014-04-25 18:26 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
>>> >>>>> 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 <lidza.louina@gmail.com>
>>> >> L:      driverdev-devel@linuxdriverproject.org
>>> >> 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
>>> >>
>>> >>>>> On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
>>> >>>>>> @@ -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);
>>> >>>>>
>>> >>>>> 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.
>>> >>>>
>>> >>>
>>> >>> 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.
>>> >>
>>> >>>
>>> >>>> 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?
>>> >>
>>> >> 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".
>>
>> Mark sent a patch to remove the check.  Could you redo your patch based
>> on his?
>>
>> regards,
>> dan carpenter
>>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[Dan Carpenter]

On Fri, May 16, 2014 at 06:40:28PM +0900, DaeSeok Youn wrote:
> 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

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[Dan Carpenter]

On Fri, May 16, 2014 at 06:40:28PM +0900, DaeSeok Youn wrote:
> 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

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

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[DaeSeok Youn]

2014-05-16 18:52 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> On Fri, May 16, 2014 at 06:40:28PM +0900, DaeSeok Youn wrote:
>> 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.
>
> regards,
> dan carpenter
>

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[DaeSeok Youn]

2014-05-16 18:52 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> On Fri, May 16, 2014 at 06:40:28PM +0900, DaeSeok Youn wrote:
>> 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.
>
> regards,
> dan carpenter
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[Greg KH]

On Fri, May 16, 2014 at 12:52:47PM +0300, Dan Carpenter wrote:
> On Fri, May 16, 2014 at 06:40:28PM +0900, DaeSeok Youn wrote:
> > 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

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[Greg KH]

On Fri, May 16, 2014 at 12:52:47PM +0300, Dan Carpenter wrote:
> On Fri, May 16, 2014 at 06:40:28PM +0900, DaeSeok Youn wrote:
> > 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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[Greg KH]

On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
> - 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 <daeseok.youn@gmail.com>
> ---
>  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

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[Greg KH]

On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
> - 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 <daeseok.youn@gmail.com>
> ---
>  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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[DaeSeok Youn]

2014-05-17 8:09 GMT+09:00, Greg KH <gregkh@linuxfoundation.org>:
> On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
>> - 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 <daeseok.youn@gmail.com>
>> ---
>>  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.

> thanks,
>
> greg k-h
>

Re: [PATCH] staging: dgap: implement error handling in dgap_tty_register()

[DaeSeok Youn]

2014-05-17 8:09 GMT+09:00, Greg KH <gregkh@linuxfoundation.org>:
> On Fri, Apr 25, 2014 at 04:04:59PM +0900, Daeseok Youn wrote:
>> - 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 <daeseok.youn@gmail.com>
>> ---
>>  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.

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