Re: [PATCH 54/54] staging: wilc1000: wilc_msgqueue.c : remove the goto ERRORHANDER

[Tony Cho <tony.cho@atmel.com>]

On 2015년 10월 13일 23:08, Mike Rapoport wrote:
> On Tue, Oct 13, 2015 at 08:02:12PM +0900, Tony Cho wrote:
>> From: Leo Kim <leo.kim@atmel.com>
>>
>> This patch removes goto ERRORHANDER and the result variable in wilc_mq_send.
>> Then, the error type is directly returned. If normal operation, freeing memory
>> is not needed in this function.
>>
>> Signed-off-by: Leo Kim <leo.kim@atmel.com>
>> Signed-off-by: Tony Cho <tony.cho@atmel.com>
>> ---
>>   drivers/staging/wilc1000/wilc_msgqueue.c | 20 +++++---------------
>>   1 file changed, 5 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/staging/wilc1000/wilc_msgqueue.c b/drivers/staging/wilc1000/wilc_msgqueue.c
>> index b13809a..60539aa 100644
>> --- a/drivers/staging/wilc1000/wilc_msgqueue.c
>> +++ b/drivers/staging/wilc1000/wilc_msgqueue.c
>> @@ -56,20 +56,17 @@ int wilc_mq_destroy(WILC_MsgQueueHandle *pHandle)
>>   int wilc_mq_send(WILC_MsgQueueHandle *pHandle,
>>   			     const void *pvSendBuffer, u32 u32SendBufferSize)
>>   {
>> -	int result = 0;
>>   	unsigned long flags;
>>   	Message *pstrMessage = NULL;
>>   
>>   	if ((!pHandle) || (u32SendBufferSize == 0) || (!pvSendBuffer)) {
>>   		PRINT_ER("pHandle or pvSendBuffer is null\n");
>> -		result = -EFAULT;
>> -		goto ERRORHANDLER;
>> +		return -EFAULT;
>>   	}
>>   
>>   	if (pHandle->bExiting) {
>>   		PRINT_ER("pHandle fail\n");
>> -		result = -EFAULT;
>> -		goto ERRORHANDLER;
>> +		return -EFAULT;
>>   	}
>>   
>>   	spin_lock_irqsave(&pHandle->strCriticalSection, flags);
>> @@ -83,8 +80,8 @@ int wilc_mq_send(WILC_MsgQueueHandle *pHandle,
>>   	pstrMessage->pvBuffer = kmemdup(pvSendBuffer, u32SendBufferSize,
>>   					GFP_ATOMIC);
>>   	if (!pstrMessage->pvBuffer) {
>> -		result = -ENOMEM;
>> -		goto ERRORHANDLER;
>> +		kfree(pstrMessage);
>> +		return -ENOMEM;
> It seems that the error path returns from the function while still
> holding spinlock and semaphore. It would be better to rework allocation
> errors handling so that the locks will be released, rather than just
> remove goto...

Thanks for your review. I am going to send v2 for it.

Tony.

>>   	}
>>   
>>   	/* add it to the message queue */
>> @@ -103,14 +100,7 @@ int wilc_mq_send(WILC_MsgQueueHandle *pHandle,
>>   
>>   	up(&pHandle->hSem);
>>   
>> -ERRORHANDLER:
>> -	/* error occured, free any allocations */
>> -	if (pstrMessage) {
>> -		kfree(pstrMessage->pvBuffer);
>> -		kfree(pstrMessage);
>> -	}
>> -
>> -	return result;
>> +	return 0;
>>   }
>>   
>>   /*!
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> devel mailing list
>> devel@linuxdriverproject.org
>> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel