From: Leo Kim <[email protected]>
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 <[email protected]>
Signed-off-by: Tony Cho <[email protected]>
---
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);
- 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);
+ 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
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 <[email protected]>
> >>
> >>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 <[email protected]>
> >>Signed-off-by: Tony Cho <[email protected]>
> >>---
> >>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
> >>[email protected]
> >>http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
>
On Wed, Oct 14, 2015 at 02:55:43PM +0900, Tony Cho wrote:
> From: Leo Kim <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Tony Cho <[email protected]>
> ---
> 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.
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.
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
> [email protected]
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Tony Cho <[email protected]>
>> ---
>> 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?
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
>> [email protected]
>> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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 <[email protected]>
> >>
> >>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 <[email protected]>
> >>Signed-off-by: Tony Cho <[email protected]>
> >>---
> >>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
On 2015년 10월 17일 13:28, Greg KH wrote:
> 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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> Signed-off-by: Tony Cho <[email protected]>
>>>> ---
>>>> 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.
ok, I will do that,
Thanks,
Tony.
>
> thanks,
>
> greg k-h