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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
> >>
> >>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@xxxxxxxxx>
> >>Signed-off-by: Tony Cho <tony.cho@xxxxxxxxx>
> >>---
> >>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.

thanks,

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




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux