Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1427913

Re: [PATCH v2 2/2] staging: wilc1000: Replace kthread with workqueue for host interface

Show all headers | View raw

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 <binoy.jayan@linaro.org>
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 0/7] *** staging: wilc1000: Replace semaphores with mutexes or completions *** Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-13 12:40 +0200
  [PATCH 1/7] staging: wilc1000: Replace semaphore txq_event with completion Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-13 12:40 +0200
  [PATCH 2/7] staging: wilc1000: Replace semaphore txq_add_to_head_cs with mutex Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-13 12:40 +0200
    Re: [PATCH 2/7] staging: wilc1000: Replace semaphore txq_add_to_head_cs with mutex Arnd Bergmann <arnd@arndb.de> - 2016-06-13 15:20 +0200
  [PATCH 3/7] staging: wilc1000: Replace semaphore cfg_event with completion Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-13 12:40 +0200
    Re: [PATCH 3/7] staging: wilc1000: Replace semaphore cfg_event with completion Arnd Bergmann <arnd@arndb.de> - 2016-06-13 16:30 +0200
  Re: [PATCH 0/7] *** staging: wilc1000: Replace semaphores with mutexes or completions *** Arnd Bergmann <arnd@arndb.de> - 2016-06-13 16:30 +0200
    Re: [PATCH 0/7] *** staging: wilc1000: Replace semaphores with  mutexes or completions *** Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-13 16:50 +0200
  [PATCH v2 0/5] *** staging: wilc1000: Replace semaphores with mutexes or completions *** Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-15 07:30 +0200
    [PATCH v2 1/5] staging: wilc1000: Replace semaphore txq_event with completion Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-15 07:30 +0200
  [PATCH v3 4/5] staging: wilc1000: Replace semaphore sync_event with completion Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-15 07:40 +0200
  [PATCH v3 0/5] *** staging: wilc1000: Replace semaphores with mutexes or completions *** Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-15 07:40 +0200
    [PATCH v3 5/5] staging: wilc1000: Remove semaphore close_exit_sync Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-15 07:40 +0200
    [PATCH v3 3/5] staging: wilc1000: Replace semaphore cfg_event with completion Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-15 07:40 +0200
    [PATCH v3 1/5] staging: wilc1000: Replace semaphore txq_event with completion Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-15 07:40 +0200
    [PATCH v3 2/5] staging: wilc1000: Replace semaphore txq_add_to_head_cs with mutex Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-15 07:40 +0200
  [PATCH v2 1/2] staging: wilc1000: message_queue: Move code to host interface Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-20 12:20 +0200
  [PATCH v2 0/2] *** staging: wilc1000: Replace semaphores *** Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-20 12:20 +0200
    [PATCH v2 2/2] staging: wilc1000: Replace kthread with workqueue for host interface Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-20 12:20 +0200
      Re: [PATCH v2 2/2] staging: wilc1000: Replace kthread with workqueue for host interface Arnd Bergmann <arnd@arndb.de> - 2016-06-21 18:10 +0200
    [PATCH v3 2/3] staging: wilc1000: Replace kthread with workqueue for host interface Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-22 12:10 +0200
    [PATCH v3 0/3] *** staging: wilc1000: Replace semaphores *** Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-22 12:10 +0200
      [PATCH v3 1/3] staging: wilc1000: message_queue: Move code to host interface Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-22 12:10 +0200
      [PATCH v3 3/3] staging: wilc1000: Change interface wilc_mq_send to wilc_enqueue_cmd Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-22 12:10 +0200
    [PATCH v4 2/3] staging: wilc1000: Replace kthread with workqueue for host interface Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-23 07:50 +0200
    [PATCH v4 0/3] *** staging: wilc1000: Replace semaphores *** Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-23 07:50 +0200
      [PATCH v4 3/3] staging: wilc1000: Change interface wilc_mq_send to wilc_enqueue_cmd Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-23 07:50 +0200
      [PATCH v4 1/3] staging: wilc1000: message_queue: Move code to host interface Binoy Jayan <binoy.jayan@linaro.org> - 2016-06-23 07:50 +0200
      Re: [PATCH v4 0/3] *** staging: wilc1000: Replace semaphores *** Arnd Bergmann <arnd@arndb.de> - 2016-06-23 11:40 +0200

csiph-web