Re: [PATCH 2/4] staging: dgap: implement proper error handling in dgap_firmware_load()

[DaeSeok Youn]

Hi, Dan

2014-05-26 19:50 GMT+09:00 Dan Carpenter :
> On Mon, May 26, 2014 at 07:23:47PM +0900, Daeseok Youn wrote:
>> When dgap_tty_init() and dgap_tty_register_ports() are failed,
>> these are needed to free some memory properly.
>>
>> It can be handled by calling dgap_tty_uninit() and dgap_cleanup_board().
>> But tty's ports are not registered yet when these function are failed,
>> so brd->nasync set to zero.
>>
>> Signed-off-by: Daeseok Youn 
>> ---
>>  drivers/staging/dgap/dgap.c |   21 -
>>  1 files changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
>> index 60b7d70..db24f70 100644
>> --- a/drivers/staging/dgap/dgap.c
>> +++ b/drivers/staging/dgap/dgap.c
>> @@ -954,19 +954,30 @@ static int dgap_firmware_load(struct pci_dev *pdev, 
>> int card_type)
>>* Do tty device initialization.
>>*/
>>   ret = dgap_tty_init(brd);
>> - if (ret < 0) {
>> - dgap_tty_uninit(brd);
>> - return ret;
>> - }
>> + if (ret < 0)
>> + goto err_cleanup;
>>
>>   ret = dgap_tty_register_ports(brd);
>>   if (ret)
>> - return ret;
>> + goto err_cleanup;
>>
>>   brd->state = BOARD_READY;
>>   brd->dpastatus = BD_RUNNING;
>>
>>   return 0;
>> +
>> +err_cleanup:
>> + /*
>> +  * Clear nasync to zero for avoiding to call
>> +  * some destroyer for tty's ports which are not
>> +  * registered yet in dgap_tty_uninit().
>> +  */
>> + brd->nasync = 0;
>> +
>> + dgap_tty_uninit(brd);
>> + dgap_cleanup_board(brd);
>> + return ret;
>
> Wow.  This is nasty.  We shouldn't have to call dgap_tty_uninit() when
> the init failed.  Can't we clean this up instead of adding
> "brd->nasync = 0;" work arounds?
I think dgap_tty_uninit() should be called when the init failed. It
need to unregister
serial and printer driver and free them.
And I will try to clean this up without that workaround.
Thanks for review.

regards,
Daeseok Youn
>
> regards,
> dan carpenter
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/4] staging: dgap: implement proper error handling in dgap_firmware_load()

[Dan Carpenter]

On Mon, May 26, 2014 at 07:23:47PM +0900, Daeseok Youn wrote:
> When dgap_tty_init() and dgap_tty_register_ports() are failed,
> these are needed to free some memory properly.
> 
> It can be handled by calling dgap_tty_uninit() and dgap_cleanup_board().
> But tty's ports are not registered yet when these function are failed,
> so brd->nasync set to zero.
> 
> Signed-off-by: Daeseok Youn 
> ---
>  drivers/staging/dgap/dgap.c |   21 -
>  1 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
> index 60b7d70..db24f70 100644
> --- a/drivers/staging/dgap/dgap.c
> +++ b/drivers/staging/dgap/dgap.c
> @@ -954,19 +954,30 @@ static int dgap_firmware_load(struct pci_dev *pdev, int 
> card_type)
>* Do tty device initialization.
>*/
>   ret = dgap_tty_init(brd);
> - if (ret < 0) {
> - dgap_tty_uninit(brd);
> - return ret;
> - }
> + if (ret < 0)
> + goto err_cleanup;
>  
>   ret = dgap_tty_register_ports(brd);
>   if (ret)
> - return ret;
> + goto err_cleanup;
>  
>   brd->state = BOARD_READY;
>   brd->dpastatus = BD_RUNNING;
>  
>   return 0;
> +
> +err_cleanup:
> + /*
> +  * Clear nasync to zero for avoiding to call
> +  * some destroyer for tty's ports which are not
> +  * registered yet in dgap_tty_uninit().
> +  */
> + brd->nasync = 0;
> +
> + dgap_tty_uninit(brd);
> + dgap_cleanup_board(brd);
> + return ret;

