Re: [PATCH] irq/irq_sim: add locking

[Thomas Gleixner <tglx@linutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 1494 bytes --]

On Thu, 8 Nov 2018, Uwe Kleine-König wrote:
> On Thu, Nov 08, 2018 at 05:47:48PM +0100, Bartosz Golaszewski wrote:
> > @@ -74,6 +74,7 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs)
> >  	}
> >  
> >  	init_irq_work(&sim->work_ctx.work, irq_sim_handle_irq);
> > +	mutex_init(&sim->lock);
> >  	sim->irq_count = num_irqs;
> >  
> >  	return sim->irq_base;
> > @@ -142,10 +143,14 @@ EXPORT_SYMBOL_GPL(devm_irq_sim_init);
> >   */
> >  void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
> >  {
> > +	mutex_lock(&sim->lock);
> > +
> >  	if (sim->irqs[offset].enabled) {
> >  		sim->work_ctx.irq = irq_sim_irqnum(sim, offset);
> >  		irq_work_queue(&sim->work_ctx.work);
> >  	}
> > +
> > +	mutex_unlock(&sim->lock);
> 
> This doesn't fix the issue I think. irq_work_queue() only schedules the
> work function. If after irq_sim_fire() returned but before the worker
> runs another irq_sim_fire() is issued the value is still overwritten.

Right. So the obvious solution is to avoid the irq number store and use a
bitfield instead.

struct irq_sim_work_ctx {
       ...
       unsigned long	pending;
};

fire(sim, offset)
{
	if (!sim->irqs[offset].enabled)
		return;

	set_bit(offset, &sim->work_ctx.pending);
	....

and in the work handler do:

handle(work)
{
	struct irq_sim_work_ctx *ctx = container_of(work,....);

	while (ctx->pending) {
		offs = ffs(ctx->pending);
		clr_bit(offs, &ctx->pending);
		handle_simple_irq(offs);
	}
}

Or something like that.

Thanks,

	tglx