[PATCH 13/22] staging: comedi: adv_pci1710: don't check the chanlist twice

[Ian Abbott]

On 24/04/14 18:35, Hartley Sweeten wrote:
> On Thursday, April 24, 2014 4:06 AM, Ian Abbott wrote:
>> On 2014-04-24 00:07, H Hartley Sweeten wrote:
>>> The chanlist is checked in Step 5 of the (*do_cmdtest) there is no
>>> reason to check it again in the (*do_cmd). The only reasonm its done
>>> is to get the actual 'seglen', the non-repeating length of the chanlist.
>>>
>>> Save the 'seglen' found by pci171x_ai_check_chanlist() in the private
>>> data and use that in the (*do_cmd).
>>
>> Since do_cmdtest can be done while a command is running, one needs to be
>> careful to make sure that any private data modified by do_cmdtest is not
>> used by the running command, and only used by do_cmd when setting up the
>> next command.
>
> Ian,
>
> I think the "live" command used for a running command and the "test"
> command are already handled properly by the core.
>
> The COMEDI_CMDTEST ioctl is handled by do_cmdtest_ioctl(). This function
> copies the users command to a local variable that is then passed to the
> subdevice (*do_cmdtest) function, this is the "test" command. Any
> modifications that function does to the command are then passed back
> to the user. Since the comedi_cmd is a local variable it does not affect the
> running command that is stored in the async->cmd.
>
> The COMEDI_CMD ioctl is handled by do_cmd_ioctl(). This function also
> copies the users command to a local variable. It then checks the subdevice
> lock and busy members to make sure that the command can actually be
> performed. If so it then sets the async->cmd to the comedi_command
> copied from the user, this is now the "live" command. The async->cmd
> is then passed to the subdevice (*do_cmdtest) function, just like with the
> COMEDI_CMDTEST ioctl. If the (*do_cmdtest) fails the "live" command
> is dropped by do_become_nonbusy(). If it succeeds the subdevice is
> set as busy and the command is executed by the (*do_cmd).
>
> So:
> 1a) If the user does a COMEDI_CMDTEST  when a command is not running
> we don't have any issues.
> 1b) If the user does a COMEDI_CMDTEST while a command is running it will
> not affect the "live" command in async->cmd since it is using a local variable
> for the comedi_cmd.
> 2a) If the user does a COMEDI_CMD when a command is not running we
> don't have any issues.
> 2b) If the user does a COMEDI_CMD while a command is running it will not
> succeed because the subdevice is busy.
>
> Have I missed something?

The patch is fine (as I mentioned in the second paragraph of my reply) 
although I thought devpriv->act_chanlist_len was now misnamed as it's 
used for setting up the next command and there may be an active command 
already running.

The first paragraph of my reply was just a comment about being careful 
when changing the private data in the do_cmdtest routine.  Hope that 
clears up any misunderstanding!


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