Wow.  This is nasty.  We shouldn't have to call dgap_tty_uninit() when
the init failed.  Can't we clean this up instead of adding
"brd->nasync = 0;" work arounds?

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/4] staging: dgap: implement proper error handling in dgap_firmware_load()

[Daeseok Youn]

When dgap_tty_init() and dgap_tty_register_ports() are failed,
these are needed to free some memory properly.

It can be handled by calling dgap_tty_uninit() and dgap_cleanup_board().
But tty's ports are not registered yet when these function are failed,
so brd->nasync set to zero.

Signed-off-by: Daeseok Youn 
---
 drivers/staging/dgap/dgap.c |   21 -
 1 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index 60b7d70..db24f70 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -954,19 +954,30 @@ static int dgap_firmware_load(struct pci_dev *pdev, int 
card_type)
 * Do tty device initialization.
 */
ret = dgap_tty_init(brd);
-   if (ret < 0) {
-   dgap_tty_uninit(brd);
-   return ret;
-   }
+   if (ret < 0)
+   goto err_cleanup;
 
ret = dgap_tty_register_ports(brd);
if (ret)
-   return ret;
+   goto err_cleanup;
 
brd->state = BOARD_READY;
brd->dpastatus = BD_RUNNING;
 
return 0;
+
+err_cleanup:
+   /*
+* Clear nasync to zero for avoiding to call
+* some destroyer for tty's ports which are not
+* registered yet in dgap_tty_uninit().
+*/
+   brd->nasync = 0;
+
+   dgap_tty_uninit(brd);
+   dgap_cleanup_board(brd);
+   return ret;
+
 }
 
 /*
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/4] staging: dgap: implement proper error handling in dgap_firmware_load()

[Daeseok Youn]

When dgap_tty_init() and dgap_tty_register_ports() are failed,
these are needed to free some memory properly.

It can be handled by calling dgap_tty_uninit() and dgap_cleanup_board().
But tty's ports are not registered yet when these function are failed,
so brd-nasync set to zero.

Signed-off-by: Daeseok Youn daeseok.y...@gmail.com
---
 drivers/staging/dgap/dgap.c |   21 -
 1 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index 60b7d70..db24f70 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -954,19 +954,30 @@ static int dgap_firmware_load(struct pci_dev *pdev, int 
card_type)
 * Do tty device initialization.
 */
ret = dgap_tty_init(brd);
-   if (ret  0) {
-   dgap_tty_uninit(brd);
-   return ret;
-   }
+   if (ret  0)
+   goto err_cleanup;
 
ret = dgap_tty_register_ports(brd);
if (ret)
-   return ret;
+   goto err_cleanup;
 
brd-state = BOARD_READY;
brd-dpastatus = BD_RUNNING;
 
