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
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
