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

[Mike Rapoport <mike.rapoport@gmail.com>]

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(-)
> >>  	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.

If the semaphore should not be released if an error happens, you can
ignore my comments. 
 
> >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?
> Thanks for your review,
> Tony.
> 
> >
> >mem_free:
> >	kfree(pstrMessage);
> >release_lock:	
> >	spin_unlock_irqrestore(&pHandle->strCriticalSection, flags);
> >release_sem:
> >	up(&pHandle->sem);
> >out:
> >	return result;
> >
> >>+		kfree(pstrMessage);
> >>+		return -ENOMEM;
> >>  	}
> >>  	/* add it to the message queue */
> >>@@ -103,14 +106,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
>