return 0;
+
+err_cleanup:
+   /*
+* Clear nasync to zero for avoiding to call
+* some destroyer for tty's ports which are not
+* registered yet in dgap_tty_uninit().
+*/
+   brd-nasync = 0;
+
+   dgap_tty_uninit(brd);
+   dgap_cleanup_board(brd);
+   return ret;
+
 }
 
 /*
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/4] staging: dgap: implement proper error handling in dgap_firmware_load()

[Dan Carpenter]

On Mon, May 26, 2014 at 07:23:47PM +0900, Daeseok Youn wrote:
 When dgap_tty_init() and dgap_tty_register_ports() are failed,
 these are needed to free some memory properly.
 
 It can be handled by calling dgap_tty_uninit() and dgap_cleanup_board().
 But tty's ports are not registered yet when these function are failed,
 so brd-nasync set to zero.
 
 Signed-off-by: Daeseok Youn daeseok.y...@gmail.com
 ---
  drivers/staging/dgap/dgap.c |   21 -
  1 files changed, 16 insertions(+), 5 deletions(-)
 
 diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
 index 60b7d70..db24f70 100644
 --- a/drivers/staging/dgap/dgap.c
 +++ b/drivers/staging/dgap/dgap.c
 @@ -954,19 +954,30 @@ static int dgap_firmware_load(struct pci_dev *pdev, int 
 card_type)
* Do tty device initialization.
*/
   ret = dgap_tty_init(brd);
 - if (ret  0) {
 - dgap_tty_uninit(brd);
 - return ret;
 - }
 + if (ret  0)
 + goto err_cleanup;
  
   ret = dgap_tty_register_ports(brd);
   if (ret)
 - return ret;
 + goto err_cleanup;
  
   brd-state = BOARD_READY;
   brd-dpastatus = BD_RUNNING;
  
   return 0;
 +
 +err_cleanup:
 + /*
 +  * Clear nasync to zero for avoiding to call
 +  * some destroyer for tty's ports which are not
 +  * registered yet in dgap_tty_uninit().
 +  */
 + brd-nasync = 0;
 +
 + dgap_tty_uninit(brd);
 + dgap_cleanup_board(brd);
 + return ret;

Wow.  This is nasty.  We shouldn't have to call dgap_tty_uninit() when
the init failed.  Can't we clean this up instead of adding
brd-nasync = 0; work arounds?

regards,
dan carpenter

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/4] staging: dgap: implement proper error handling in dgap_firmware_load()

[DaeSeok Youn]

Hi, Dan

2014-05-26 19:50 GMT+09:00 Dan Carpenter dan.carpen...@oracle.com:
 On Mon, May 26, 2014 at 07:23:47PM +0900, Daeseok Youn wrote:
 When dgap_tty_init() and dgap_tty_register_ports() are failed,
 these are needed to free some memory properly.

 It can be handled by calling dgap_tty_uninit() and dgap_cleanup_board().
 But tty's ports are not registered yet when these function are failed,
 so brd-nasync set to zero.

 Signed-off-by: Daeseok Youn daeseok.y...@gmail.com
 ---
  drivers/staging/dgap/dgap.c |   21 -
  1 files changed, 16 insertions(+), 5 deletions(-)

 diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
 index 60b7d70..db24f70 100644
 --- a/drivers/staging/dgap/dgap.c
 +++ b/drivers/staging/dgap/dgap.c
 @@ -954,19 +954,30 @@ static int dgap_firmware_load(struct pci_dev *pdev, 
 int card_type)
* Do tty device initialization.
*/
   ret = dgap_tty_init(brd);
 - if (ret  0) {
 - dgap_tty_uninit(brd);
 - return ret;
 - }
 + if (ret  0)
 + goto err_cleanup;

   ret = dgap_tty_register_ports(brd);
   if (ret)
 - return ret;
 + goto err_cleanup;

   brd-state = BOARD_READY;
   brd-dpastatus = BD_RUNNING;

   return 0;
 +
 +err_cleanup:
 + /*
 +  * Clear nasync to zero for avoiding to call
 +  * some destroyer for tty's ports which are not
 +  * registered yet in dgap_tty_uninit().
 +  */
 + brd-nasync = 0;
 +
 + dgap_tty_uninit(brd);
 + dgap_cleanup_board(brd);
 + return ret;

 Wow.  This is nasty.  We shouldn't have to call dgap_tty_uninit() when
 the init failed.  Can't we clean this up instead of adding
 brd-nasync = 0; work arounds?
I think dgap_tty_uninit() should be called when the init failed. It
need to unregister
serial and printer driver and free them.
And I will try to clean this up without that workaround.
Thanks for review.

regards,
Daeseok Youn

 regards,
 dan carpenter

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/