lkml.org 
[lkml]   [2020]   [Aug]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [patch V2 04/46] genirq/chip: Use the first chip in irq_chip_compose_msi_msg()
    Date
    On Wed, Aug 26 2020 at 20:50, Marc Zyngier wrote:
    > On Wed, 26 Aug 2020 12:16:32 +0100,
    > Thomas Gleixner <tglx@linutronix.de> wrote:
    >> ---
    >> V2: New patch. Note, that this might break other stuff which relies on the
    >> current behaviour, but the hierarchy composition of DT based chips is
    >> really hard to follow.
    >
    > Grepping around, I don't think there is any occurrence of two irqchips
    > providing irq_compose_msi() that can share a hierarchy on any real
    > system, so we should be fine. Famous last words.

    Knocking on wood :)

    >> #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
    >> - for (; data; data = data->parent_data)
    >> -#endif
    >> - if (data->chip && data->chip->irq_compose_msi_msg)
    >> + for (; data; data = data->parent_data) {
    >> + if (data->chip && data->chip->irq_compose_msi_msg) {
    >> pos = data;
    >> + break;
    >> + }
    >> + }
    >> +#else
    >> + if (data->chip && data->chip->irq_compose_msi_msg)
    >> + pos = data;
    >> +#endif
    >> if (!pos)
    >> return -ENOSYS;
    >
    > Is it just me, or is this last change more complex than it ought to
    > be?

    Kinda.

    > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
    > index 857f5f4c8098..25e18b73699c 100644
    > --- a/kernel/irq/chip.c
    > +++ b/kernel/irq/chip.c
    > @@ -1544,7 +1544,7 @@ int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
    > struct irq_data *pos = NULL;
    >
    > #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
    > - for (; data; data = data->parent_data)
    > + for (; data && !pos; data = data->parent_data)
    > #endif
    > if (data->chip && data->chip->irq_compose_msi_msg)
    > pos = data;
    >
    > Though the for loop in a #ifdef in admittedly an acquired taste...

    Checking !pos is simpler obviously. That doesn't make me hate the loop
    in the #ifdef less. :)

    What about the below?

    Thanks,

    tglx
    ---
    --- a/kernel/irq/internals.h
    +++ b/kernel/irq/internals.h
    @@ -473,6 +473,15 @@ static inline void irq_domain_deactivate
    }
    #endif

    +static inline struct irq_data *irqd_get_parent_data(struct irq_data *irqd)
    +{
    +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
    + return irqd->parent_data;
    +#else
    + return NULL;
    +#endif
    +}
    +
    #ifdef CONFIG_GENERIC_IRQ_DEBUGFS
    #include <linux/debugfs.h>

    --- a/kernel/irq/chip.c
    +++ b/kernel/irq/chip.c
    @@ -1541,18 +1541,17 @@ EXPORT_SYMBOL_GPL(irq_chip_release_resou
    */
    int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
    {
    - struct irq_data *pos = NULL;
    + struct irq_data *pos;

    -#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
    - for (; data; data = data->parent_data)
    -#endif
    + for (pos = NULL; !pos && data; data = irqd_get_parent_data(data)) {
    if (data->chip && data->chip->irq_compose_msi_msg)
    pos = data;
    + }
    +
    if (!pos)
    return -ENOSYS;

    pos->chip->irq_compose_msi_msg(pos, msg);
    -
    return 0;
    }

    \
     
     \ /
      Last update: 2020-08-26 23:21    [W:4.316 / U:0.164 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site