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(-)
> >> 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@xxxxxxxxxxxxxxxxxxxxxx
> >>http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
>
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
