Re: [RFC] Apple Motion Sensor driver

[Michael Buesch]

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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/