[PATCH] handle failure of irqchip->set_type in setup_irq - Kernel

This is a discussion on [PATCH] handle failure of irqchip->set_type in setup_irq - Kernel ; set_type returns an int but currently setup_irq ignores that. To save me from undoing some changes setting the IRQ_NO_BALANCING bit in desc->flags, setting IRQ_PER_CPU in desc->status and adding the new action to desc is only done after set_type succeeded. Signed-off-by: ...

+ Reply to Thread
Page 1 of 2 1 2 LastLast
Results 1 to 20 of 34

Thread: [PATCH] handle failure of irqchip->set_type in setup_irq

  1. [PATCH] handle failure of irqchip->set_type in setup_irq

    set_type returns an int but currently setup_irq ignores that.

    To save me from undoing some changes setting the IRQ_NO_BALANCING bit in
    desc->flags, setting IRQ_PER_CPU in desc->status and adding the new
    action to desc is only done after set_type succeeded.

    Signed-off-by: Uwe Kleine-König
    ---
    Hello,

    in my case set_type returns an error because gpio-key tries to use
    IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING and my irq-chip can only do
    one of these.

    Best regards
    Uwe

    kernel/irq/manage.c | 38 ++++++++++++++++++++++++--------------
    1 files changed, 24 insertions(+), 14 deletions(-)

    diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
    index 46d6611..bc990a1 100644
    --- a/kernel/irq/manage.c
    +++ b/kernel/irq/manage.c
    @@ -281,6 +281,7 @@ int setup_irq(unsigned int irq, struct irqaction *new)
    const char *old_name = NULL;
    unsigned long flags;
    int shared = 0;
    + int ret;

    if (irq >= NR_IRQS)
    return -EINVAL;
    @@ -338,26 +339,23 @@ int setup_irq(unsigned int irq, struct irqaction *new)
    shared = 1;
    }

    - *p = new;
    -
    - /* Exclude IRQ from balancing */
    - if (new->flags & IRQF_NOBALANCING)
    - desc->status |= IRQ_NO_BALANCING;
    -
    if (!shared) {
    irq_chip_set_defaults(desc->chip);

    -#if defined(CONFIG_IRQ_PER_CPU)
    - if (new->flags & IRQF_PERCPU)
    - desc->status |= IRQ_PER_CPU;
    -#endif
    -
    /* Setup the type (level, edge polarity) if configured: */
    if (new->flags & IRQF_TRIGGER_MASK) {
    - if (desc->chip && desc->chip->set_type)
    - desc->chip->set_type(irq,
    + if (desc->chip && desc->chip->set_type) {
    + ret = desc->chip->set_type(irq,
    new->flags & IRQF_TRIGGER_MASK);
    - else
    + if (ret) {
    + pr_err("setting flow type for irq %u "
    + "failed\n", irq);
    + spin_unlock_irqrestore(&desc->lock,
    + flags);
    + return ret;
    + }
    +
    + } else
    /*
    * IRQF_TRIGGER_* but the PIC does not support
    * multiple flow-types?
    @@ -369,6 +367,11 @@ int setup_irq(unsigned int irq, struct irqaction *new)
    } else
    compat_irq_chip_set_default_handler(desc);

    +#if defined(CONFIG_IRQ_PER_CPU)
    + if (new->flags & IRQF_PERCPU)
    + desc->status |= IRQ_PER_CPU;
    +#endif
    +
    desc->status &= ~(IRQ_AUTODETECT | IRQ_WAITING |
    IRQ_INPROGRESS | IRQ_SPURIOUS_DISABLED);

    @@ -383,6 +386,13 @@ int setup_irq(unsigned int irq, struct irqaction *new)
    /* Undo nested disables: */
    desc->depth = 1;
    }
    +
    + *p = new;
    +
    + /* Exclude IRQ from balancing */
    + if (new->flags & IRQF_NOBALANCING)
    + desc->status |= IRQ_NO_BALANCING;
    +
    /* Reset broken irq detection when installing new handler */
    desc->irq_count = 0;
    desc->irqs_unhandled = 0;
    --
    1.5.6


    --
    Uwe Kleine-König, Software Engineer
    Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
    Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH] handle failure of irqchip->set_type in setup_irq

    Hello,

    [extending the Cc: list]

    Uwe Kleine-König wrote:
    > set_type returns an int but currently setup_irq ignores that.
    >
    > To save me from undoing some changes setting the IRQ_NO_BALANCING bit in
    > desc->flags, setting IRQ_PER_CPU in desc->status and adding the new
    > action to desc is only done after set_type succeeded.
    >
    > Signed-off-by: Uwe Kleine-König
    > ---
    > Hello,
    >
    > in my case set_type returns an error because gpio-key tries to use
    > IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING and my irq-chip can only do
    > one of these.


    up to now I didn't get any response for this patch. It addresses a real
    problem for my platform so I really like to have a fix in.

    In mmotm of 2008-07-01-21-57 kernel/irq/manage.c is touched[1],
    too, but git was able to automerge the change correctly when applying it
    on top of mmotm.

    For your convenience you can find the patch again at the end of this
    mail.

    Andrew: Do you can take this patch into mm?

    Best regards
    Uwe

    [1] kernel/irq/manage.c is modified by linux-next patch. The relevant
    commit is:
    1840475... genirq: Expose default irq affinity mask (take 3)


    > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
    > index 46d6611..bc990a1 100644
    > --- a/kernel/irq/manage.c
    > +++ b/kernel/irq/manage.c
    > @@ -281,6 +281,7 @@ int setup_irq(unsigned int irq, struct irqaction *new)
    > const char *old_name = NULL;
    > unsigned long flags;
    > int shared = 0;
    > + int ret;
    >
    > if (irq >= NR_IRQS)
    > return -EINVAL;
    > @@ -338,26 +339,23 @@ int setup_irq(unsigned int irq, struct irqaction *new)
    > shared = 1;
    > }
    >
    > - *p = new;
    > -
    > - /* Exclude IRQ from balancing */
    > - if (new->flags & IRQF_NOBALANCING)
    > - desc->status |= IRQ_NO_BALANCING;
    > -
    > if (!shared) {
    > irq_chip_set_defaults(desc->chip);
    >
    > -#if defined(CONFIG_IRQ_PER_CPU)
    > - if (new->flags & IRQF_PERCPU)
    > - desc->status |= IRQ_PER_CPU;
    > -#endif
    > -
    > /* Setup the type (level, edge polarity) if configured: */
    > if (new->flags & IRQF_TRIGGER_MASK) {
    > - if (desc->chip && desc->chip->set_type)
    > - desc->chip->set_type(irq,
    > + if (desc->chip && desc->chip->set_type) {
    > + ret = desc->chip->set_type(irq,
    > new->flags & IRQF_TRIGGER_MASK);
    > - else
    > + if (ret) {
    > + pr_err("setting flow type for irq %u "
    > + "failed\n", irq);
    > + spin_unlock_irqrestore(&desc->lock,
    > + flags);
    > + return ret;
    > + }
    > +
    > + } else
    > /*
    > * IRQF_TRIGGER_* but the PIC does not support
    > * multiple flow-types?
    > @@ -369,6 +367,11 @@ int setup_irq(unsigned int irq, struct irqaction *new)
    > } else
    > compat_irq_chip_set_default_handler(desc);
    >
    > +#if defined(CONFIG_IRQ_PER_CPU)
    > + if (new->flags & IRQF_PERCPU)
    > + desc->status |= IRQ_PER_CPU;
    > +#endif
    > +
    > desc->status &= ~(IRQ_AUTODETECT | IRQ_WAITING |
    > IRQ_INPROGRESS | IRQ_SPURIOUS_DISABLED);
    >
    > @@ -383,6 +386,13 @@ int setup_irq(unsigned int irq, struct irqaction *new)
    > /* Undo nested disables: */
    > desc->depth = 1;
    > }
    > +
    > + *p = new;
    > +
    > + /* Exclude IRQ from balancing */
    > + if (new->flags & IRQF_NOBALANCING)
    > + desc->status |= IRQ_NO_BALANCING;
    > +
    > /* Reset broken irq detection when installing new handler */
    > desc->irq_count = 0;
    > desc->irqs_unhandled = 0;


    --
    Uwe Kleine-König, Software Engineer
    Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
    Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH] handle failure of irqchip->set_type in setup_irq

    On Wed, 2 Jul 2008 11:17:57 +0200 Uwe Kleine-K__nig wrote:

    > Hello,
    >
    > [extending the Cc: list]
    >
    > Uwe Kleine-K__nig wrote:
    > > set_type returns an int but currently setup_irq ignores that.
    > >
    > > To save me from undoing some changes setting the IRQ_NO_BALANCING bit in
    > > desc->flags, setting IRQ_PER_CPU in desc->status and adding the new
    > > action to desc is only done after set_type succeeded.
    > >
    > > Signed-off-by: Uwe Kleine-K__nig
    > > ---
    > > Hello,
    > >
    > > in my case set_type returns an error because gpio-key tries to use
    > > IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING and my irq-chip can only do
    > > one of these.

    >
    > up to now I didn't get any response for this patch. It addresses a real
    > problem for my platform so I really like to have a fix in.
    >
    > In mmotm of 2008-07-01-21-57 kernel/irq/manage.c is touched[1],
    > too, but git was able to automerge the change correctly when applying it
    > on top of mmotm.
    >
    > For your convenience you can find the patch again at the end of this
    > mail.
    >
    > Andrew: Do you can take this patch into mm?
    >


    From a brief squint the patch seems to be reasonable. But the
    changelog is a bit mangled. Perhaps you could have another go when
    resending it. Explan more clearly under what circumststances your
    ->set_type() implementation can fail and why you require the core code
    to handle this.

    Perhaps we want a dump_stack() on the error path so we can see who
    goofed. Or a print_symbol() of desc->chip->set_type. Or perhaps not.

    Did you check that all the current ->set_type() implementations are
    returning zero?


    >
    > [1] kernel/irq/manage.c is modified by linux-next patch. The relevant
    > commit is:
    > 1840475... genirq: Expose default irq affinity mask (take 3)
    >
    >
    > > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
    > > index 46d6611..bc990a1 100644
    > > --- a/kernel/irq/manage.c
    > > +++ b/kernel/irq/manage.c
    > > @@ -281,6 +281,7 @@ int setup_irq(unsigned int irq, struct irqaction *new)
    > > const char *old_name = NULL;
    > > unsigned long flags;
    > > int shared = 0;
    > > + int ret;
    > >
    > > if (irq >= NR_IRQS)
    > > return -EINVAL;
    > > @@ -338,26 +339,23 @@ int setup_irq(unsigned int irq, struct irqaction *new)
    > > shared = 1;
    > > }
    > >
    > > - *p = new;
    > > -
    > > - /* Exclude IRQ from balancing */
    > > - if (new->flags & IRQF_NOBALANCING)
    > > - desc->status |= IRQ_NO_BALANCING;
    > > -
    > > if (!shared) {
    > > irq_chip_set_defaults(desc->chip);
    > >
    > > -#if defined(CONFIG_IRQ_PER_CPU)
    > > - if (new->flags & IRQF_PERCPU)
    > > - desc->status |= IRQ_PER_CPU;
    > > -#endif
    > > -
    > > /* Setup the type (level, edge polarity) if configured: */
    > > if (new->flags & IRQF_TRIGGER_MASK) {
    > > - if (desc->chip && desc->chip->set_type)
    > > - desc->chip->set_type(irq,
    > > + if (desc->chip && desc->chip->set_type) {
    > > + ret = desc->chip->set_type(irq,
    > > new->flags & IRQF_TRIGGER_MASK);
    > > - else
    > > + if (ret) {
    > > + pr_err("setting flow type for irq %u "
    > > + "failed\n", irq);
    > > + spin_unlock_irqrestore(&desc->lock,
    > > + flags);
    > > + return ret;
    > > + }
    > > +
    > > + } else
    > > /*
    > > * IRQF_TRIGGER_* but the PIC does not support
    > > * multiple flow-types?
    > > @@ -369,6 +367,11 @@ int setup_irq(unsigned int irq, struct irqaction *new)
    > > } else
    > > compat_irq_chip_set_default_handler(desc);
    > >
    > > +#if defined(CONFIG_IRQ_PER_CPU)
    > > + if (new->flags & IRQF_PERCPU)
    > > + desc->status |= IRQ_PER_CPU;
    > > +#endif
    > > +
    > > desc->status &= ~(IRQ_AUTODETECT | IRQ_WAITING |
    > > IRQ_INPROGRESS | IRQ_SPURIOUS_DISABLED);
    > >
    > > @@ -383,6 +386,13 @@ int setup_irq(unsigned int irq, struct irqaction *new)
    > > /* Undo nested disables: */
    > > desc->depth = 1;
    > > }
    > > +
    > > + *p = new;
    > > +
    > > + /* Exclude IRQ from balancing */
    > > + if (new->flags & IRQF_NOBALANCING)
    > > + desc->status |= IRQ_NO_BALANCING;
    > > +
    > > /* Reset broken irq detection when installing new handler */
    > > desc->irq_count = 0;
    > > desc->irqs_unhandled = 0;


    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: [PATCH] handle failure of irqchip->set_type in setup_irq

    Hello Andrew,

    Andrew Morton wrote:
    > On Wed, 2 Jul 2008 11:17:57 +0200 Uwe Kleine-K__nig wrote:
    >
    > > Hello,
    > >
    > > [extending the Cc: list]
    > >
    > > Uwe Kleine-K__nig wrote:
    > > > set_type returns an int but currently setup_irq ignores that.
    > > >
    > > > To save me from undoing some changes setting the IRQ_NO_BALANCING bit in
    > > > desc->flags, setting IRQ_PER_CPU in desc->status and adding the new
    > > > action to desc is only done after set_type succeeded.
    > > >
    > > > Signed-off-by: Uwe Kleine-K__nig
    > > > ---
    > > > Hello,
    > > >
    > > > in my case set_type returns an error because gpio-key tries to use
    > > > IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING and my irq-chip can only do
    > > > one of these.

    > >
    > > up to now I didn't get any response for this patch. It addresses a real
    > > problem for my platform so I really like to have a fix in.
    > >
    > > In mmotm of 2008-07-01-21-57 kernel/irq/manage.c is touched[1],
    > > too, but git was able to automerge the change correctly when applying it
    > > on top of mmotm.
    > >
    > > For your convenience you can find the patch again at the end of this
    > > mail.
    > >
    > > Andrew: Do you can take this patch into mm?
    > >

    >
    > >From a brief squint the patch seems to be reasonable. But the

    > changelog is a bit mangled. Perhaps you could have another go when
    > resending it. Explan more clearly under what circumststances your
    > ->set_type() implementation can fail and why you require the core code
    > to handle this.

    OK.

    > Perhaps we want a dump_stack() on the error path so we can see who
    > goofed. Or a print_symbol() of desc->chip->set_type. Or perhaps not.

    I thought about a dump_stack() but considered it as too much.
    print_symbol() is a nice idea though.

    > Did you check that all the current ->set_type() implementations are
    > returning zero?

    No, I don't. But for example orion5x_gpio_set_irq_type might return
    -EINVAL. txx9_irq_set_type seems to have the same limitation as ns9xxx
    and returns -EINVAL if more than one trigger type is requested.

    Best regards
    Uwe

    --
    Uwe Kleine-König, Software Engineer
    Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
    Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. [PATCH v2] handle failure of irqchip->set_type in setup_irq

    set_type returns an int indicating success or failure, but up to now
    setup_irq ignores that.

    In my case this resulted in a machine hang:
    gpio-keys requested IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, but
    arm/ns9xxx can only trigger on one direction so set_type didn't touch
    the configuration which happens do default to a level sensitiveness and
    returned -EINVAL. setup_irq ignored that and unmasked the irq. This
    resulted in an endless triggering of the gpio-key interrupt service
    routine which effectively killed the machine.

    With this patch applied setup_irq propagates the error to the caller.

    Note that before in the case

    chip && !chip->set_type && !chip->name

    a NULL pointer was feed to printk. This is fixed, too.

    Signed-off-by: Uwe Kleine-König
    ---
    Hello,

    Changes since initial post:

    - improve commit log (hopefully)
    - move code to a dedicated function to improve readability and code
    line length.
    - include the symbolic name of the failing callback in the error
    message.

    Best regards
    Uwe

    kernel/irq/manage.c | 72 ++++++++++++++++++++++++++++++++++----------------
    1 files changed, 49 insertions(+), 23 deletions(-)

    diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
    index 46d6611..178b966 100644
    --- a/kernel/irq/manage.c
    +++ b/kernel/irq/manage.c
    @@ -270,6 +270,35 @@ void compat_irq_chip_set_default_handler(struct irq_desc *desc)
    desc->handle_irq = NULL;
    }

    +static int __irq_set_trigger(struct irq_chip *chip, unsigned int irq,
    + unsigned long flags)
    +{
    + int ret;
    +
    + if (!chip || !chip->set_type) {
    + /*
    + * IRQF_TRIGGER_* but the PIC does not support multiple
    + * flow-types?
    + */
    + pr_warning("No set_type function for IRQ %d (%s)\n", irq,
    + chip ? (chip->name ? : "unknown") : "unknown");
    + return 0;
    + }
    +
    + ret = chip->set_type(irq, flags & IRQF_TRIGGER_MASK);
    +
    + if (ret) {
    + char buf[100];
    +
    + snprintf(buf, sizeof(buf), KERN_ERR
    + "setting flow type for irq %u failed (%%s)\n",
    + irq);
    + print_fn_descriptor_symbol(buf, chip->set_type);
    + }
    +
    + return ret;
    +}
    +
    /*
    * Internal function to register an irqaction - typically used to
    * allocate special interrupts that are part of the architecture.
    @@ -281,6 +310,7 @@ int setup_irq(unsigned int irq, struct irqaction *new)
    const char *old_name = NULL;
    unsigned long flags;
    int shared = 0;
    + int ret;

    if (irq >= NR_IRQS)
    return -EINVAL;
    @@ -338,37 +368,26 @@ int setup_irq(unsigned int irq, struct irqaction *new)
    shared = 1;
    }

    - *p = new;
    -
    - /* Exclude IRQ from balancing */
    - if (new->flags & IRQF_NOBALANCING)
    - desc->status |= IRQ_NO_BALANCING;
    -
    if (!shared) {
    irq_chip_set_defaults(desc->chip);

    -#if defined(CONFIG_IRQ_PER_CPU)
    - if (new->flags & IRQF_PERCPU)
    - desc->status |= IRQ_PER_CPU;
    -#endif
    -
    /* Setup the type (level, edge polarity) if configured: */
    if (new->flags & IRQF_TRIGGER_MASK) {
    - if (desc->chip && desc->chip->set_type)
    - desc->chip->set_type(irq,
    - new->flags & IRQF_TRIGGER_MASK);
    - else
    - /*
    - * IRQF_TRIGGER_* but the PIC does not support
    - * multiple flow-types?
    - */
    - printk(KERN_WARNING "No IRQF_TRIGGER set_type "
    - "function for IRQ %d (%s)\n", irq,
    - desc->chip ? desc->chip->name :
    - "unknown");
    + ret = __irq_set_trigger(desc->chip, irq, new->flags);
    +
    + if (ret) {
    + spin_unlock_irqrestore(&desc->lock, flags);
    + return ret;
    + }
    +
    } else
    compat_irq_chip_set_default_handler(desc);

    +#if defined(CONFIG_IRQ_PER_CPU)
    + if (new->flags & IRQF_PERCPU)
    + desc->status |= IRQ_PER_CPU;
    +#endif
    +
    desc->status &= ~(IRQ_AUTODETECT | IRQ_WAITING |
    IRQ_INPROGRESS | IRQ_SPURIOUS_DISABLED);

    @@ -383,6 +402,13 @@ int setup_irq(unsigned int irq, struct irqaction *new)
    /* Undo nested disables: */
    desc->depth = 1;
    }
    +
    + *p = new;
    +
    + /* Exclude IRQ from balancing */
    + if (new->flags & IRQF_NOBALANCING)
    + desc->status |= IRQ_NO_BALANCING;
    +
    /* Reset broken irq detection when installing new handler */
    desc->irq_count = 0;
    desc->irqs_unhandled = 0;
    --
    1.5.6

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: [PATCH v2] handle failure of irqchip->set_type in setup_irq

    On Fri, 4 Jul 2008 12:46:34 +0200 Uwe Kleine-K__nig wrote:

    > set_type returns an int indicating success or failure, but up to now
    > setup_irq ignores that.
    >
    > In my case this resulted in a machine hang:
    > gpio-keys requested IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, but
    > arm/ns9xxx can only trigger on one direction so set_type didn't touch
    > the configuration which happens do default to a level sensitiveness and
    > returned -EINVAL. setup_irq ignored that and unmasked the irq. This
    > resulted in an endless triggering of the gpio-key interrupt service
    > routine which effectively killed the machine.
    >
    > With this patch applied setup_irq propagates the error to the caller.
    >
    > Note that before in the case
    >
    > chip && !chip->set_type && !chip->name
    >
    > a NULL pointer was feed to printk. This is fixed, too.
    >


    hm, OK. Do I recall there being some urgency to this?

    > + if (ret) {
    > + char buf[100];
    > +
    > + snprintf(buf, sizeof(buf), KERN_ERR
    > + "setting flow type for irq %u failed (%%s)\n",
    > + irq);
    > + print_fn_descriptor_symbol(buf, chip->set_type);
    > + }


    eww. We really mucked up that interface. I wonder if we can do better.
    Let me think about that.


    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: [PATCH v2] handle failure of irqchip->set_type in setup_irq

    Hello,

    Andrew Morton wrote:
    > On Fri, 4 Jul 2008 12:46:34 +0200 Uwe Kleine-K__nig wrote:
    >
    > > set_type returns an int indicating success or failure, but up to now
    > > setup_irq ignores that.
    > >
    > > In my case this resulted in a machine hang:
    > > gpio-keys requested IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, but
    > > arm/ns9xxx can only trigger on one direction so set_type didn't touch
    > > the configuration which happens do default to a level sensitiveness and
    > > returned -EINVAL. setup_irq ignored that and unmasked the irq. This
    > > resulted in an endless triggering of the gpio-key interrupt service
    > > routine which effectively killed the machine.
    > >
    > > With this patch applied setup_irq propagates the error to the caller.
    > >
    > > Note that before in the case
    > >
    > > chip && !chip->set_type && !chip->name
    > >
    > > a NULL pointer was feed to printk. This is fixed, too.
    > >

    >
    > hm, OK. Do I recall there being some urgency to this?

    No, I didn't mention this before. (Fixing NULL-Pointers feed to printk
    is a bit of a reflex for me as I used to program for Solaris and if you
    feed a NULL-Pointer to printf there your program segfaults.) I didn't
    check that recently, but IIRC it's no problem here.

    >
    > > + if (ret) {
    > > + char buf[100];
    > > +
    > > + snprintf(buf, sizeof(buf), KERN_ERR
    > > + "setting flow type for irq %u failed (%%s)\n",
    > > + irq);
    > > + print_fn_descriptor_symbol(buf, chip->set_type);
    > > + }

    >
    > eww. We really mucked up that interface.

    That's what I thought, too. ;-) This was the nicest way I found to
    print the whole line in one go.

    > I wonder if we can do better.
    > Let me think about that.

    I was about to suggest something like:

    /* WARNING: this returns a static pointer, so you cannot use the
    * returned value after another call to creative_function_name
    */
    char *creative_function_name(void *addr)
    {
    static char buf[SOME_LENGTH];

    ... format symbol name into buf ...

    return buf;
    }

    Then I could have used

    pr_err("setting flow type for irq %u failed (%s)\n",
    irq, creative_function_name(chip->set_type));

    which looks definitely nicer.

    Thanks for taking the patch.

    Best regards
    Uwe

    --
    Uwe Kleine-König, Software Engineer
    Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
    Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  8. Re: [PATCH v2] handle failure of irqchip->set_type in setup_irq

    On Fri, 4 Jul 2008 20:43:07 +0200 Uwe Kleine-K__nig wrote:

    > > I wonder if we can do better.
    > > Let me think about that.

    > I was about to suggest something like:
    >
    > /* WARNING: this returns a static pointer, so you cannot use the
    > * returned value after another call to creative_function_name
    > */
    > char *creative_function_name(void *addr)
    > {
    > static char buf[SOME_LENGTH];
    >
    > ... format symbol name into buf ...
    >
    > return buf;
    > }
    >
    > Then I could have used
    >
    > pr_err("setting flow type for irq %u failed (%s)\n",
    > irq, creative_function_name(chip->set_type));
    >
    > which looks definitely nicer.


    Better would be

    char buf[ENOUGH /* rofl */];

    pr_err("setting flow type for irq %u failed (%s)\n",
    irq, render_function_name(buf, chip->set_type));

    However I'm presently brewing up a plot to do this:

    printk("function name is %Ss\n", (unsigned long *)chip->set_type);

    which I think will work. We can also do

    printk("IP address is %Si\n", (unsigned long *)ip_address_buffer);

    to replace NIPQUAD. And similar printk extensions.

    Am still thinking about it though.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  9. Re: the printk problem


    (heck, let's cc lkml - avoid having to go through all this again)

    On Fri, 4 Jul 2008 14:42:53 -0600 Matthew Wilcox wrote:

    > On Fri, Jul 04, 2008 at 01:27:16PM -0700, Andrew Morton wrote:
    > > On Fri, 4 Jul 2008 13:02:05 -0700 (PDT) Linus Torvalds wrote:
    > > > > so I think we could easily just say that we extend %p in various ways:
    > > > >
    > > > > - %pS - print pointer as a symbol
    > > > >
    > > > > and leave tons of room for future extensions for different kinds of
    > > > > pointers.

    > >
    > > If this takes off we might want a register-your-printk-handler
    > > interface. Maybe.
    > >
    > > We also jump through hoops to print things like sector_t and
    > > resource_size_t. They always need to be cast to `unsiged long long',
    > > which generates additional stack space and text in some setups.

    >
    > The thing is that GCC checks types. So it's fine to add "print this
    > pointer specially", but you can't in general add new printf arguments
    > without also hacking GCC. Unless you use -Wno-format, and require
    > sparse to check special kernel types.


    It would be excellent if gcc had an extension system so that you could
    add new printf control chars and maybe even tell gcc how to check them.
    But of course, if that were to happen, we couldn't use it for 4-5 years.

    What I had initially proposed was to abuse %S, which takes a wchar_t*.
    gcc accepts `unsigned long *' for %S.

    Then, we put the kernel-specific control char after the S, so we can
    print an inode (rofl) with

    struct inode *inode;

    printk("here is an inode: %Si\n", (unsigned long *)inode);

    Downsides are:

    - there's a cast, so you could accidentally do

    printk("here is an inode: %Si\n", (unsigned long *)dentry);

    - there's a cast, and they're ugly

    - gcc cannot of course check that the arg matches the control string

    Unfortunately (and this seems weird), gcc printf checking will not
    accept a void* for %S: it _has_ to be wchar_t*, and the checker won't
    permit void* substitution for that.

    Anyway, Linus took all that and said "let's abuse %p instead". It
    _will_ accept any pointer so we can instead do:

    printk("here is an inode: %pi\n", inode);

    which is nicer.


    I think the main customers of this are print_symbol():

    printk("I am about to call %ps\n", fn);
    (*fn)();

    and NIPQUAD and its ipv6 version.

    We don't know how much interest there would be in churning NIPQUAD from
    the net guys. Interestingly, there's also %C (wint_t) which is a
    32-bit quantity. So we could just go and say "%C prints an ipv4
    address" and be done with it. But there's no way of doing that for
    ipv6 addresses so things would become asymmetrical there.

    Another customer is net mac addresses. There are surely others. One
    which should have been in printf 30 years ago was %b: binary.


    > > And then there's the perennial "need to cast u64 to unsigned long long
    > > to print it". If we were to do
    > >
    > > printk("%SL", (void *)some_u64);
    > >
    > > then that's still bloody ugly, but it'll save a little text-n-stack.

    >
    > u64 is easy to fix, and I don't know why we haven't. Just make it
    > unsigned long long on all architectures.


    Yeah. Why don't we do that?
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  10. Re: the printk problem

    On Fri, Jul 04, 2008 at 03:01:00PM -0700, Andrew Morton wrote:
    > (heck, let's cc lkml - avoid having to go through all this again)
    >
    > It would be excellent if gcc had an extension system so that you could
    > add new printf control chars and maybe even tell gcc how to check them.
    > But of course, if that were to happen, we couldn't use it for 4-5 years.


    I believe NetBSD added that as an extension many years ago. Dunno if
    they still have it.

    > What I had initially proposed was to abuse %S, which takes a wchar_t*.
    > gcc accepts `unsigned long *' for %S.
    >

    [...]
    > - there's a cast, so you could accidentally do
    >
    > printk("here is an inode: %Si\n", (unsigned long *)dentry);
    >
    > - there's a cast, and they're ugly
    >
    > - gcc cannot of course check that the arg matches the control string
    >
    > Unfortunately (and this seems weird), gcc printf checking will not
    > accept a void* for %S: it _has_ to be wchar_t*, and the checker won't
    > permit void* substitution for that.


    Presumably that's the compiler getting rid of the first and third
    downside ;-)

    > Anyway, Linus took all that and said "let's abuse %p instead". It
    > _will_ accept any pointer so we can instead do:
    >
    > printk("here is an inode: %pi\n", inode);
    >
    > which is nicer.


    Yes. It's possible to confuse it, of course.

    printk("Function %pSucks\n", sys_open);

    but I really doubt we have such a usage in the kernel today.

    > > u64 is easy to fix, and I don't know why we haven't. Just make it
    > > unsigned long long on all architectures.

    >
    > Yeah. Why don't we do that?


    Patch ...

    [PATCH] Make u64 long long on all architectures

    It is currently awkward to print a u64 type. Some architectures use
    unsigned long while others use unsigned long long. Since unsigned long
    long is 64-bit for all existing Linux architectures, change those that
    use long to use long long. Note that this applies only within the
    kernel. If u64 is being used in a C++ method definition, the symbol
    mangling would change.

    Signed-off-by: Matthew Wilcox

    diff --git a/include/asm-generic/int-l64.h b/include/asm-generic/int-l64.h
    index 2af9b75..32f07bd 100644
    --- a/include/asm-generic/int-l64.h
    +++ b/include/asm-generic/int-l64.h
    @@ -23,8 +23,13 @@ typedef unsigned short __u16;
    typedef __signed__ int __s32;
    typedef unsigned int __u32;

    +#ifdef __KERNEL__
    +typedef __signed__ long long __s64;
    +typedef unsigned long long __u64;
    +#else
    typedef __signed__ long __s64;
    typedef unsigned long __u64;
    +#endif

    #endif /* __ASSEMBLY__ */


    --
    Intel are signing my paycheques ... these opinions are still mine
    "Bill, look, we understand that you're interested in selling us this
    operating system, but compare it to ours. We can't possibly take such
    a retrograde step."
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  11. Re: the printk problem

    On Saturday 05 July 2008 00:01, Andrew Morton wrote:
    > > > We also jump through hoops to print things like sector_t and
    > > > resource_size_t. They always need to be cast to `unsiged long long',
    > > > which generates additional stack space and text in some setups.

    > >
    > > The thing is that GCC checks types. So it's fine to add "print this
    > > pointer specially", but you can't in general add new printf arguments
    > > without also hacking GCC. Unless you use -Wno-format, and require
    > > sparse to check special kernel types.

    >
    > It would be excellent if gcc had an extension system so that you could
    > add new printf control chars and maybe even tell gcc how to check them.
    > But of course, if that were to happen, we couldn't use it for 4-5 years.
    >
    > What I had initially proposed was to abuse %S, which takes a wchar_t*.
    > gcc accepts `unsigned long *' for %S.
    >
    > Then, we put the kernel-specific control char after the S, so we can
    > print an inode (rofl) with
    >
    > struct inode *inode;
    >
    > printk("here is an inode: %Si\n", (unsigned long *)inode);
    >
    > Downsides are:
    >
    > - there's a cast, so you could accidentally do
    >
    > printk("here is an inode: %Si\n", (unsigned long *)dentry);
    >
    > - there's a cast, and they're ugly
    >
    > - gcc cannot of course check that the arg matches the control string
    >
    > Unfortunately (and this seems weird), gcc printf checking will not
    > accept a void* for %S: it _has_ to be wchar_t*, and the checker won't
    > permit void* substitution for that.


    Repeating myself here...
    We can add an alternative alias to printk:

    asmlinkage int printk(const char * fmt, ...)
    __attribute__ ((format (printf, 1, 2))) __cold;
    +asmlinkage int custom_printk(const char * fmt, ...) __cold asm ("printk");

    custom_printk() is actually just printk(), that is,
    we won't need additional function, we need to teach
    *printk* about MAC addresses, NIPQUADs etc;

    and then use printk() if you use only standard %fmt (and have it
    checked by gcc), or use custom_printk() if you have non-standard
    %fmt in the format string.

    The only downside that in second case, you lose gcc checking.
    No big deal.
    --
    vda
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  12. Re: the printk problem


    On Saturday 2008-07-05 00:01, Andrew Morton wrote:
    >
    >We don't know how much interest there would be in churning NIPQUAD from
    >the net guys. Interestingly, there's also %C (wint_t) which is a
    >32-bit quantity. So we could just go and say "%C prints an ipv4
    >address" and be done with it. But there's no way of doing that for
    >ipv6 addresses so things would become asymmetrical there.


    struct in6_addr src;
    printk("Source address: %p{ipv6}\n", &src);

    How about %p{feature}?
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  13. Re: the printk problem

    On Sat, Jul 5, 2008 at 1:33 PM, Jan Engelhardt wrote:
    >
    > On Saturday 2008-07-05 00:01, Andrew Morton wrote:
    >>
    >>We don't know how much interest there would be in churning NIPQUAD from
    >>the net guys. Interestingly, there's also %C (wint_t) which is a
    >>32-bit quantity. So we could just go and say "%C prints an ipv4
    >>address" and be done with it. But there's no way of doing that for
    >>ipv6 addresses so things would become asymmetrical there.

    >
    > struct in6_addr src;
    > printk("Source address: %p{ipv6}\n", &src);
    >
    > How about %p{feature}?


    Something like this?

    (It's hard on the stack, yes, I know. We should fix kallsyms.)

    Vegard


    From: Vegard Nossum
    Date: Sat, 5 Jul 2008 14:01:00 +0200
    Subject: [PATCH] printf: add %p{} extension

    Signed-off-by: Vegard Nossum
    ---
    lib/vsprintf.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
    1 files changed, 44 insertions(+), 0 deletions(-)

    diff --git a/lib/vsprintf.c b/lib/vsprintf.c
    index 6021757..011cf3f 100644
    --- a/lib/vsprintf.c
    +++ b/lib/vsprintf.c
    @@ -17,6 +17,7 @@
    */

    #include
    +#include
    #include
    #include
    #include
    @@ -366,6 +367,30 @@ static noinline char* put_dec(char *buf, unsigned long long num)
    #define SMALL 32 /* Must be 32 == 0x20 */
    #define SPECIAL 64 /* 0x */

    +static char *custom_format(char *buf, char *end,
    + const char *fmt, unsigned int fmtlen, void *arg)
    +{
    + if (!strncmp(fmt, "sym", fmtlen)) {
    + char name[KSYM_SYMBOL_LEN];
    + int len;
    + int i;
    +
    + len = sprint_symbol(name, (unsigned long) arg);
    + if (len < 0)
    + return buf;
    +
    + i = 0;
    + while (i < len) {
    + if (buf < end)
    + *buf = name[i];
    + ++buf;
    + ++i;
    + }
    + }
    +
    + return buf;
    +}
    +
    static char *number(char *buf, char *end, unsigned long long num, int base, int size, int precision, int type)
    {
    /* we are called with base 8, 10 or 16, only, thus don't need "G..." */
    @@ -648,6 +673,25 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
    continue;

    case 'p':
    + if (fmt[1] == '{') {
    + const char *cfmt;
    +
    + /* Skip the '%{' */
    + ++fmt;
    + ++fmt;
    +
    + cfmt = fmt;
    +
    + /* Skip everything up to the '}' */
    + while (*fmt && *fmt != '}')
    + ++fmt;
    +
    + str = custom_format(str, end,
    + cfmt, fmt - cfmt,
    + va_arg(args, void *));
    + continue;
    + }
    +
    flags |= SMALL;
    if (field_width == -1) {
    field_width = 2*sizeof(void *);
    --
    1.5.4.1

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  14. Re: the printk problem


    On Saturday 2008-07-05 14:52, Vegard Nossum wrote:
    >> On Saturday 2008-07-05 00:01, Andrew Morton wrote:
    >>>
    >>>We don't know how much interest there would be in churning NIPQUAD from
    >>>the net guys. Interestingly, there's also %C (wint_t) which is a
    >>>32-bit quantity. So we could just go and say "%C prints an ipv4
    >>>address" and be done with it. But there's no way of doing that for
    >>>ipv6 addresses so things would become asymmetrical there.

    >>
    >> struct in6_addr src;
    >> printk("Source address: %p{ipv6}\n", &src);
    >>
    >> How about %p{feature}?

    >
    >Something like this?
    >
    >+static char *custom_format(char *buf, char *end,
    >+ const char *fmt, unsigned int fmtlen, void *arg)


    I'd use const void *arg here, so nobody gets the idea
    that you could modify the argument while printing

    >+{
    >+ if (!strncmp(fmt, "sym", fmtlen)) {
    >+ char name[KSYM_SYMBOL_LEN];
    >+ int len;
    >+ int i;
    >+
    >+ len = sprint_symbol(name, (unsigned long) arg);
    >+ if (len < 0)
    >+ return buf;
    >+
    >+ i = 0;
    >+ while (i < len) {
    >+ if (buf < end)
    >+ *buf = name[i];
    >+ ++buf;
    >+ ++i;
    >+ }
    >+ }


    And I assume it's then as simple as

    } else if (strncmp(fmt, "nip6", fmtlen) == 0) {
    snprintf(buf, end - buf (+1?), NIP6_FMT in expanded form,
    NIP6 in expanded form(arg));
    }

    Hm, that's going to get a big function when everyone adds their
    fmttypes.

    >+
    >+ return buf;
    >+}
    >+
    > static char *number(char *buf, char *end, unsigned long long num, int base, int size, int precision, int type)
    > {
    > /* we are called with base 8, 10 or 16, only, thus don't need "G..." */
    >@@ -648,6 +673,25 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
    > continue;
    >
    > case 'p':
    >+ if (fmt[1] == '{') {
    >+ const char *cfmt;
    >+
    >+ /* Skip the '%{' */
    >+ ++fmt;
    >+ ++fmt;
    >+


    Not this?

    /* Skip the '%p{' */
    fmt += 3;
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  15. Re: the printk problem

    On Sat, Jul 5, 2008 at 3:24 PM, Jan Engelhardt wrote:
    >
    > On Saturday 2008-07-05 14:52, Vegard Nossum wrote:
    >>> On Saturday 2008-07-05 00:01, Andrew Morton wrote:
    >>>>
    >>>>We don't know how much interest there would be in churning NIPQUAD from
    >>>>the net guys. Interestingly, there's also %C (wint_t) which is a
    >>>>32-bit quantity. So we could just go and say "%C prints an ipv4
    >>>>address" and be done with it. But there's no way of doing that for
    >>>>ipv6 addresses so things would become asymmetrical there.
    >>>
    >>> struct in6_addr src;
    >>> printk("Source address: %p{ipv6}\n", &src);
    >>>
    >>> How about %p{feature}?

    >>
    >>Something like this?
    >>
    >>+static char *custom_format(char *buf, char *end,
    >>+ const char *fmt, unsigned int fmtlen, void *arg)

    >
    > I'd use const void *arg here, so nobody gets the idea
    > that you could modify the argument while printing
    >


    Oops, of course. Thanks.

    >>+{
    >>+ if (!strncmp(fmt, "sym", fmtlen)) {

    ....
    >>+ }

    >
    > And I assume it's then as simple as
    >
    > } else if (strncmp(fmt, "nip6", fmtlen) == 0) {
    > snprintf(buf, end - buf (+1?), NIP6_FMT in expanded form,
    > NIP6 in expanded form(arg));
    > }
    >
    > Hm, that's going to get a big function when everyone adds their
    > fmttypes.
    >


    Yes. Luckily, the entry point is then fixed in a single place and it's
    easy to convert it into something more dynamic :-)

    A static array wouldn't help much because it still concentrates all
    the names. But we could at least do a binary search on that and get
    some speed improvements if it grows large.

    I think the most elegant solution would be a macro similar to the
    initcall macros, that adds the custom extensions to a table which is
    defined by a special linker section. This allows complete
    decentralization, but I don't think it's possible to do binary search
    on the names anymore.

    Dynamic registering/unregistering could be done too. Maybe this is a
    good thing for modules...

    Thoughts?

    >>+
    >>+ return buf;
    >>+}
    >>+
    >> static char *number(char *buf, char *end, unsigned long long num, int base, int size, int precision, int type)
    >> {
    >> /* we are called with base 8, 10 or 16, only, thus don't need "G..." */
    >>@@ -648,6 +673,25 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
    >> continue;
    >>
    >> case 'p':
    >>+ if (fmt[1] == '{') {
    >>+ const char *cfmt;
    >>+
    >>+ /* Skip the '%{' */
    >>+ ++fmt;
    >>+ ++fmt;
    >>+

    >
    > Not this?
    >
    > /* Skip the '%p{' */
    > fmt += 3;
    >


    Oops, the comment is wrong. It should say: "Skip the p{". But fmt += 3
    is wrong. Since fmt[0] is at this point 'p', and fmt[1] is '{'. The %
    and flags, etc. have already been skipped by the common code. So it
    should be fmt += 2 :-)

    Thanks!


    Vegard

    --
    "The animistic metaphor of the bug that maliciously sneaked in while
    the programmer was not looking is intellectually dishonest as it
    disguises that the error is the programmer's own creation."
    -- E. W. Dijkstra, EWD1036
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  16. Re: the printk problem


    On Saturday 2008-07-05 15:50, Vegard Nossum wrote:
    >
    >I think the most elegant solution would be a macro similar to the
    >initcall macros, that adds the custom extensions to a table which is
    >defined by a special linker section. This allows complete
    >decentralization, but I don't think it's possible to do binary search
    >on the names anymore.


    With an rbtree, you can still do binary search.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  17. Re: the printk problem



    On Sat, 5 Jul 2008, Vegard Nossum wrote:

    > On Sat, Jul 5, 2008 at 1:33 PM, Jan Engelhardt wrote:
    > >
    > > On Saturday 2008-07-05 00:01, Andrew Morton wrote:
    > >>
    > >>We don't know how much interest there would be in churning NIPQUAD from
    > >>the net guys. Interestingly, there's also %C (wint_t) which is a
    > >>32-bit quantity. So we could just go and say "%C prints an ipv4
    > >>address" and be done with it. But there's no way of doing that for
    > >>ipv6 addresses so things would become asymmetrical there.

    > >
    > > struct in6_addr src;
    > > printk("Source address: %p{ipv6}\n", &src);
    > >
    > > How about %p{feature}?


    No.

    I _expressly_ chose '%p[alphanumeric]*' because it's basically
    totally insane to have that in a *real* printk() string: the end result
    would be totally unreadable.

    In contrast, '%p[specialchar]' is not unreadable, and in fact we have lots
    of those already in the kernel. In fact, there are 40 occurrences of '%p{'
    in the kernel, just grep for it (especially the AFS code seems to be very
    happy to use that kind of printout in its debug statements).

    So it makes perfect sense to have a _real_ printk string that says

    "BUG: Dentry %p{i=%lx,n=%s}"

    where we have that '%p{...' sequence: the end result is easily parseable.
    In contrast, anybody who uses '%pS' or something like that and expects a
    pointer name to be immediately followed by teh letter 'S' is simply
    insane, because the end result is an unreadable mess.

    > (It's hard on the stack, yes, I know. We should fix kallsyms.)


    Not just that, but it's broken when KALLSYMS is disabled. Look at what
    sprint_symbol() becomes.

    The patch I already sent out is about a million times better, because it
    avoids all these issues, and knows about subtle issues like the difference
    between a direct pointer and a pointer to a function descriptor.

    Linus
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  18. Re: the printk problem



    On Sat, 5 Jul 2008, Jan Engelhardt wrote:
    >
    > So, and what do you do when you run out of alphanumeric characters?


    Did you actually look at my patch?

    It's not a single alnum character. It's an arbitrary sequence of alnum
    characters. IOW, my patch allows

    %p6N

    or something like that for showing a ipv6 "NIP" format string etc. Or you
    could spell them out even more, although I consider it unlikely that you
    really want to see too many of these, since gcc won't actually be able to
    type-check them (so they will always remain _secondary_ formats).

    Linus
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  19. Re: the printk problem

    On Sat, Jul 5, 2008 at 7:56 PM, Linus Torvalds
    wrote:
    > On Sat, 5 Jul 2008, Vegard Nossum wrote:
    >> On Sat, Jul 5, 2008 at 1:33 PM, Jan Engelhardt wrote:
    >> > How about %p{feature}?

    >
    > No.
    >
    > I _expressly_ chose '%p[alphanumeric]*' because it's basically
    > totally insane to have that in a *real* printk() string: the end result
    > would be totally unreadable.
    >
    > In contrast, '%p[specialchar]' is not unreadable, and in fact we have lots
    > of those already in the kernel. In fact, there are 40 occurrences of '%p{'
    > in the kernel, just grep for it (especially the AFS code seems to be very
    > happy to use that kind of printout in its debug statements).
    >
    > So it makes perfect sense to have a _real_ printk string that says
    >
    > "BUG: Dentry %p{i=%lx,n=%s}"
    >
    > where we have that '%p{...' sequence: the end result is easily parseable.
    > In contrast, anybody who uses '%pS' or something like that and expects a
    > pointer name to be immediately followed by teh letter 'S' is simply
    > insane, because the end result is an unreadable mess.


    That's really a truly hideous hack :-)

    Single letters are bad because it hurts readability and limits the
    usefulness of the extension.

    If it's just the {} that is the problem, use something else. I'm sure
    we can find something that isn't used already.

    >
    >> (It's hard on the stack, yes, I know. We should fix kallsyms.)

    >
    > Not just that, but it's broken when KALLSYMS is disabled. Look at what
    > sprint_symbol() becomes.


    Oops. Not hard to mend, though.

    > The patch I already sent out is about a million times better, because it
    > avoids all these issues, and knows about subtle issues like the difference
    > between a direct pointer and a pointer to a function descriptor.


    Right, right. I found it now:

    http://ozlabs.org/pipermail/linuxppc...ly/059257.html

    Argh... Some pointer to original thread would be nice when adding new Ccs.


    Vegard

    PS: Your version has exactly the same stack problem. Will send a patch
    for kallsyms later.

    --
    "The animistic metaphor of the bug that maliciously sneaked in while
    the programmer was not looking is intellectually dishonest as it
    disguises that the error is the programmer's own creation."
    -- E. W. Dijkstra, EWD1036
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  20. Re: the printk problem


    On Saturday 2008-07-05 19:56, Linus Torvalds wrote:
    >> >
    >> > How about %p{feature}?

    >
    >No.
    >
    >I _expressly_ chose '%p[alphanumeric]*' because it's basically
    >totally insane to have that in a *real* printk() string: the end result
    >would be totally unreadable.


    So, and what do you do when you run out of alphanumeric characters?
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread
Page 1 of 2 1 2 LastLast