On Tuesday 04 July 2006 00:45, you wrote:
> +static void ams_handle_irq(void *data)
> +{
> + enum ams_irq irq = *((enum ams_irq *)data);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ams.irq_lock, flags);
> +
> + ams.worker_irqs |= irq;
> + schedule_work(&ams.worker);
> +
> + spin_unlock_irqrestore(&ams.irq_lock, flags);
> +}
I would say this is racy.
Locks should be added as shown below.
> +static void ams_worker(void *data)
> +{
unsigned long flags;
> + mutex_lock(&ams.lock);
> +
> + if (ams.has_device) {
spin_lock_irqsave(&ams.irq_lock, flags);
> + if (ams.worker_irqs & AMS_IRQ_FREEFALL) {
> + if (verbose)
> + printk(KERN_INFO "ams: freefall detected!\n");
> +
> + ams.worker_irqs &= ~AMS_IRQ_FREEFALL;
> + ams.clear_irq(AMS_IRQ_FREEFALL);
> + }
> +
> + if (ams.worker_irqs & AMS_IRQ_SHOCK) {
> + if (verbose)
> + printk(KERN_INFO "ams: shock detected!\n");
> +
> + ams.worker_irqs &= ~AMS_IRQ_SHOCK;
> + ams.clear_irq(AMS_IRQ_SHOCK);
> +
spin_unlock_irqrestore(&ams.irq_lock, flags);
> + }
> +
> + mutex_unlock(&ams.lock);
> +}
Otherwise an IRQ could trigger while the bottom half is
executing and corrupt the ams.worker_irqs field, because it
is modified non-atomically.
--
Greetings Michael.
