On Sun, Sep 27, 2015 at 07:43:18PM +0530, Chandra S Gorentla wrote:
> The message queue is replaced with standard Linux linked list.  A check for
> return value of receive method is added.
> 
> Signed-off-by: Chandra S Gorentla <csgoren...@gmail.com>
> ---
>  drivers/staging/wilc1000/host_interface.c |  7 +++-
>  drivers/staging/wilc1000/wilc_msgqueue.c  | 62 
> ++++++++++++++-----------------
>  drivers/staging/wilc1000/wilc_platform.h  |  5 ++-
>  3 files changed, 36 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/host_interface.c 
> b/drivers/staging/wilc1000/host_interface.c
> index 62f4a8a..8d0776f 100644
> --- a/drivers/staging/wilc1000/host_interface.c
> +++ b/drivers/staging/wilc1000/host_interface.c
> @@ -4304,11 +4304,16 @@ static int hostIFthread(void *pvArg)
>       u32 u32Ret;
>       struct host_if_msg msg;
>       tstrWILC_WFIDrv *pstrWFIDrv;
> +     int recv_ret;

Name it "ret".

        int ret;

>  
>       memset(&msg, 0, sizeof(struct host_if_msg));
>  
>       while (1) {
> -             wilc_mq_recv(&gMsgQHostIF, &msg, sizeof(struct host_if_msg), 
> &u32Ret);
> +             recv_ret = wilc_mq_recv(&gMsgQHostIF, &msg,
> +                                     sizeof(struct host_if_msg), &u32Ret);
> +             if (recv_ret)
> +                     continue;

This looks like a forever loop.

> +
>               pstrWFIDrv = (tstrWILC_WFIDrv *)msg.drvHandler;
>               if (msg.id == HOST_IF_MSG_EXIT) {
>                       PRINT_D(GENERIC_DBG, "THREAD: Exiting HostIfThread\n");
> diff --git a/drivers/staging/wilc1000/wilc_msgqueue.c 
> b/drivers/staging/wilc1000/wilc_msgqueue.c
> index 869736a..67617d5 100644
> --- a/drivers/staging/wilc1000/wilc_msgqueue.c
> +++ b/drivers/staging/wilc1000/wilc_msgqueue.c
> @@ -12,9 +12,15 @@
>   */
>  int wilc_mq_create(WILC_MsgQueueHandle *pHandle)
>  {
> +     pHandle->msg_cache = kmem_cache_create("wilc_message_queue",
> +                                             sizeof(Message),
> +                                             0, SLAB_POISON, NULL);
> +     if (!pHandle->msg_cache)
> +             return -ENOMEM;

This is a separate thing from using list macros.  Or maybe it's not but
that isn't explained in the changelog.  Do one thing per patch so it's
easier to review.

> +
>       spin_lock_init(&pHandle->strCriticalSection);
>       sema_init(&pHandle->hSem, 0);
> -     pHandle->pstrMessageList = NULL;
> +     INIT_LIST_HEAD(&pHandle->msg_list_head);
>       pHandle->u32ReceiversCount = 0;
>       pHandle->bExiting = false;
>       return 0;
> @@ -28,6 +34,7 @@ int wilc_mq_create(WILC_MsgQueueHandle *pHandle)
>   */
>  int wilc_mq_destroy(WILC_MsgQueueHandle *pHandle)
>  {
> +     Message *msg;
>       pHandle->bExiting = true;
>  
>       /* Release any waiting receiver thread. */
> @@ -36,13 +43,16 @@ int wilc_mq_destroy(WILC_MsgQueueHandle *pHandle)
>               pHandle->u32ReceiversCount--;
>       }
>  
> -     while (pHandle->pstrMessageList) {
> -             Message *pstrMessge = pHandle->pstrMessageList->pstrNext;
> -
> -             kfree(pHandle->pstrMessageList);
> -             pHandle->pstrMessageList = pstrMessge;
> +     while (!list_empty(&pHandle->msg_list_head)) {
> +             msg = list_first_entry(&pHandle->msg_list_head,
> +                                     Message, link);
> +             list_del(&msg->link);
> +             kfree(msg->pvBuffer);
> +             kmem_cache_free(pHandle->msg_cache, msg);
>       }
>  
> +     kmem_cache_destroy(pHandle->msg_cache);
> +
>       return 0;
>  }
>  
> @@ -74,41 +84,28 @@ int wilc_mq_send(WILC_MsgQueueHandle *pHandle,
>       spin_lock_irqsave(&pHandle->strCriticalSection, flags);
>  
>       /* construct a new message */
> -     pstrMessage = kmalloc(sizeof(Message), GFP_ATOMIC);
> -     if (!pstrMessage)
> +     pstrMessage = kmem_cache_zalloc(pHandle->msg_cache, GFP_ATOMIC);
> +     if (!pstrMessage) {
> +             spin_unlock_irqrestore(&pHandle->strCriticalSection, flags);
>               return -ENOMEM;
> +     }

Do the allocation outside the lock so it doesn't have to be atomic.

>       pstrMessage->u32Length = u32SendBufferSize;
> -     pstrMessage->pstrNext = NULL;
>       pstrMessage->pvBuffer = kmalloc(u32SendBufferSize, GFP_ATOMIC);
>       if (!pstrMessage->pvBuffer) {
> +             kmem_cache_free(pHandle->msg_cache, pstrMessage);
>               result = -ENOMEM;
>               goto ERRORHANDLER;

This goto is meaningless now that it doesn't handle any errors.  Just do
a direct return.  Except, wait, aren't we holding a couple locks?  This
looks buggy.

>       }
>       memcpy(pstrMessage->pvBuffer, pvSendBuffer, u32SendBufferSize);
>  
>       /* add it to the message queue */
> -     if (!pHandle->pstrMessageList) {
> -             pHandle->pstrMessageList  = pstrMessage;
> -     } else {
> -             Message *pstrTailMsg = pHandle->pstrMessageList;
> -
> -             while (pstrTailMsg->pstrNext)
> -                     pstrTailMsg = pstrTailMsg->pstrNext;
> -
> -             pstrTailMsg->pstrNext = pstrMessage;
> -     }
> +     list_add_tail(&pstrMessage->link, &pHandle->msg_list_head);
>  
>       spin_unlock_irqrestore(&pHandle->strCriticalSection, flags);
>  
>       up(&pHandle->hSem);
>  
>  ERRORHANDLER:
> -     /* error occured, free any allocations */
> -     if (pstrMessage) {
> -             kfree(pstrMessage->pvBuffer);
> -             kfree(pstrMessage);
> -     }
> -
>       return result;
>  }
>  
> @@ -144,11 +141,6 @@ int wilc_mq_recv(WILC_MsgQueueHandle *pHandle,
>       down(&pHandle->hSem);
>  
>       /* other non-timeout scenarios */
> -     if (result) {
> -             PRINT_ER("Non-timeout\n");
> -             return result;
> -     }
> -

This is correct, but not related to list macros.  Do it in a separate
patch.

>       if (pHandle->bExiting) {
>               PRINT_ER("pHandle fail\n");
>               return -EFAULT;
> @@ -156,12 +148,13 @@ int wilc_mq_recv(WILC_MsgQueueHandle *pHandle,
>  
>       spin_lock_irqsave(&pHandle->strCriticalSection, flags);
>  
> -     pstrMessage = pHandle->pstrMessageList;
> -     if (!pstrMessage) {
> +     if (list_empty(&pHandle->msg_list_head)) {
>               spin_unlock_irqrestore(&pHandle->strCriticalSection, flags);
>               PRINT_ER("pstrMessage is null\n");

This printk is outdated now.

>               return -EFAULT;
>       }
> +

regards,
dan carpenter

_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Reply via email to