| Message ID | 1444734132-17858-9-git-send-email-tony.cho@atmel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
On Tue, Oct 13, 2015 at 08:02:12PM +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. > > Signed-off-by: Leo Kim <leo.kim@atmel.com> > Signed-off-by: Tony Cho <tony.cho@atmel.com> > --- > drivers/staging/wilc1000/wilc_msgqueue.c | 20 +++++--------------- > 1 file changed, 5 insertions(+), 15 deletions(-) > > diff --git a/drivers/staging/wilc1000/wilc_msgqueue.c b/drivers/staging/wilc1000/wilc_msgqueue.c > index b13809a..60539aa 100644 > --- a/drivers/staging/wilc1000/wilc_msgqueue.c > +++ b/drivers/staging/wilc1000/wilc_msgqueue.c > @@ -56,20 +56,17 @@ 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); > @@ -83,8 +80,8 @@ int wilc_mq_send(WILC_MsgQueueHandle *pHandle, > pstrMessage->pvBuffer = kmemdup(pvSendBuffer, u32SendBufferSize, > GFP_ATOMIC); > if (!pstrMessage->pvBuffer) { > - result = -ENOMEM; > - goto ERRORHANDLER; > + kfree(pstrMessage); > + return -ENOMEM; It seems that the error path returns from the function while still holding spinlock and semaphore. It would be better to rework allocation errors handling so that the locks will be released, rather than just remove goto... > } > > /* add it to the message queue */ > @@ -103,14 +100,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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015? 10? 13? 23:08, Mike Rapoport wrote: > On Tue, Oct 13, 2015 at 08:02:12PM +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. >> >> Signed-off-by: Leo Kim <leo.kim@atmel.com> >> Signed-off-by: Tony Cho <tony.cho@atmel.com> >> --- >> drivers/staging/wilc1000/wilc_msgqueue.c | 20 +++++--------------- >> 1 file changed, 5 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/staging/wilc1000/wilc_msgqueue.c b/drivers/staging/wilc1000/wilc_msgqueue.c >> index b13809a..60539aa 100644 >> --- a/drivers/staging/wilc1000/wilc_msgqueue.c >> +++ b/drivers/staging/wilc1000/wilc_msgqueue.c >> @@ -56,20 +56,17 @@ 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); >> @@ -83,8 +80,8 @@ int wilc_mq_send(WILC_MsgQueueHandle *pHandle, >> pstrMessage->pvBuffer = kmemdup(pvSendBuffer, u32SendBufferSize, >> GFP_ATOMIC); >> if (!pstrMessage->pvBuffer) { >> - result = -ENOMEM; >> - goto ERRORHANDLER; >> + kfree(pstrMessage); >> + return -ENOMEM; > It seems that the error path returns from the function while still > holding spinlock and semaphore. It would be better to rework allocation > errors handling so that the locks will be released, rather than just > remove goto... Thanks for your review. I am going to send v2 for it. Tony. >> } >> >> /* add it to the message queue */ >> @@ -103,14 +100,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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/staging/wilc1000/wilc_msgqueue.c b/drivers/staging/wilc1000/wilc_msgqueue.c index b13809a..60539aa 100644 --- a/drivers/staging/wilc1000/wilc_msgqueue.c +++ b/drivers/staging/wilc1000/wilc_msgqueue.c @@ -56,20 +56,17 @@ 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); @@ -83,8 +80,8 @@ int wilc_mq_send(WILC_MsgQueueHandle *pHandle, pstrMessage->pvBuffer = kmemdup(pvSendBuffer, u32SendBufferSize, GFP_ATOMIC); if (!pstrMessage->pvBuffer) { - result = -ENOMEM; - goto ERRORHANDLER; + kfree(pstrMessage); + return -ENOMEM; } /* add it to the message queue */ @@ -103,14 +100,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; } /*!