From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1424438AbcFMOYU (ORCPT ); Mon, 13 Jun 2016 10:24:20 -0400 Received: from mout.kundenserver.de ([212.227.17.24]:55086 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1424120AbcFMOYQ (ORCPT ); Mon, 13 Jun 2016 10:24:16 -0400 From: Arnd Bergmann To: Binoy Jayan Cc: Greg Kroah-Hartman , Johnny Kim , Austin Shin , Chris Park , Tony Cho , Glen Lee , Leo Kim , devel@driverdev.osuosl.org, linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 6/7] staging: wilc1000: message_queue: Replace semaphore sem with completion Date: Mon, 13 Jun 2016 16:24:57 +0200 Message-ID: <3241664.xlvexBiRFW@wuerfel> User-Agent: KMail/5.1.3 (Linux/4.4.0-22-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: <1465814259-3009-7-git-send-email-binoy.jayan@linaro.org> References: <1465814259-3009-1-git-send-email-binoy.jayan@linaro.org> <1465814259-3009-7-git-send-email-binoy.jayan@linaro.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:o4Kqju5I4XoiIWOQ31i4htOlzJw1LN8opFdSf+uxnsP6sb+9sKJ xe4M5f71SIjvuxCcnMKJdjEn6grDI5rsrDGxUS8SNr+PAussLeMrG/v9kLc/bB6RvYACdlG cmR0b3BMRT0HviA87V/qKkdTvUM6mdSEvLxBQE/gMLJiHUR4EKSqF2uYk6p9JLUHYV1xzxc 5FbzCGDndNtmPTkX+xSuQ== X-UI-Out-Filterresults: notjunk:1;V01:K0:BhC4MNUHsGw=:zZhueREG8MZeI8+Al87aok oxkVtcOacspKjPo60jcuitLDVdLxT6lljsZrX0ctOHs1SuA2BsiY2l7rJTuCCetO6d9CGb7YG 8WAzrE+vFT+hSID/TqhpS9WLoTWDEssJCXD+TMrUBOS/dmV+RZPYq9tFmbMdpJPaDTnIsZffh KjPfnofsMO3aBx7Dx2of4Ft/41T61stJuOVTSlCq+IYzgyCzRkP+80XxmG/HhEcZMX/PoHop1 QuWXb+nkClFaxr1KtS3iUqcEISzzhVBhoz30suKRN3RNi8bcvSGckltd88dz5Xrx6XBe7kpzs dLSYygmiF+PSmn/ococ0e2UuoKJPmJK4QerZeRrbuQpgKG7YrllU0x1zYcJzJPvy2TteDhrvx 2BSx7IXSeGndycScWpmS2uWTTWediuPxxnKdNJxAA/lUs66aXtEx82ymihfIfHGNZy+SH4F88 L8/FnzXnCmyB5ZWLikZuPWnZquWR8jtgiAI6IwCe7CPWgFypgNCnniZhai99QpgSnPskCinWy dtOphX8ZuJC1Lx44rFeWyaaNjnx4ibDDxdV+9q7SXmiRd0MOxJqp0E6UFnd+qFR59FhKPXi6y GTkQ4cde6ySx5KF2XqsNk8263xGJGmi90tUlxr74CUdOvEh6q9v3HltrbtlWUs17aFboVaxgr 9u7sUIMrnftukpJG59rDi/nuw6UuGuWWK2nYSSqr9zCn/lVg0QXEB6tSWm8labCLgsvVqaOhY Mh50Yb2AKZy2jiSA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Monday, June 13, 2016 4:07:38 PM CEST Binoy Jayan wrote: > The semaphore 'sem' is used as completion, so convert it to a > struct completion type. > > Signed-off-by: Binoy Jayan This does not really look like a classic completion, instead I'd classify this as a counting semaphore. > --- > drivers/staging/wilc1000/wilc_msgqueue.c | 13 +++++++------ > drivers/staging/wilc1000/wilc_msgqueue.h | 4 ++-- > 2 files changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/staging/wilc1000/wilc_msgqueue.c b/drivers/staging/wilc1000/wilc_msgqueue.c > index 6cb894e..80c9631 100644 > --- a/drivers/staging/wilc1000/wilc_msgqueue.c > +++ b/drivers/staging/wilc1000/wilc_msgqueue.c > @@ -3,6 +3,7 @@ > #include > #include > #include > +#include > > /*! > * @author syounan > @@ -13,7 +14,7 @@ > int wilc_mq_create(struct message_queue *mq) > { > spin_lock_init(&mq->lock); > - sema_init(&mq->sem, 0); > + init_completion(&mq->comp); > INIT_LIST_HEAD(&mq->msg_list); > mq->recv_count = 0; > mq->exiting = false; > @@ -34,7 +35,7 @@ int wilc_mq_destroy(struct message_queue *mq) > > /* Release any waiting receiver thread. */ > while (mq->recv_count > 0) { > - up(&mq->sem); > + complete(&mq->comp); > mq->recv_count--; > } > Here it gets released multiple times in a row, always corresponding to recv_count. > @@ -85,7 +86,7 @@ int wilc_mq_send(struct message_queue *mq, > > spin_unlock_irqrestore(&mq->lock, flags); > > - up(&mq->sem); > + complete(&mq->comp); > > return 0; > } > @@ -112,19 +113,19 @@ int wilc_mq_recv(struct message_queue *mq, > mq->recv_count++; > spin_unlock_irqrestore(&mq->lock, flags); > > - down(&mq->sem); > + wait_for_completion(&mq->comp); > spin_lock_irqsave(&mq->lock, flags); > > if (list_empty(&mq->msg_list)) { > spin_unlock_irqrestore(&mq->lock, flags); > - up(&mq->sem); > + complete(&mq->comp); > return -EFAULT; > } > /* check buffer size */ > msg = list_first_entry(&mq->msg_list, struct message, list); > if (recv_buf_size < msg->len) { > spin_unlock_irqrestore(&mq->lock, flags); > - up(&mq->sem); > + complete(&mq->comp); > return -EOVERFLOW; > } > And here you have the same function call both up() and down(), which is fairly unusual. My reading of the functions is that the semaphore value is the number of messages currently outstanding to be consumed by wilc_mq_recv. Interestingly, there is only one instance of the message queue, in host_interface.c, with a single caller of the recv function and many instances of send, so a good start for a cleanup would be to move all the contents of wilc_msgqueue.c and wilc_msgqueue.h into host_interface.c, making the functions all 'static'. After that, the next observation is that the entire host_interface.c file is basically a reimplementation of a 'work queue', and that should not be needed. We can deconstruct that kthread/message_queue logic by replacing it with a regular create_singlethread_workqueue()/queue_work() setup, by adding a 'struct work_struct' to 'struct host_if_msg'. The current hostIFthread() loop then becomes a simple work queue helper (without the loop). Finally, we could move handling for each individual members of 'union message_body' out into a separate 'struct work_struct' and completely remove the multiplexer that is currently part of hostIFthread(), allowing us to move the implementation of each message handler into the callsite of the function that currently sends the 'host_if_msg'. I would suggest adding this last part to the TODO file, but trying to just do the previous step of using a single work function to get rid of the message queue implementation and the semaphore inside of it. Arnd