[RFC] Killing struct cmd_ctrl_node.wait_option

[Dan Williams]

On Thu, 2007-12-13 at 13:48 -0500, David Woodhouse wrote:
> I had this patch lined up last night, but now I'm not so sure.
> 
> I want command nodes to be freed by whoever submitted them -- i.e. do it
> in __lbs_cmd() (and lbs_prepare_and_send_command() too for now, until
> that dies). That way we can fix the races with priv->cur_cmd_retcode,
> which as it stands can be overwritten by subsequent commands before the
> thread waiting in __lbs_cmd() ever wakes up and looks at it.
> 
> The question is: how do we free the command structures for the
> _asynchronous_ commands? We can't do it from the callback if we aren't
> calling the callback on failure (which we aren't, right now).

Yeah, I started thinking about that for async commands too yesterday.
If we want to completely kill prepare_and_send_command(), we'll have to
make __lbs_cmd() do async operations in some fashion.  Maybe we do an
lbs_cmd_async() too, which must take a callback but for which 'extra' is
always NULL, because there's nothing to return back to the caller,
because it's async.

> The simple answer is that we actually start using the wait_option field,
> which means I shouldn't remove it. If wait_option isn't set (indicating
> an async command), the structure should be freed in cmdresp.c.
> 
> I think I prefer the alternative answer, which is that we start calling
> the callbacks even in the failure case. Yes, it means the callback has
> to start checking resp->result. But most functions will hopefully be
> using the same callback function (lbs_cmd_copyback) anyway, so it's not
> as if we have to add that in too many places.

Sounds fine, do this, though we need to adapt __lbs_cmd() to handle
async commands too.

Dan

> commit f6a24cc26d0ad95c440846b15ec218e386994e4a
> Author: David Woodhouse <dwmw2 at infradead.org>
> Date:   Thu Dec 13 02:36:57 2007 -0500
> 
>     libertas: kill unused wait_option field in struct cmd_ctrl_node
>     
>     Signed-off-by: David Woodhouse <dwmw2 at infradead.org>
> 
> diff --git a/drivers/net/wireless/libertas/cmd.c b/drivers/net/wireless/libertas/cmd.c
> index 171acc2..01052cf 100644
> --- a/drivers/net/wireless/libertas/cmd.c
> +++ b/drivers/net/wireless/libertas/cmd.c
> @@ -17,7 +17,7 @@ static void cleanup_cmdnode(struct cmd_ctrl_node *ptempnode);
>  static struct cmd_ctrl_node *lbs_get_cmd_ctrl_node(struct lbs_private *priv);
>  static void lbs_set_cmd_ctrl_node(struct lbs_private *priv,
>  		    struct cmd_ctrl_node *ptempnode,
> -		    u16 wait_option, void *pdata_buf);
> +		    void *pdata_buf);
>  
> 
>  /**
> @@ -1392,7 +1392,7 @@ int lbs_prepare_and_send_command(struct lbs_private *priv,
>  		goto done;
>  	}
>  
> -	lbs_set_cmd_ctrl_node(priv, cmdnode, wait_option, pdata_buf);
> +	lbs_set_cmd_ctrl_node(priv, cmdnode, pdata_buf);
>  
>  	cmdptr = (struct cmd_ds_command *)cmdnode->cmdbuf;
>  
> @@ -1554,7 +1554,7 @@ int lbs_prepare_and_send_command(struct lbs_private *priv,
>  	case CMD_802_11_INACTIVITY_TIMEOUT:
>  		ret = lbs_cmd_802_11_inactivity_timeout(priv, cmdptr,
>  							 cmd_action, pdata_buf);
> -		lbs_set_cmd_ctrl_node(priv, cmdnode, 0, pdata_buf);
> +		lbs_set_cmd_ctrl_node(priv, cmdnode, pdata_buf);
>  		break;
>  
>  	case CMD_802_11_TPC_CFG:
> @@ -1800,7 +1800,6 @@ static void cleanup_cmdnode(struct cmd_ctrl_node *cmdnode)
>  		return;
>  	cmdnode->cmdwaitqwoken = 1;
>  	wake_up_interruptible(&cmdnode->cmdwait_q);
> -	cmdnode->wait_option = 0;
>  	cmdnode->pdata_buf = NULL;
>  	cmdnode->callback = NULL;
>  	cmdnode->callback_arg = 0;
> @@ -1816,20 +1815,18 @@ static void cleanup_cmdnode(struct cmd_ctrl_node *cmdnode)
>   *
>   *  @param priv		A pointer to struct lbs_private structure
>   *  @param ptempnode	A pointer to cmd_ctrl_node structure
> - *  @param wait_option	wait option: wait response or not
>   *  @param pdata_buf	A pointer to informaion buffer
>   *  @return 		0 or -1
>   */
>  static void lbs_set_cmd_ctrl_node(struct lbs_private *priv,
>  				  struct cmd_ctrl_node *ptempnode,
> -				  u16 wait_option, void *pdata_buf)
> +				  void *pdata_buf)
>  {
>  	lbs_deb_enter(LBS_DEB_HOST);
>  
>  	if (!ptempnode)
>  		return;
>  
> -	ptempnode->wait_option = wait_option;
>  	ptempnode->pdata_buf = pdata_buf;
>  	ptempnode->callback = NULL;
>  	ptempnode->callback_arg = 0;
> @@ -2213,7 +2210,6 @@ int __lbs_cmd(struct lbs_private *priv, uint16_t command,
>  		goto done;
>  	}
>  
> -	cmdnode->wait_option = CMD_OPTION_WAITFORRSP;
>  	cmdnode->callback = callback;
>  	cmdnode->callback_arg = callback_arg;
>  
> diff --git a/drivers/net/wireless/libertas/hostcmd.h b/drivers/net/wireless/libertas/hostcmd.h
> index aa4cea0..1b31250 100644
> --- a/drivers/net/wireless/libertas/hostcmd.h
> +++ b/drivers/net/wireless/libertas/hostcmd.h
> @@ -74,8 +74,6 @@ struct cmd_header {
>  
>  struct cmd_ctrl_node {
>  	struct list_head list;
> -	/* wait for finish or not */
> -	u16 wait_option;
>  	/* command response */
>  	void *pdata_buf;
>  	int (*callback)(struct lbs_private *, unsigned long, struct cmd_header *);
>