lkml.org 
[lkml]   [2012]   [Feb]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: Please add irqdomain branch to linux-next
    On Mon, Feb 06, 2012 at 12:35:11PM +1100, Benjamin Herrenschmidt wrote:
    > On Thu, 2012-02-02 at 14:10 -0700, Grant Likely wrote:
    > > Hi Stephen,
    > >
    > > Can you please add the following branch to linux-next? It contains
    > > the majority of the irqdomain rework that I've been doing. I'd like
    > > to get it marinating in linux-next early so I'm sure it will be ready
    > > when the v3.4 merge window rolls around.
    >
    > Ho !
    >
    > I don't have v4 in my mailbox to reply to the individual patches,
    > but I've spotted some issues so here they are in no specific order.
    >
    > @@ -739,31 +712,36 @@ unsigned int irq_create_mapping(struct irq_domain *host,
    >
    > /* Get a virtual interrupt number */
    > if (host->revmap_type == IRQ_DOMAIN_MAP_LEGACY) {
    > /* Handle legacy */
    > virq = (unsigned int)hwirq;
    > if (virq == 0 || virq >= NUM_ISA_INTERRUPTS)
    > return NO_IRQ;
    > return virq;
    > } else {
    > /* Allocate a virtual interrupt number */
    > hint = hwirq % irq_virq_count;
    > - virq = irq_alloc_virt(host, 1, hint);
    > + virq = irq_alloc_desc_from(hint, 0);
    > + if (!virq)
    > + virq = irq_alloc_desc_from(1, 0);
    > if (virq == NO_IRQ) {
    > pr_debug("irq: -> virq allocation failed\n");
    > return NO_IRQ;
    > }
    >
    > So first, the way you "avoid" allocating irq 0 seems to be by ...
    > allocating irq 0 and then allocating again once you've done that :-)
    >
    > You should either make sure hint is non-0 to begin with or use
    > irq_reserve_irq() to reserve irq 0 (tho I don't know whether the later
    > could be an issue on x86).

    Okay, I'll ensure that hint != 0

    > Also, you no longer honor irq_virq_count. It's a limitation of
    > __irq_alloc_descs() to not be able to get an upper boundary, but you
    > need that for iseries and ps3 at least.

    I'll look at adding an upper limit to __irq_alloc_descs(). If that won't
    work, then I'll add an explicit test after allocation to make sure it is not
    over the limit.

    > Also the default for irq_virq_count should probably be changed when you
    > move to the core to use IRQ_BITMAP_BITS (so we get the 8192 additional
    > irqs on SPARSE_IRQ).

    Good catch.

    >
    > Another thing I noticed, tho I'm still only half way through the series
    > so you -may- have fixed that, is that you allocate all descs on node 0
    > (not even the current node) and have no way to do otherwise.

    No, I've not fixed that.

    > Now, it's a bit of a nasty issue because ideally we should "move" the
    > descs around as we set the affinity of interrupts and we really can't do
    > that just yet, but at least having a way to allocate the desc with a
    > node number (adding a node argument to irq_create_mapping) would be
    > useful. For things like PCI we could make that use the node where the
    > device is, which is better than having everything on node 0.

    okay.

    > Also you should probably make the whole match & xlate business
    > #ifdef CONFIG_OF (especially in the definition of the irq domain). There
    > is no reason why archs couldn't use the domain mapper without
    > device-tree support.

    It builds and runs fine without the CONFIG_OF wrappers, but I can trim stuff
    down.

    > +int irq_domain_xlate_pci(struct irq_domain *d, struct device_node *ctrlr,
    > + const u32 *intspec, unsigned int intsize,
    > + unsigned long *out_hwirq, unsigned int *out_type)
    > +{
    > + if (WARN_ON(intsize != 1))
    > + return -EINVAL;
    > + *out_hwirq = intspec[0];
    > + *out_type = IRQ_TYPE_LEVEL_HIGH;
    > + return 0;
    > +}
    > +EXPORT_SYMBOL_GPL(irq_domain_xlate_pci);
    >
    > That's bogus. PCI interrupts are level -low-. However some bridges
    > internally invert them on the way to the PIC (this is actually common
    > with PCIe bridges where they are generated from messages). So if you are
    > to provide a default helper, make it LEVEL_LOW really.

    Haha, good point. I'll fix that.

    > Overall, I'm not that fan of those helpers... do they really "help" ?
    > IE, Is the call significantly smaller ?

    I think it makes the code a lot easier to read, and it makes it a lot
    easier for a user to know what they are supposed to do I think. The
    build size change wasn't significant either way (but I've lost those
    numbers, I'll need to recalculate them again to give specifics)..

    Thanks for the review.

    g.



    \
     
     \ /
      Last update: 2012-02-06 07:17    [W:0.027 / U:61.380 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site