Messages in this thread | | | Date | Wed, 14 Feb 2024 09:46:54 +0000 | Subject | Re: [PATCH] comedi: comedi_test: Prevent timers rescheduling during deletion | From | 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@vger.kernel.org> # 4.4+ > Signed-off-by: Ian Abbott <abbotti@mev.co.uk> > --- > 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@mev.co.uk> || 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 )=-
| |