Re: [PATCH] irq: move some interrupt arch_* functions into struct irq_chip.

Ian Campbell [off-list ref] writes:

On Sat, 2010-03-13 at 00:29 +0000, Eric W. Biederman wrote: 

quoted

[...]

quoted

after that xen could use
irq_to_desc_alloc_node_f(irq, node, xen_init_chip_data);

as need...

at last we don't need to call x86_init_chip_data everywhere.

This was one of the things I was considering. It seems like one of the
easiest solutions to make work correctly with the current locking in
e.g. irq_to_desc_alloc_node since the callback would always happen under
the lock taken in that function.

quoted

So trying to evaluate races.  The worse case for this particular piece
of code appears to be create_irq_nr.  As this is the only place where
we are setting up irqs and possibly repurposing the structure.

Yes, create_irq_nr was one of the functions I was struggling to solve
cleanly. There is a similar construct in the Xen code as well.

Part of the problem I'm having is the combination of lookup and allocate
in irq_to_desc_alloc_node but also the kind of "implicit" repurposing is
tricky to deal with. (by implicit I just mean that I can't find where
the previous user explicitly says they are finished with it, if you see
what I mean)

What do you think of adding an explicit free operation for the irq_desc
structs? (does one already exist? I couldn't find it). This would go
along with some tracking of allocation state, trivial in the sparse case
where you can treat a NULL node in the radix tree as unallocated, I
guess a flag would suffice in the static array non-sparse case?

We have free_one_irq_desc used in the numa irq migration code.
We have dynamic_irq_cleanup.

I don't expect it would be fundamentally hard to put all of the pieces
together.

Going further could we split the alloc and lookup functions into
separate operations instead of combining them in irq_to_desc_alloc_node?
We already have irq_to_desc for the lookup portion so this would largely
involve changes to the semantics of irq_to_desc_alloc_node, perhaps
returning ERR_PTR(-EBUSY) if the node was already allocated.

Having a variant which found a free IRQ rather than operating on a
specific requested IRQ could also be useful for create_irq_nr as well as
find_unbound_irq on the Xen side. I'm not convinced irq_alloc_virt on
powerpc isn't implementing broadly the same concept as well, although it
seems to work very differently from the other two.

quoted

  Today
we figure out if an irq has been assigned by looking at irq_cfg->vector,
and if it is non-zero the irq has been assigned.

Which is tricky to move into generic code hence my suggestions of
explicitly freeing the irq_desc and tracking the allocation status in
the generic code. 

Agreed.  Getting an allocation assist into the generic code sounds
nlike something for the next round of patches after your current one.
At least for msi irqs, hi irqs and other irqs that are totally software
constructs this makes a lot of sense.

We really need to get to the point on x86 where sparse irq has no penalties
and stop making it an option.  Otherwise it gets a bit tricky to rely
on sparse irq for an allocation helper.

I need to go look up my old patchset.  I did something different with
create_irq(), that I think was cleaner and it may be worth resurrecting.
At the very least I generalized the msi case and the ht irq case into
using the same pointer in struct irq_desc.

quoted

The logic in x86_init_chip_data is correct we only assign desc->chip_data
if the generic layers are above it.  However we need a lock to ensure that
two paths don't race in that comparison and that assignment.  There is
no lock in x86_init_chip_data.  Which unfortunately means as it stands
this patchset is buggy.

Yes, unfortunately I think you are right. The callback idea fixes this.
I'll respin with that.

Thanks.

Eric