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....@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....@atmel.com> > >>Signed-off-by: Tony Cho <tony....@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 > >>de...@linuxdriverproject.org > >>http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html