Re: [PATCH v1 1/4] max3100: added raise_threaded_irq

[Thomas Gleixner]

On Tue, 23 Mar 2010, Christian Pellegrin wrote:

> raise_threaded_irq schedules the execution of an interrupt thread

I really have a hard time to understand _WHY_ we want to have that
function.

Interrupt threads are woken up either by the primary handler or by a
interrupt demultiplexer and the code has all interfaces for that
already.

Can you please explain, what you are trying to achieve and why it
can't be done with the existing interfaces ?

> +
> +/**
> + *	raise_threaded_irq - triggers a threded interrupt
> + *	@irq: Interrupt line to trigger
> + */
> +int raise_threaded_irq(unsigned int irq)
> +{
> +	struct irq_desc *desc = irq_to_desc(irq);
> +	struct irqaction *action;
> +
> +	if (!desc)
> +		return -ENOENT;
> +	action = desc->action;

That's racy. You cannot access desc->action w/o holding desc->lock or
having set the IRQ_INPROGRESS flag in desc->status under desc->lock.

> +	if (!action)
> +		return -ENOENT;
> +	if (unlikely(!action->thread_fn))
> +		return -EINVAL;
> +	if (likely(!test_bit(IRQTF_DIED,
> +			     &action->thread_flags))) {
> +		set_bit(IRQTF_RUNTHREAD, &action->thread_flags);
> +		wake_up_process(action->thread);
> +	} else {
> +		return -ECHILD;
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL(raise_threaded_irq);

EXPORT_SYMBOL_GPL if at all.

Aside of that the name of of the function sucks: irq_wake_thread()
perhaps ?

But I still have no idea why we would need it at all.

Thanks,

	tglx
--
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/