Re: [PATCH] comedi: comedi_test: Prevent timers rescheduling during deletion

[Ian Abbott]

On 13/02/2024 18:10, Ian Abbott wrote:
> The comedi_test devices have a couple of timers (ai_timer and ao_timer)
> that can be started to simulate hardware interrupts.  Their expiry
> functions normally reschedule the timer.  The driver code calls either
> del_timer_sync() or del_timer() to delete the timers from the queue, but
> does not currently prevent the timers from rescheduling themselves so
> synchronized deletion may be ineffective.
>
> Add a couple of boolean members (one for each timer: ai_timer_enable and
> ao_timer_enable) to the device private data structure to indicate
> whether the timers are allowed to reschedule themselves.  Set the member
> to true when adding the timer to the queue, and to false when deleting
> the timer from the queue in the waveform_ai_cancel() and
> waveform_ao_cancel() functions.
>
> The del_timer_sync() function is also called from the waveform_detach()
> function, but the timer enable members will already be set to false when
> that function is called, so no change is needed there.
>
> Fixes: 403fe7f34e33 ("staging: comedi: comedi_test: fix timer race conditions")
> Cc: <stable@xxxxxxxxxxxxxxx> # 4.4+
> Signed-off-by: Ian Abbott <abbotti@xxxxxxxxx>
> ---
>   drivers/comedi/drivers/comedi_test.c | 37 +++++++++++++++++++++++++---
>   1 file changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/comedi/drivers/comedi_test.c b/drivers/comedi/drivers/comedi_test.c
> index 30ea8b53ebf8..7fefe0de0bcc 100644
> --- a/drivers/comedi/drivers/comedi_test.c
> +++ b/drivers/comedi/drivers/comedi_test.c
> @@ -87,6 +87,8 @@ struct waveform_private {
>   	struct comedi_device *dev;	/* parent comedi device */
>   	u64 ao_last_scan_time;		/* time of previous AO scan in usec */
>   	unsigned int ao_scan_period;	/* AO scan period in usec */
> +	bool ai_timer_enable:1;		/* should AI timer be running? */
> +	bool ao_timer_enable:1;		/* should AO timer be running? */
>   	unsigned short ao_loopbacks[N_CHANS];
>   };
>   
> @@ -232,12 +234,18 @@ static void waveform_ai_timer(struct timer_list *t)
>   	if (cmd->stop_src == TRIG_COUNT && async->scans_done >= cmd->stop_arg) {
>   		async->events |= COMEDI_CB_EOA;
>   	} else {
> +		unsigned long flags;
> +
>   		if (devpriv->ai_convert_time > now)
>   			time_increment = devpriv->ai_convert_time - now;
>   		else
>   			time_increment = 1;
> -		mod_timer(&devpriv->ai_timer,
> -			  jiffies + usecs_to_jiffies(time_increment));
> +		spin_lock_irqsave(&dev->spinlock, flags);
> +		if (devpriv->ai_timer_enable) {
> +			mod_timer(&devpriv->ai_timer,
> +				  jiffies + usecs_to_jiffies(time_increment));
> +		}
> +		spin_unlock_irqrestore(&dev->spinlock, flags);

Actually, I should have used a plain old spin_lock() in the above (and 
spin_lock_bh() elsewhere in the code) instead of spin_lock_irqsave(), 
since there are no hard interrupts involved.

I'll send a v2 patch shortly.

--
-=( Ian Abbott <abbotti@xxxxxxxxx> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-