From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752381AbcFUQFx (ORCPT ); Tue, 21 Jun 2016 12:05:53 -0400 Received: from mout.kundenserver.de ([212.227.17.13]:59982 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751900AbcFUQFw (ORCPT ); Tue, 21 Jun 2016 12:05:52 -0400 From: Arnd Bergmann To: Binoy Jayan Cc: Greg Kroah-Hartman , Johnny Kim , Austin Shin , Chris Park , Tony Cho , Glen Lee , Leo Kim , linux-wireless@vger.kernel.org, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 2/2] staging: wilc1000: Replace kthread with workqueue for host interface Date: Tue, 21 Jun 2016 18:07:41 +0200 Message-ID: <7461266.G5R58nnEKu@wuerfel> User-Agent: KMail/5.1.3 (Linux/4.4.0-22-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: <1466417419-13568-3-git-send-email-binoy.jayan@linaro.org> References: <1465814259-3009-1-git-send-email-binoy.jayan@linaro.org> <1466417419-13568-1-git-send-email-binoy.jayan@linaro.org> <1466417419-13568-3-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:1ToHY/SxxsmZOa8d7QlvLX9xj0O4CgJWMIUlZ9ZXsNWwBXCxj/J lB8fZzMBZkcpW8id/caeZ6RZdClhUtP29Y2vUyQ1CR8UpDjgJX/OVAa67fpOL4TSds7KboO 8yj25uZ9SOXYFX8mIIQtDYL1odNzuEA4UBlqNPI9rYR7n2DjZqf5EL007YEVdnB24V1+Wc1 ZynzaPyLJgU5+27mYLi0Q== X-UI-Out-Filterresults: notjunk:1;V01:K0:hJbLb3vbcYk=:GsU+7xkU32JnDAzZLeEtHL AnNsH9SKDMh+afi8urJO1uzfNx84kCjdTHyeScY2TpKxkEIT0TEHFiyfTau8Pk+EFM6TOycaP rjMaxAsNk5LjBBIQyGgNBqstwtvtgahqEisyxPkCdamv0qpBwQEUa4SLk51Tyvl+7i2Y78NCc 0lrGoMgBcGcPBTZsfWzECWIJcYnW99QB/IV7vaY75nKzLYkZqp18pIowEHDdU11LdMzBNHKJa 9OScqpfqRZf+JvWBGItuZUFfP5yj9GX0bO3zSTZcU9CMUUuTd29SCjrSEVDZv8qgT+J3xq74U CeQS3Xq3nDppbY4qHtOGQ2LWqYp1h+luABPA1TOYtrd+275JxAbq36VZU4ghxD1aAaYMIH94E 9K0GyZFBeOA3C1InDxR2zoDaN/Wmz8Ivq5EUatO8XC8xvDrKv27zo/6X7nx/Bi2vPNtabBqNv b64xC+6uA+7MhxtSXo2TTiOTD2zqJU6jnAnhqPGB0OlQW81T/dE8raAzxTOZNkJLZigLM75kq FnKCx2RpnsCbwGN7fq/dcOaCAhR7fEeupYENb4r3Hqz2fo2URJ725bTEh69DQcbvlSp9aG6ch 7/OpU5LTSE/hT5N/8gU1Z/zNu3rfgsJmyv25Ly8JE4IBy8C3ubPbnCl1guQ4cao1CGAWhkxt4 Q/6INOk+9C4KYfxujvffI4bZGxSagYgbBT8K5b7xKaM8hFlxS+kA6bGYTml1E6Kq9wtoTXM4B MPtfwrHbTiZqP2Bh Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Monday, June 20, 2016 3:40:19 PM CEST Binoy Jayan wrote: > Deconstruct the kthread / message_queue logic, replacing it with > create_singlethread_workqueue() / queue_work() setup, by adding a > 'struct work_struct' to 'struct host_if_msg'. The current kthread > hostIFthread() is converted to a work queue helper with the name > 'host_if_work'. > > Signed-off-by: Binoy Jayan > Reviewed-by: Arnd Bergmann Hi Binoy, You made a mistake here in adding the 'Reviewed-by:' tag before I replied with that tag. I did a review of an earlier version and you addressed the comments that I had for that, but you can only add the tag after I send that reply. That said, the patch looks very good to me, and I see no remaining problems that would prevent it from getting merged (after Atmel have tested it), just a couple of things that would make it easier to review: > +static int wilc_enqueue_cmd(struct host_if_msg *msg); > +static void host_if_work(struct work_struct *work); A small comment on coding style: we try to avoid forward declaration for local functions. Instead, you can reorder the code to have the callee first. This is the order which most readers will expect, and having no forward declarations in the code makes it more likely that there are no recursions that would be problematic for stack overflow. > /*! > * @author syounan > @@ -336,95 +276,19 @@ static int wilc_mq_destroy(struct message_queue *mq) > * @note copied from FLO glue implementatuion > * @version 1.0 > */ > -static int wilc_mq_send(struct message_queue *mq, > - const void *send_buf, u32 send_buf_size) > +static int wilc_enqueue_cmd(struct host_if_msg *msg) I think this API change can be done as a separate patch: the mq and send_buf_size arguments are both constant for all callers, so you can have one patch just removes them and renames the function, to make the patch that does the tricky rework smaller. > - } > + if (msg->id == HOST_IF_MSG_CONNECT && > + msg->vif->hif_drv->usr_scan_req.scan_result) { > + wilc_enqueue_cmd(msg); > + usleep_range(2 * 1000, 2 * 1000); > + goto end; > + } > > - switch (msg.id) { > - case HOST_IF_MSG_SCAN: A similar trick could apply here: we can leave the switch() indented at the original level by putting it in an 'else' clause. This again makes the patch shorter and easier to review, and the optional reformatting can be done as a follow-up. Arnd