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

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

On 2015년 10월 17일 13:28, Greg KH wrote:
> On Wed, Oct 14, 2015 at 05:52:55PM +0900, Tony Cho wrote:
>>
>> On 2015년 10월 14일 17:32, Mike Rapoport wrote:
>>> On Wed, Oct 14, 2015 at 02:55:43PM +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.
>>>> If an error occurs, returns an error after releasing the spin lock.
>>>>
>>>> Signed-off-by: Leo Kim <leo.kim@atmel.com>
>>>> Signed-off-by: Tony Cho <tony.cho@atmel.com>
>>>> ---
>>>> V2: add releasing spinlock before returnig an error when an error happens.
>>>> ---
>>>>   drivers/staging/wilc1000/wilc_msgqueue.c | 28 ++++++++++++----------------
>>>>   1 file changed, 12 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/wilc1000/wilc_msgqueue.c b/drivers/staging/wilc1000/wilc_msgqueue.c
>>>> index b13809a..4dfd476 100644
>>>> --- a/drivers/staging/wilc1000/wilc_msgqueue.c
>>>> +++ b/drivers/staging/wilc1000/wilc_msgqueue.c
>>>> @@ -56,35 +56,38 @@ 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);
>>>>   	/* construct a new message */
>>>>   	pstrMessage = kmalloc(sizeof(Message), GFP_ATOMIC);
>>> You can make this allocation outisde the spinlock, then there won't be
>>> need to release it if the allocation fails.
>>>
>>>> -	if (!pstrMessage)
>>>> +
>>>> +	if (!pstrMessage) {
>>>> +		spin_unlock_irqrestore(&pHandle->strCriticalSection, flags);
>>>>   		return -ENOMEM;
>>>> +	}
>>>> +
>>>>   	pstrMessage->u32Length = u32SendBufferSize;
>>>>   	pstrMessage->pstrNext = NULL;
>>>>   	pstrMessage->pvBuffer = kmemdup(pvSendBuffer, u32SendBufferSize,
>>>>   					GFP_ATOMIC);
>>>> +
>>>>   	if (!pstrMessage->pvBuffer) {
>>>> -		result = -ENOMEM;
>>>> -		goto ERRORHANDLER;
>>>> +		spin_unlock_irqrestore(&pHandle->strCriticalSection, flags);
>>> You are still holding pHandle->hSem here. Moreover, all error paths
>>> return while still holding pHandle->hSem.
>> Can you explain to me what you mean for holding hsem here? Basically, this function cannot release
>>
>> the semaphore (of course, this synchronization mechanism will be changed, anyway) if errors happen.
>>
>> The thread will do his work when it is released without unexpected situations.
>>
>>> I'd suggest, that instead of trying to return immediately, you'd better
>>> move error handling code to the end of the function and use goto and
>>> several labels to do appropriate cleanups depending on when the error
>>> was caught. E.g.
>> I don't want to use goto statement as possible as I can. Do you
>> concern semaphore uses in this function?
> You should use a goto, it will make the error unwinding much easier and
> is a very common pattern in Linux kernel code.  It will save you many
> lines in this function, please do it that way instead.

ok, I will do that,

Thanks,

Tony.

>
> thanks,
>
> greg k-h