[PATCH 34/53] staging: comedi: usbdux: tidy up usbdux_ai_cmd()

[Ian Abbott]

On 2013-07-24 22:19, H Hartley Sweeten wrote:
> Rename the local variable used for the private data pointer to the
> comedi "norm".
>
> Remove the unnecessary sanity check of the private data pointer. This
> function can only be called is the private data was allocated during
> the attach.
>
> Tidy up the exit path using goto to ensure that the semaphore is
> released.
>
> Set the ai_cmd_running flag after submitting the urbs to remove the
> need to clear the flag if the submit fails.

As for ai_inttrig, this could cause the URb completion routine to 
execute before the ai_cmd_running flag has been set.

>
> Signed-off-by: H Hartley Sweeten <hsweeten at visionengravers.com>
> Cc: Ian Abbott <abbotti at mev.co.uk>
> Cc: Greg Kroah-Hartman <gregkh at linuxfondation.org>
> ---
>   drivers/staging/comedi/drivers/usbdux.c | 92 ++++++++++++++++-----------------
>   1 file changed, 44 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/usbdux.c b/drivers/staging/comedi/drivers/usbdux.c
> index 204867b..1982e23 100644
> --- a/drivers/staging/comedi/drivers/usbdux.c
> +++ b/drivers/staging/comedi/drivers/usbdux.c
> @@ -881,86 +881,79 @@ ai_trig_exit:
>
>   static int usbdux_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
>   {
> +	struct usbdux_private *devpriv = dev->private;
>   	struct comedi_cmd *cmd = &s->async->cmd;
> -	unsigned int chan, range;
> -	int i, ret;
> -	struct usbdux_private *this_usbduxsub = dev->private;
> -	int result;
> -
> -	if (!this_usbduxsub)
> -		return -EFAULT;
> +	int len = cmd->chanlist_len;
> +	int ret = -EBUSY;
> +	int i;
>
>   	/* block other CPUs from starting an ai_cmd */
> -	down(&this_usbduxsub->sem);
> -	if (this_usbduxsub->ai_cmd_running) {
> -		up(&this_usbduxsub->sem);
> -		return -EBUSY;
> -	}
> +	down(&devpriv->sem);
> +
> +	if (devpriv->ai_cmd_running)
> +		goto ai_cmd_exit;
> +
>   	/* set current channel of the running acquisition to zero */
>   	s->async->cur_chan = 0;
>
> -	this_usbduxsub->dux_commands[1] = cmd->chanlist_len;
> -	for (i = 0; i < cmd->chanlist_len; ++i) {
> -		chan = CR_CHAN(cmd->chanlist[i]);
> -		range = CR_RANGE(cmd->chanlist[i]);
> +	devpriv->dux_commands[1] = len;
> +	for (i = 0; i < len; ++i) {
> +		unsigned int chan = CR_CHAN(cmd->chanlist[i]);
> +		unsigned int range = CR_RANGE(cmd->chanlist[i]);
> +
>   		if (i >= NUMCHANNELS)
>   			break;
> -		this_usbduxsub->dux_commands[i + 2] =
> -		    create_adc_command(chan, range);
> -	}
>
> -	result = send_dux_commands(dev, SENDADCOMMANDS);
> -	if (result < 0) {
> -		up(&this_usbduxsub->sem);
> -		return result;
> +		devpriv->dux_commands[i + 2] = create_adc_command(chan, range);
>   	}
>
> -	if (this_usbduxsub->high_speed) {
> +	ret = send_dux_commands(dev, SENDADCOMMANDS);
> +	if (ret < 0)
> +		goto ai_cmd_exit;
> +
> +	if (devpriv->high_speed) {
>   		/*
>   		 * every channel gets a time window of 125us. Thus, if we
>   		 * sample all 8 channels we need 1ms. If we sample only one
>   		 * channel we need only 125us
>   		 */
> -		this_usbduxsub->ai_interval = 1;
> +		devpriv->ai_interval = 1;
>   		/* find a power of 2 for the interval */
> -		while ((this_usbduxsub->ai_interval) < (cmd->chanlist_len)) {
> -			this_usbduxsub->ai_interval =
> -			    (this_usbduxsub->ai_interval) * 2;
> -		}
> -		this_usbduxsub->ai_timer = cmd->scan_begin_arg / (125000 *
> -							  (this_usbduxsub->
> -							   ai_interval));
> +		while (devpriv->ai_interval < len)
> +			devpriv->ai_interval *= 2;
> +
> +		devpriv->ai_timer = cmd->scan_begin_arg /
> +				    (125000 * devpriv->ai_interval);
>   	} else {
>   		/* interval always 1ms */
> -		this_usbduxsub->ai_interval = 1;
> -		this_usbduxsub->ai_timer = cmd->scan_begin_arg / 1000000;
> +		devpriv->ai_interval = 1;
> +		devpriv->ai_timer = cmd->scan_begin_arg / 1000000;
>   	}
> -	if (this_usbduxsub->ai_timer < 1) {
> -		up(&this_usbduxsub->sem);
> -		return -EINVAL;
> +	if (devpriv->ai_timer < 1) {
> +		ret = -EINVAL;
> +		goto ai_cmd_exit;
>   	}
> -	this_usbduxsub->ai_counter = this_usbduxsub->ai_timer;
> +
> +	devpriv->ai_counter = devpriv->ai_timer;
>
>   	if (cmd->stop_src == TRIG_COUNT) {
>   		/* data arrives as one packet */
> -		this_usbduxsub->ai_sample_count = cmd->stop_arg;
> -		this_usbduxsub->ai_continous = 0;
> +		devpriv->ai_sample_count = cmd->stop_arg;
> +		devpriv->ai_continous = 0;
>   	} else {
>   		/* continous acquisition */
> -		this_usbduxsub->ai_continous = 1;
> -		this_usbduxsub->ai_sample_count = 0;
> +		devpriv->ai_continous = 1;
> +		devpriv->ai_sample_count = 0;
>   	}
>
>   	if (cmd->start_src == TRIG_NOW) {
>   		/* enable this acquisition operation */
> -		this_usbduxsub->ai_cmd_running = 1;
>   		ret = usbduxsub_submit_inurbs(dev);
>   		if (ret < 0) {
> -			this_usbduxsub->ai_cmd_running = 0;
>   			/* fixme: unlink here?? */
> -			up(&this_usbduxsub->sem);
> -			return ret;
> +			goto ai_cmd_exit;
>   		}
> +		devpriv->ai_cmd_running = 1;
>   		s->async->inttrig = NULL;
>   	} else {
>   		/* TRIG_INT */
> @@ -968,8 +961,11 @@ static int usbdux_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
>   		/* wait for an internal signal */
>   		s->async->inttrig = usbdux_ai_inttrig;
>   	}
> -	up(&this_usbduxsub->sem);
> -	return 0;
> +
> +ai_cmd_exit:
> +	up(&devpriv->sem);
> +
> +	return ret;
>   }
>
>   /* Mode 0 is used to get a single conversion on demand */
>


-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti at mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-