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: <[email protected]> # 4.4+
Signed-off-by: Ian Abbott <[email protected]>
---
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);
}
overrun:
@@ -352,6 +360,7 @@ static int waveform_ai_cmd(struct comedi_device *dev,
struct comedi_cmd *cmd = &s->async->cmd;
unsigned int first_convert_time;
u64 wf_current;
+ unsigned long flags;
if (cmd->flags & CMDF_PRIORITY) {
dev_err(dev->class_dev,
@@ -393,9 +402,12 @@ static int waveform_ai_cmd(struct comedi_device *dev,
* Seem to need an extra jiffy here, otherwise timer expires slightly
* early!
*/
+ spin_lock_irqsave(&dev->spinlock, flags);
+ devpriv->ai_timer_enable = true;
devpriv->ai_timer.expires =
jiffies + usecs_to_jiffies(devpriv->ai_convert_period) + 1;
add_timer(&devpriv->ai_timer);
+ spin_unlock_irqrestore(&dev->spinlock, flags);
return 0;
}
@@ -403,7 +415,11 @@ static int waveform_ai_cancel(struct comedi_device *dev,
struct comedi_subdevice *s)
{
struct waveform_private *devpriv = dev->private;
+ unsigned long flags;
+ spin_lock_irqsave(&dev->spinlock, flags);
+ devpriv->ai_timer_enable = false;
+ spin_unlock_irqrestore(&dev->spinlock, flags);
if (in_softirq()) {
/* Assume we were called from the timer routine itself. */
del_timer(&devpriv->ai_timer);
@@ -494,9 +510,14 @@ static void waveform_ao_timer(struct timer_list *t)
} else {
unsigned int time_inc = devpriv->ao_last_scan_time +
devpriv->ao_scan_period - now;
+ unsigned long flags;
- mod_timer(&devpriv->ao_timer,
- jiffies + usecs_to_jiffies(time_inc));
+ spin_lock_irqsave(&dev->spinlock, flags);
+ if (devpriv->ao_timer_enable) {
+ mod_timer(&devpriv->ao_timer,
+ jiffies + usecs_to_jiffies(time_inc));
+ }
+ spin_unlock_irqrestore(&dev->spinlock, flags);
}
underrun:
@@ -510,6 +531,7 @@ static int waveform_ao_inttrig_start(struct comedi_device *dev,
struct waveform_private *devpriv = dev->private;
struct comedi_async *async = s->async;
struct comedi_cmd *cmd = &async->cmd;
+ unsigned long flags;
if (trig_num != cmd->start_arg)
return -EINVAL;
@@ -517,9 +539,12 @@ static int waveform_ao_inttrig_start(struct comedi_device *dev,
async->inttrig = NULL;
devpriv->ao_last_scan_time = ktime_to_us(ktime_get());
+ spin_lock_irqsave(&dev->spinlock, flags);
+ devpriv->ao_timer_enable = true;
devpriv->ao_timer.expires =
jiffies + usecs_to_jiffies(devpriv->ao_scan_period);
add_timer(&devpriv->ao_timer);
+ spin_unlock_irqrestore(&dev->spinlock, flags);
return 1;
}
@@ -602,8 +627,12 @@ static int waveform_ao_cancel(struct comedi_device *dev,
struct comedi_subdevice *s)
{
struct waveform_private *devpriv = dev->private;
+ unsigned long flags;
s->async->inttrig = NULL;
+ spin_lock_irqsave(&dev->spinlock, flags);
+ devpriv->ao_timer_enable = false;
+ spin_unlock_irqrestore(&dev->spinlock, flags);
if (in_softirq()) {
/* Assume we were called from the timer routine itself. */
del_timer(&devpriv->ao_timer);
--
2.43.0
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: <[email protected]> # 4.4+
> Signed-off-by: Ian Abbott <[email protected]>
> ---
> 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 <[email protected]> || 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. || http://www.mev.co.uk )=-
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: <[email protected]> # 4.4+
Signed-off-by: Ian Abbott <[email protected]>
---
v2: Use spin_lock_bh() instead of spin_lock_irqsave().
---
drivers/comedi/drivers/comedi_test.c | 30 ++++++++++++++++++++++++----
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/drivers/comedi/drivers/comedi_test.c b/drivers/comedi/drivers/comedi_test.c
index 30ea8b53ebf8..05ae9122823f 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];
};
@@ -236,8 +238,12 @@ static void waveform_ai_timer(struct timer_list *t)
time_increment = devpriv->ai_convert_time - now;
else
time_increment = 1;
- mod_timer(&devpriv->ai_timer,
- jiffies + usecs_to_jiffies(time_increment));
+ spin_lock(&dev->spinlock);
+ if (devpriv->ai_timer_enable) {
+ mod_timer(&devpriv->ai_timer,
+ jiffies + usecs_to_jiffies(time_increment));
+ }
+ spin_unlock(&dev->spinlock);
}
overrun:
@@ -393,9 +399,12 @@ static int waveform_ai_cmd(struct comedi_device *dev,
* Seem to need an extra jiffy here, otherwise timer expires slightly
* early!
*/
+ spin_lock_bh(&dev->spinlock);
+ devpriv->ai_timer_enable = true;
devpriv->ai_timer.expires =
jiffies + usecs_to_jiffies(devpriv->ai_convert_period) + 1;
add_timer(&devpriv->ai_timer);
+ spin_unlock_bh(&dev->spinlock);
return 0;
}
@@ -404,6 +413,9 @@ static int waveform_ai_cancel(struct comedi_device *dev,
{
struct waveform_private *devpriv = dev->private;
+ spin_lock_bh(&dev->spinlock);
+ devpriv->ai_timer_enable = false;
+ spin_unlock_bh(&dev->spinlock);
if (in_softirq()) {
/* Assume we were called from the timer routine itself. */
del_timer(&devpriv->ai_timer);
@@ -495,8 +507,12 @@ static void waveform_ao_timer(struct timer_list *t)
unsigned int time_inc = devpriv->ao_last_scan_time +
devpriv->ao_scan_period - now;
- mod_timer(&devpriv->ao_timer,
- jiffies + usecs_to_jiffies(time_inc));
+ spin_lock(&dev->spinlock);
+ if (devpriv->ao_timer_enable) {
+ mod_timer(&devpriv->ao_timer,
+ jiffies + usecs_to_jiffies(time_inc));
+ }
+ spin_unlock(&dev->spinlock);
}
underrun:
@@ -517,9 +533,12 @@ static int waveform_ao_inttrig_start(struct comedi_device *dev,
async->inttrig = NULL;
devpriv->ao_last_scan_time = ktime_to_us(ktime_get());
+ spin_lock_bh(&dev->spinlock);
+ devpriv->ao_timer_enable = true;
devpriv->ao_timer.expires =
jiffies + usecs_to_jiffies(devpriv->ao_scan_period);
add_timer(&devpriv->ao_timer);
+ spin_unlock_bh(&dev->spinlock);
return 1;
}
@@ -604,6 +623,9 @@ static int waveform_ao_cancel(struct comedi_device *dev,
struct waveform_private *devpriv = dev->private;
s->async->inttrig = NULL;
+ spin_lock_bh(&dev->spinlock);
+ devpriv->ao_timer_enable = false;
+ spin_unlock_bh(&dev->spinlock);
if (in_softirq()) {
/* Assume we were called from the timer routine itself. */
del_timer(&devpriv->ao_timer);
--
2.43.0