Re: [PATCH] powerpc/pseries: Add support for IO Event interrupt drivers

[Michael Ellerman <michael@ellerman.id.au>]

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

On Wed, 2010-05-19 at 20:24 +1000, Mark Nelson wrote:
> On Tuesday 18 May 2010 23:37:31 Michael Ellerman wrote:
> > On Mon, 2010-05-17 at 22:53 +1000, Mark Nelson wrote:

> > 
> > > +	/* check to see if we've already registered this function with
> > > +	 * this scope. If we have, don't register it again
> > > +	 */
> > > +	iter = ioei_isr_list;
> > > +	while (iter) {
> > > +		if (iter->ioei_isr == isr && iter->scope == scope)
> > > +			break;
> > > +		iter = iter->next;
> > > +	}
> > > +
> > > +	if (iter) {
> > > +		ret = -EEXIST;
> > > +		goto out;
> > > +	}
> > > +
> > > +	cons = kmalloc(sizeof(struct ioei_consumer), GFP_KERNEL);
> > 
> > But you don't want to kmalloc while holding the lock and with interrupts
> > off.
> 
> I could allocate above, before taking the lock, and then if we get the
> case where it already exists in the list I could just free it before
> returning. Would that work?

Yeah I think so, optimise for the normal case where it doesn't already
exist. The other option would be to take the lock, check, do the alloc,
retake the lock and recheck - but that's a pain and not really worth the
trouble.

> > > +	ioei_sec = (struct io_events_section *)ch_ptr;
> > > +
> > > +	ioei_call_consumers(ioei_sec->scope, ioei_sec);
> > 
> > Guaranteed to be only one section returned to us per call?
> 
> My understanding is that there's only ever one, but I'll double check.

OK good to check. Could be worth checking in the code, unless it's going
to be really expensive.

> > 
> > We /could/ copy the ioei_sec and drop the buf lock, which would allow
> > another interrupt to come in and start doing the RTAS call (on another
> > cpu, and iff there are actually multiple interrupts). But we probably
> > don't care.
> 
> Good point - I'll update it so that we do the copy.

Sounds like we should. It's not such a concern to call the handlers with
the lock held IMHO (Sonny raised that), as long as the handlers don't
try and register/unregister themselves. But that will be pretty obvious
if it happens.

> > > +		request_event_sources_irqs(np, io_event_interrupt, "IO_EVENT");
> > > +		of_node_put(np);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +device_initcall(init_ioei_IRQ);
> > 
> > Should probably be a machine_initcall().
> 
> I'll change it to machine_device_initcall?

Yep.

> > > Index: upstream/arch/powerpc/platforms/pseries/Makefile
> > > ===================================================================
> > > --- upstream.orig/arch/powerpc/platforms/pseries/Makefile
> > > +++ upstream/arch/powerpc/platforms/pseries/Makefile
> > > @@ -8,7 +8,7 @@ endif
> > >  
> > >  obj-y			:= lpar.o hvCall.o nvram.o reconfig.o \
> > >  			   setup.o iommu.o event_sources.o ras.o \
> > > -			   firmware.o power.o dlpar.o
> > > +			   firmware.o power.o dlpar.o io_events.o
> > 
> > The BML guys might appreciate an option to turn it off?
> 
> I initially had an option that gets selected by PPC_PSERIES, how about
> that?

Select is not great because it disregards dependencies, and BML is
PSERIES. Probably just have an option that depends on PSERIES and is
default y.

cheers


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]