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

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

[Uwe Kleine-König]

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 <Uwe.Kleine-Koenig@digi.com>
---
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

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

[Uwe Kleine-König]

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 <Uwe.Kleine-Koenig@digi.com>
> ---
> 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

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

[Andrew Morton]

On Wed, 2 Jul 2008 11:17:57 +0200 Uwe Kleine-K__nig <Uwe.Kleine-Koenig@digi.com> 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 <Uwe.Kleine-Koenig@digi.com>
> > ---
> > 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;

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

[Uwe Kleine-König]

Hello Andrew,

Andrew Morton wrote:
> On Wed, 2 Jul 2008 11:17:57 +0200 Uwe Kleine-K__nig <Uwe.Kleine-Koenig@digi.com> 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 <Uwe.Kleine-Koenig@digi.com>
> > > ---
> > > 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

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

[Uwe Kleine-König]

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 <Uwe.Kleine-Koenig@digi.com>
---
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

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

[Andrew Morton]

On Fri, 4 Jul 2008 12:46:34 +0200 Uwe Kleine-K__nig <Uwe.Kleine-Koenig@digi.com> 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.

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

[Uwe Kleine-König]

Hello,

Andrew Morton wrote:
> On Fri, 4 Jul 2008 12:46:34 +0200 Uwe Kleine-K__nig <Uwe.Kleine-Koenig@digi.com> 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

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

[Andrew Morton]

On Fri, 4 Jul 2008 20:43:07 +0200 Uwe Kleine-K__nig <Uwe.Kleine-Koenig@digi.com> 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.

Re: the printk problem

[Andrew Morton]

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

On Fri, 4 Jul 2008 14:42:53 -0600 Matthew Wilcox <matthew@wil.cx> 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 <torvalds@linux-foundation.org> 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?

Re: the printk problem

[Matthew Wilcox]

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 <willy@linux.intel.com>

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."

Re: the printk problem

[Denys Vlasenko]

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

Re: the printk problem

[Jan Engelhardt]

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}?

Re: the printk problem

[Vegard Nossum]

On Sat, Jul 5, 2008 at 1:33 PM, Jan Engelhardt <jengelh@medozas.de> 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 <vegard.nossum@gmail.com>
Date: Sat, 5 Jul 2008 14:01:00 +0200
Subject: [PATCH] printf: add %p{} extension

Signed-off-by: Vegard Nossum <vegard.nossum@gmail.com>
---
 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 <stdarg.h>
+#include <linux/kallsyms.h>
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/string.h>
@@ -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

Re: the printk problem

[Jan Engelhardt]

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;

Re: the printk problem

[Vegard Nossum]

On Sat, Jul 5, 2008 at 3:24 PM, Jan Engelhardt <jengelh@medozas.de> 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

Re: the printk problem

[Jan Engelhardt]

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.

Re: the printk problem

[Linus Torvalds]

On Sat, 5 Jul 2008, Vegard Nossum wrote:

> On Sat, Jul 5, 2008 at 1:33 PM, Jan Engelhardt <jengelh@medozas.de> 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

Re: the printk problem

[Jan Engelhardt]

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?

Re: the printk problem

[Vegard Nossum]

On Sat, Jul 5, 2008 at 7:56 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sat, 5 Jul 2008, Vegard Nossum wrote:
>> On Sat, Jul 5, 2008 at 1:33 PM, Jan Engelhardt <jengelh@medozas.de> 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.</MHO>

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-dev/2008-July/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

Re: the printk problem

[Linus Torvalds]

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

Re: the printk problem

[Matthew Wilcox]

On Sat, Jul 05, 2008 at 08:41:39PM +0200, Vegard Nossum wrote:
> Single letters are bad because it hurts readability and limits the
> usefulness of the extension.</MHO>

I think you need a little warning noise that goes off in your head that
means "I might be overdesigning this".  Linus' code is elegant and
solves a problem nicely.

-- 
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."

Re: the printk problem

[Pekka Enberg]

On Sat, Jul 05, 2008 at 08:41:39PM +0200, Vegard Nossum wrote:
>> Single letters are bad because it hurts readability and limits the
>> usefulness of the extension.</MHO>

On Sat, Jul 5, 2008 at 9:52 PM, Matthew Wilcox <matthew@wil.cx> wrote:
> I think you need a little warning noise that goes off in your head that
> means "I might be overdesigning this".  Linus' code is elegant and
> solves a problem nicely.

Am I the only one who missed Linus' patch? Did it make it to the list?

Re: the printk problem

[Randy Dunlap]

On Sun, 6 Jul 2008 03:02:59 +0300 Pekka Enberg wrote:

> On Sat, Jul 05, 2008 at 08:41:39PM +0200, Vegard Nossum wrote:
> >> Single letters are bad because it hurts readability and limits the
> >> usefulness of the extension.</MHO>
> 
> On Sat, Jul 5, 2008 at 9:52 PM, Matthew Wilcox <matthew@wil.cx> wrote:
> > I think you need a little warning noise that goes off in your head that
> > means "I might be overdesigning this".  Linus' code is elegant and
> > solves a problem nicely.
> 
> Am I the only one who missed Linus' patch? Did it make it to the list?

No, you are not the only one.  It was on linuxppc-dev for some reason.
http://ozlabs.org/pipermail/linuxppc-dev/2008-July/059257.html

---
~Randy
Linux Plumbers Conference, 17-19 September 2008, Portland, Oregon USA
http://linuxplumbersconf.org/

Re: the printk problem

[Linus Torvalds]

On Sun, 6 Jul 2008, Ingo Molnar wrote:
> 
> applied (with the commit message below) to tip/x86/debug for v2.6.27 
> merging, thanks Linus. Can i add your SOB too?

Sure, add my S-O-B. But I hope/assuem that you also added my earlier patch 
that added the support for '%pS' too? I'm not entirely sure that should go 
in an x86-specific branch, since it has nothing x86-specific in it.

Also, I've cleaned up the lib/vsnprintf.c patch to fix up some minor 
details. For example, it should not default to a field width of 
"2*sizeof(unsigned long)" - it should do that only if it ends up printing 
the pointer as a hex number.

So this is my current version.. (the "precision" thing was moved into the 
'pointer()' function, and to after the special case handling).

		Linus
---
 lib/vsprintf.c |  118 ++++++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 85 insertions(+), 33 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 6021757..f60c7c0 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -22,6 +22,8 @@
 #include <linux/string.h>
 #include <linux/ctype.h>
 #include <linux/kernel.h>
+#include <linux/kallsyms.h>
+#include <linux/uaccess.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/div64.h>
@@ -482,6 +484,82 @@ static char *number(char *buf, char *end, unsigned long long num, int base, int
 	return buf;
 }
 
+static char *string(char *buf, char *end, char *s, int field_width, int precision, int flags)
+{
+	int len, i;
+
+	if ((unsigned long)s < PAGE_SIZE)
+		s = "<NULL>";
+
+	len = strnlen(s, precision);
+
+	if (!(flags & LEFT)) {
+		while (len < field_width--) {
+			if (buf < end)
+				*buf = ' ';
+			++buf;
+		}
+	}
+	for (i = 0; i < len; ++i) {
+		if (buf < end)
+			*buf = *s;
+		++buf; ++s;
+	}
+	while (len < field_width--) {
+		if (buf < end)
+			*buf = ' ';
+		++buf;
+	}
+	return buf;
+}
+
+static inline void *dereference_function_descriptor(void *ptr)
+{
+#if defined(CONFIG_IA64) || defined(CONFIG_PPC64)
+	void *p;
+	if (!probe_kernel_address(ptr, p))
+		ptr = p;
+#endif
+	return ptr;
+}
+
+
+/*
+ * Show a '%p' thing.  A kernel extension is that the '%p' is followed
+ * by an extra set of alphanumeric characters that are extended format
+ * specifiers.  Right now we just handle 'F' (for symbolic Function
+ * pointers) and 'S' (for Symbolic data pointers), but this can easily
+ * be extended in the future (network address types etc).
+ *
+ * The difference between 'S' and 'F' is that on ia64 and ppc64 function
+ * pointers are really function descriptors, which contain a pointer the
+ * real address. 
+ */
+static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int base, int size, int precision, int type)
+{
+	switch (*fmt) {
+	case 'F':
+		ptr = dereference_function_descriptor(ptr);
+		/* Fallthrough */
+	case 'S': {	/* Other (direct) pointer */
+#if CONFIG_KALLSYMS
+		char sym[KSYM_SYMBOL_LEN];
+		sprint_symbol(sym, (unsigned long) ptr);
+		return string(buf, end, sym, size, precision, type);
+#else
+		type |= SPECIAL;
+		break;
+#endif
+	}
+	}
+	type |= SMALL;
+	if (precision == -1) {
+		precision = 2*sizeof(void *);
+		type |= ZEROPAD;
+	}
+	return number(buf, end, (unsigned long long) ptr, base, size, precision, type);
+}
+
 /**
  * vsnprintf - Format a string and place it in a buffer
  * @buf: The buffer to place the result into
@@ -502,11 +580,9 @@ static char *number(char *buf, char *end, unsigned long long num, int base, int
  */
 int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 {
-	int len;
 	unsigned long long num;
-	int i, base;
+	int base;
 	char *str, *end, c;
-	const char *s;
 
 	int flags;		/* flags to number() */
 
@@ -622,40 +698,16 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 				continue;
 
 			case 's':
-				s = va_arg(args, char *);
-				if ((unsigned long)s < PAGE_SIZE)
-					s = "<NULL>";
-
-				len = strnlen(s, precision);
-
-				if (!(flags & LEFT)) {
-					while (len < field_width--) {
-						if (str < end)
-							*str = ' ';
-						++str;
-					}
-				}
-				for (i = 0; i < len; ++i) {
-					if (str < end)
-						*str = *s;
-					++str; ++s;
-				}
-				while (len < field_width--) {
-					if (str < end)
-						*str = ' ';
-					++str;
-				}
+				str = string(str, end, va_arg(args, char *), field_width, precision, flags);
 				continue;
 
 			case 'p':
-				flags |= SMALL;
-				if (field_width == -1) {
-					field_width = 2*sizeof(void *);
-					flags |= ZEROPAD;
-				}
-				str = number(str, end,
-						(unsigned long) va_arg(args, void *),
+				str = pointer(fmt+1, str, end,
+						va_arg(args, void *),
 						16, field_width, precision, flags);
+				/* Skip all alphanumeric pointer suffixes */
+				while (isalnum(fmt[1]))
+					fmt++;
 				continue;
 
 

Re: the printk problem

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sun, 6 Jul 2008, Ingo Molnar wrote:
> > 
> > applied (with the commit message below) to tip/x86/debug for v2.6.27 
> > merging, thanks Linus. Can i add your SOB too?
> 
> Sure, add my S-O-B. But I hope/assuem that you also added my earlier 
> patch that added the support for '%pS' too? I'm not entirely sure that 
> should go in an x86-specific branch, since it has nothing x86-specific 
> in it.

yeah, agreed, combined it's not an x86 topic anymore.

[ There's some lkml trouble so i've missed the earlier patch. I'm not 
  sure the email problem is on my side, see how incomplete the 
  discussion is on lkml.org as well:

     http://lkml.org/lkml/2008/6/25/170   ]

Anyway, i have have added this second patch of yours to tip/core/printk 
and moved your first patch over to that topic (which relies on it).

That topic has a few other (smaller) printk enhancements queued for 
v2.6.27 already:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git core/printk

[find other stats below]

so it fits in naturally.

	Ingo

------------------>
Ingo Molnar (1):
      printk: export console_drivers

Jan Kiszka (1):
      printk: don't prefer unsuited consoles on registration

Jiri Slaby (1):
      x86, generic: mark early_printk as asmlinkage

Linus Torvalds (2):
      printk: add support for '%pS'
      x86, 64-bit: standardize printk_address()

Nick Andrew (2):
      printk: refactor processing of line severity tokens
      printk: remember the message level for multi-line output

Tejun Heo (1):
      printk: clean up recursion check related static variables

Thomas Gleixner (2):
      namespacecheck: fix kernel printk.c
      namespacecheck: more kernel/printk.c fixes

 arch/x86/kernel/early_printk.c |    2 +-
 arch/x86/kernel/traps_64.c     |   25 +--------
 include/linux/kernel.h         |    8 +---
 kernel/printk.c                |  107 +++++++++++++++---------------------
 lib/vsprintf.c                 |  118 +++++++++++++++++++++++++++++-----------
 5 files changed, 133 insertions(+), 127 deletions(-)

diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
index 643fd86..ff9e735 100644
--- a/arch/x86/kernel/early_printk.c
+++ b/arch/x86/kernel/early_printk.c
@@ -196,7 +196,7 @@ static struct console simnow_console = {
 static struct console *early_console = &early_vga_console;
 static int early_console_initialized;
 
-void early_printk(const char *fmt, ...)
+asmlinkage void early_printk(const char *fmt, ...)
 {
 	char buf[512];
 	int n;
diff --git a/arch/x86/kernel/traps_64.c b/arch/x86/kernel/traps_64.c
index adff76e..f1a95d1 100644
--- a/arch/x86/kernel/traps_64.c
+++ b/arch/x86/kernel/traps_64.c
@@ -104,30 +104,7 @@ int kstack_depth_to_print = 12;
 
 void printk_address(unsigned long address, int reliable)
 {
-#ifdef CONFIG_KALLSYMS
-	unsigned long offset = 0, symsize;
-	const char *symname;
-	char *modname;
-	char *delim = ":";
-	char namebuf[KSYM_NAME_LEN];
-	char reliab[4] = "";
-
-	symname = kallsyms_lookup(address, &symsize, &offset,
-					&modname, namebuf);
-	if (!symname) {
-		printk(" [<%016lx>]\n", address);
-		return;
-	}
-	if (!reliable)
-		strcpy(reliab, "? ");
-
-	if (!modname)
-		modname = delim = "";
-	printk(" [<%016lx>] %s%s%s%s%s+0x%lx/0x%lx\n",
-		address, reliab, delim, modname, delim, symname, offset, symsize);
-#else
-	printk(" [<%016lx>]\n", address);
-#endif
+	printk(" [<%016lx>] %s%pS\n", address, reliable ? "": "? ", (void *) address);
 }
 
 static unsigned long *in_exception_stack(unsigned cpu, unsigned long stack,
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 792bf0a..4cb8d3d 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -184,9 +184,6 @@ asmlinkage int vprintk(const char *fmt, va_list args)
 	__attribute__ ((format (printf, 1, 0)));
 asmlinkage int printk(const char * fmt, ...)
 	__attribute__ ((format (printf, 1, 2))) __cold;
-extern int log_buf_get_len(void);
-extern int log_buf_read(int idx);
-extern int log_buf_copy(char *dest, int idx, int len);
 
 extern int printk_ratelimit_jiffies;
 extern int printk_ratelimit_burst;
@@ -202,9 +199,6 @@ static inline int vprintk(const char *s, va_list args) { return 0; }
 static inline int printk(const char *s, ...)
 	__attribute__ ((format (printf, 1, 2)));
 static inline int __cold printk(const char *s, ...) { return 0; }
-static inline int log_buf_get_len(void) { return 0; }
-static inline int log_buf_read(int idx) { return 0; }
-static inline int log_buf_copy(char *dest, int idx, int len) { return 0; }
 static inline int printk_ratelimit(void) { return 0; }
 static inline int __printk_ratelimit(int ratelimit_jiffies, \
 				     int ratelimit_burst) { return 0; }
@@ -213,7 +207,7 @@ static inline bool printk_timed_ratelimit(unsigned long *caller_jiffies, \
 		{ return false; }
 #endif
 
-extern void __attribute__((format(printf, 1, 2)))
+extern void asmlinkage __attribute__((format(printf, 1, 2)))
 	early_printk(const char *fmt, ...);
 
 unsigned long int_sqrt(unsigned long);
diff --git a/kernel/printk.c b/kernel/printk.c
index 8fb01c3..de1a4f4 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -38,7 +38,7 @@
 /*
  * Architectures can override it:
  */
-void __attribute__((weak)) early_printk(const char *fmt, ...)
+void asmlinkage __attribute__((weak)) early_printk(const char *fmt, ...)
 {
 }
 
@@ -75,6 +75,8 @@ EXPORT_SYMBOL(oops_in_progress);
 static DECLARE_MUTEX(console_sem);
 static DECLARE_MUTEX(secondary_console_sem);
 struct console *console_drivers;
+EXPORT_SYMBOL_GPL(console_drivers);
+
 /*
  * This is used for debugging the mess that is the VT code by
  * keeping track if we have the console semaphore held. It's
@@ -231,7 +233,7 @@ static inline void boot_delay_msec(void)
 /*
  * Return the number of unread characters in the log buffer.
  */
-int log_buf_get_len(void)
+static int log_buf_get_len(void)
 {
 	return logged_chars;
 }
@@ -268,19 +270,6 @@ int log_buf_copy(char *dest, int idx, int len)
 }
 
 /*
- * Extract a single character from the log buffer.
- */
-int log_buf_read(int idx)
-{
-	char ret;
-
-	if (log_buf_copy(&ret, idx, 1) == 1)
-		return ret;
-	else
-		return -1;
-}
-
-/*
  * Commands to do_syslog:
  *
  * 	0 -- Close the log.  Currently a NOP.
@@ -665,18 +654,17 @@ static int acquire_console_semaphore_for_printk(unsigned int cpu)
 	spin_unlock(&logbuf_lock);
 	return retval;
 }
-
-const char printk_recursion_bug_msg [] =
-			KERN_CRIT "BUG: recent printk recursion!\n";
-static int printk_recursion_bug;
+static const char recursion_bug_msg [] =
+		KERN_CRIT "BUG: recent printk recursion!\n";
+static int recursion_bug;
+	static int new_text_line = 1;
+static char printk_buf[1024];
 
 asmlinkage int vprintk(const char *fmt, va_list args)
 {
-	static int log_level_unknown = 1;
-	static char printk_buf[1024];
-
-	unsigned long flags;
 	int printed_len = 0;
+	int current_log_level = default_message_loglevel;
+	unsigned long flags;
 	int this_cpu;
 	char *p;
 
@@ -699,7 +687,7 @@ asmlinkage int vprintk(const char *fmt, va_list args)
 		 * it can be printed at the next appropriate moment:
 		 */
 		if (!oops_in_progress) {
-			printk_recursion_bug = 1;
+			recursion_bug = 1;
 			goto out_restore_irqs;
 		}
 		zap_locks();
@@ -709,70 +697,62 @@ asmlinkage int vprintk(const char *fmt, va_list args)
 	spin_lock(&logbuf_lock);
 	printk_cpu = this_cpu;
 
-	if (printk_recursion_bug) {
-		printk_recursion_bug = 0;
-		strcpy(printk_buf, printk_recursion_bug_msg);
-		printed_len = sizeof(printk_recursion_bug_msg);
+	if (recursion_bug) {
+		recursion_bug = 0;
+		strcpy(printk_buf, recursion_bug_msg);
+		printed_len = sizeof(recursion_bug_msg);
 	}
 	/* Emit the output into the temporary buffer */
 	printed_len += vscnprintf(printk_buf + printed_len,
 				  sizeof(printk_buf) - printed_len, fmt, args);
 
+
 	/*
 	 * Copy the output into log_buf.  If the caller didn't provide
 	 * appropriate log level tags, we insert them here
 	 */
 	for (p = printk_buf; *p; p++) {
-		if (log_level_unknown) {
-                        /* log_level_unknown signals the start of a new line */
+		if (new_text_line) {
+			/* If a token, set current_log_level and skip over */
+			if (p[0] == '<' && p[1] >= '0' && p[1] <= '7' &&
+			    p[2] == '>') {
+				current_log_level = p[1] - '0';
+				p += 3;
+				printed_len -= 3;
+			}
+
+			/* Always output the token */
+			emit_log_char('<');
+			emit_log_char(current_log_level + '0');
+			emit_log_char('>');
+			printed_len += 3;
+			new_text_line = 0;
+
 			if (printk_time) {
-				int loglev_char;
+				/* Follow the token with the time */
 				char tbuf[50], *tp;
 				unsigned tlen;
 				unsigned long long t;
 				unsigned long nanosec_rem;
 
-				/*
-				 * force the log level token to be
-				 * before the time output.
-				 */
-				if (p[0] == '<' && p[1] >='0' &&
-				   p[1] <= '7' && p[2] == '>') {
-					loglev_char = p[1];
-					p += 3;
-					printed_len -= 3;
-				} else {
-					loglev_char = default_message_loglevel
-						+ '0';
-				}
 				t = cpu_clock(printk_cpu);
 				nanosec_rem = do_div(t, 1000000000);
-				tlen = sprintf(tbuf,
-						"<%c>[%5lu.%06lu] ",
-						loglev_char,
-						(unsigned long)t,
-						nanosec_rem/1000);
+				tlen = sprintf(tbuf, "[%5lu.%06lu] ",
+						(unsigned long) t,
+						nanosec_rem / 1000);
 
 				for (tp = tbuf; tp < tbuf + tlen; tp++)
 					emit_log_char(*tp);
 				printed_len += tlen;
-			} else {
-				if (p[0] != '<' || p[1] < '0' ||
-				   p[1] > '7' || p[2] != '>') {
-					emit_log_char('<');
-					emit_log_char(default_message_loglevel
-						+ '0');
-					emit_log_char('>');
-					printed_len += 3;
-				}
 			}
-			log_level_unknown = 0;
+
 			if (!*p)
 				break;
 		}
+
 		emit_log_char(*p);
 		if (*p == '\n')
-			log_level_unknown = 1;
+			new_text_line = 1;
 	}
 
 	/*
@@ -1172,8 +1152,11 @@ void register_console(struct console *console)
 			console->index = 0;
 		if (console->setup == NULL ||
 		    console->setup(console, NULL) == 0) {
-			console->flags |= CON_ENABLED | CON_CONSDEV;
-			preferred_console = 0;
+			console->flags |= CON_ENABLED;
+			if (console->device) {
+				console->flags |= CON_CONSDEV;
+				preferred_console = 0;
+			}
 		}
 	}
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 6021757..f60c7c0 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -22,6 +22,8 @@
 #include <linux/string.h>
 #include <linux/ctype.h>
 #include <linux/kernel.h>
+#include <linux/kallsyms.h>
+#include <linux/uaccess.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/div64.h>
@@ -482,6 +484,82 @@ static char *number(char *buf, char *end, unsigned long long num, int base, int
 	return buf;
 }
 
+static char *string(char *buf, char *end, char *s, int field_width, int precision, int flags)
+{
+	int len, i;
+
+	if ((unsigned long)s < PAGE_SIZE)
+		s = "<NULL>";
+
+	len = strnlen(s, precision);
+
+	if (!(flags & LEFT)) {
+		while (len < field_width--) {
+			if (buf < end)
+				*buf = ' ';
+			++buf;
+		}
+	}
+	for (i = 0; i < len; ++i) {
+		if (buf < end)
+			*buf = *s;
+		++buf; ++s;
+	}
+	while (len < field_width--) {
+		if (buf < end)
+			*buf = ' ';
+		++buf;
+	}
+	return buf;
+}
+
+static inline void *dereference_function_descriptor(void *ptr)
+{
+#if defined(CONFIG_IA64) || defined(CONFIG_PPC64)
+	void *p;
+	if (!probe_kernel_address(ptr, p))
+		ptr = p;
+#endif
+	return ptr;
+}
+
+
+/*
+ * Show a '%p' thing.  A kernel extension is that the '%p' is followed
+ * by an extra set of alphanumeric characters that are extended format
+ * specifiers.  Right now we just handle 'F' (for symbolic Function
+ * pointers) and 'S' (for Symbolic data pointers), but this can easily
+ * be extended in the future (network address types etc).
+ *
+ * The difference between 'S' and 'F' is that on ia64 and ppc64 function
+ * pointers are really function descriptors, which contain a pointer the
+ * real address. 
+ */
+static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int base, int size, int precision, int type)
+{
+	switch (*fmt) {
+	case 'F':
+		ptr = dereference_function_descriptor(ptr);
+		/* Fallthrough */
+	case 'S': {	/* Other (direct) pointer */
+#if CONFIG_KALLSYMS
+		char sym[KSYM_SYMBOL_LEN];
+		sprint_symbol(sym, (unsigned long) ptr);
+		return string(buf, end, sym, size, precision, type);
+#else
+		type |= SPECIAL;
+		break;
+#endif
+	}
+	}
+	type |= SMALL;
+	if (precision == -1) {
+		precision = 2*sizeof(void *);
+		type |= ZEROPAD;
+	}
+	return number(buf, end, (unsigned long long) ptr, base, size, precision, type);
+}
+
 /**
  * vsnprintf - Format a string and place it in a buffer
  * @buf: The buffer to place the result into
@@ -502,11 +580,9 @@ static char *number(char *buf, char *end, unsigned long long num, int base, int
  */
 int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 {
-	int len;
 	unsigned long long num;
-	int i, base;
+	int base;
 	char *str, *end, c;
-	const char *s;
 
 	int flags;		/* flags to number() */
 
@@ -622,40 +698,16 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 				continue;
 
 			case 's':
-				s = va_arg(args, char *);
-				if ((unsigned long)s < PAGE_SIZE)
-					s = "<NULL>";
-
-				len = strnlen(s, precision);
-
-				if (!(flags & LEFT)) {
-					while (len < field_width--) {
-						if (str < end)
-							*str = ' ';
-						++str;
-					}
-				}
-				for (i = 0; i < len; ++i) {
-					if (str < end)
-						*str = *s;
-					++str; ++s;
-				}
-				while (len < field_width--) {
-					if (str < end)
-						*str = ' ';
-					++str;
-				}
+				str = string(str, end, va_arg(args, char *), field_width, precision, flags);
 				continue;
 
 			case 'p':
-				flags |= SMALL;
-				if (field_width == -1) {
-					field_width = 2*sizeof(void *);
-					flags |= ZEROPAD;
-				}
-				str = number(str, end,
-						(unsigned long) va_arg(args, void *),
+				str = pointer(fmt+1, str, end,
+						va_arg(args, void *),
 						16, field_width, precision, flags);
+				/* Skip all alphanumeric pointer suffixes */
+				while (isalnum(fmt[1]))
+					fmt++;
 				continue;
 
 

Re: the printk problem

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> yeah, agreed, combined it's not an x86 topic anymore.
> 
> [ There's some lkml trouble so i've missed the earlier patch. I'm not 
>   sure the email problem is on my side, see how incomplete the 
>   discussion is on lkml.org as well:
> 
>      http://lkml.org/lkml/2008/6/25/170   ]
> 
> Anyway, i have have added this second patch of yours to 
> tip/core/printk and moved your first patch over to that topic (which 
> relies on it).

ah, i found the reason - it started out on linux-ia64 originally then 
moved over to lkml - so only half of the discussion was visible there. 
And linux-ia64 is one of the few vger lists i'm not subscribed to 
apparently. (there's no vger-please-give-me-all-emails list - making the 
following of Linux development even harder)

	Ingo

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

[Uwe Kleine-König]

Andrew Morton wrote:
> On Fri, 4 Jul 2008 20:43:07 +0200 Uwe Kleine-K__nig <Uwe.Kleine-Koenig@digi.com> 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.
I assume the result is 0fe1ef24f7bd0020f29ffe287dfdb9ead33ca0b2?

Should I send a new version using %pF or is it easier if you fix up 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

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

[Andrew Morton]

On Wed, 9 Jul 2008 15:13:53 +0200 Uwe Kleine-K__nig <Uwe.Kleine-Koenig@digi.com> wrote:

> Andrew Morton wrote:
> > On Fri, 4 Jul 2008 20:43:07 +0200 Uwe Kleine-K__nig <Uwe.Kleine-Koenig@digi.com> 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.
> I assume the result is 0fe1ef24f7bd0020f29ffe287dfdb9ead33ca0b2?

yup.

> Should I send a new version using %pF or is it easier if you fix up the
> patch?

A new version would be nice, thanks.  Or an incremental diff, but I can and
will turn new versions into an incremental in the twinkle of an eye, so
either is OK.

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

[Uwe Kleine-König]

Hello,

Andrew Morton wrote:
> > > 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.
> > I assume the result is 0fe1ef24f7bd0020f29ffe287dfdb9ead33ca0b2?
> 
> yup.
When I saw this commit I wondered that it was you who told me to think
about this but the commiter was Linus.
 
> > Should I send a new version using %pF or is it easier if you fix up the
> > patch?
> 
> A new version would be nice, thanks.  Or an incremental diff, but I can and
> will turn new versions into an incremental in the twinkle of an eye, so
> either is OK.
voilà:

--->8---
>From c991997725fdce93352aab53ccab34f41b5afd52 Mon Sep 17 00:00:00 2001
From: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
Date: Thu, 10 Jul 2008 10:03:59 +0200
Subject: [PATCH] __irq_set_trigger: use %pF to print the set_type callback's name
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

This uses the new feature of printk introduced in
0fe1ef24f7bd0020f29ffe287dfdb9ead33ca0b2.

The actual output is unchanged.

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
 kernel/irq/manage.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 178b966..e01ad8e 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -287,14 +287,9 @@ static int __irq_set_trigger(struct irq_chip *chip, unsigned int irq,
 
 	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);
-	}
+	if (ret)
+		pr_err("setting flow type for irq %u failed (%pF)\n",
+				irq, chip->set_type);
 
 	return ret;
 }
-- 
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

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

[Andrew Morton]

On Thu, 10 Jul 2008 10:23:36 +0200 Uwe Kleine-K__nig <Uwe.Kleine-Koenig@digi.com> wrote:

> Hello,
> 
> Andrew Morton wrote:
> > > > 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.
> > > I assume the result is 0fe1ef24f7bd0020f29ffe287dfdb9ead33ca0b2?
> > 
> > yup.
> When I saw this commit I wondered that it was you who told me to think
> about this but the commiter was Linus.

A long story.  I rubbed a magic lantern ;)

> > > Should I send a new version using %pF or is it easier if you fix up the
> > > patch?
> > 
> > A new version would be nice, thanks.  Or an incremental diff, but I can and
> > will turn new versions into an incremental in the twinkle of an eye, so
> > either is OK.
> voila:

Thanks.

Re: [PATCH] Make u64 long long on all architectures (was: the printk problem)

[Andrew Morton]

On Fri, 4 Jul 2008 20:03:51 -0600 Matthew Wilcox <matthew@wil.cx> wrote:

> [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 <willy@linux.intel.com>
> 
> 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__ */

This is (IMO) a desirable change and will prevent a heck of a lot of
goofing around, and will permit a lot of prior goofing around to
be removed.

But I bet there are lots of instalces of printk("%l", some_u64) down in
arch code where the type of u64 _is_ known which will now spew warnings.

Oh well.

Re: [PATCH] Make u64 long long on all architectures (was: the printk problem)

[Michael Ellerman]

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

On Tue, 2008-07-22 at 03:05 -0700, Andrew Morton wrote:
> On Fri, 4 Jul 2008 20:03:51 -0600 Matthew Wilcox <matthew@wil.cx> wrote:
> 
> > [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 <willy@linux.intel.com>
> > 
> > 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__ */
> 
> This is (IMO) a desirable change and will prevent a heck of a lot of
> goofing around, and will permit a lot of prior goofing around to
> be removed.
> 
> But I bet there are lots of instalces of printk("%l", some_u64) down in
> arch code where the type of u64 _is_ known which will now spew warnings.
> 
> Oh well.

As a rough estimate:

concordia powerpc(master) $ find arch/powerpc/ ! -name '*32.*' | xargs grep "%l" | grep -v "%ll" | wc -l
635


Someone's gonna get a lot of git points for fixing all those. Might keep
the speeling fix crowd busy for a while.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

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

Re: [PATCH] Make u64 long long on all architectures (was: the printk problem)

[Andrew Morton]

On Tue, 22 Jul 2008 20:36:35 +1000 Michael Ellerman <michael@ellerman.id.au> wrote:

> On Tue, 2008-07-22 at 03:05 -0700, Andrew Morton wrote:
> > On Fri, 4 Jul 2008 20:03:51 -0600 Matthew Wilcox <matthew@wil.cx> wrote:
> > 
> > > [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 <willy@linux.intel.com>
> > > 
> > > 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__ */
> > 
> > This is (IMO) a desirable change and will prevent a heck of a lot of
> > goofing around, and will permit a lot of prior goofing around to
> > be removed.
> > 
> > But I bet there are lots of instalces of printk("%l", some_u64) down in
> > arch code where the type of u64 _is_ known which will now spew warnings.
> > 
> > Oh well.
> 
> As a rough estimate:
> 
> concordia powerpc(master) $ find arch/powerpc/ ! -name '*32.*' | xargs grep "%l" | grep -v "%ll" | wc -l
> 635

lolz.  If yesterdays-linux-next on todays-mainline wasn't such a
hilarious trainwreck I'd test your grepping.  I guess that could be
done on mainline too.

> Someone's gonna get a lot of git points for fixing all those. Might keep
> the speeling fix crowd busy for a while.

I'm not sure I have the energy for this.

But we really should do it.

Re: [PATCH] Make u64 long long on all architectures (was: the printk problem)

[Benjamin Herrenschmidt]

> 
> This is (IMO) a desirable change and will prevent a heck of a lot of
> goofing around, and will permit a lot of prior goofing around to
> be removed.
> 
> But I bet there are lots of instalces of printk("%l", some_u64) down in
> arch code where the type of u64 _is_ known which will now spew warnings.

Well, I'm about to call a big "warning fixing day" on the powerpc list,
I saw a few today when building a couple of configs and that hurts my
eyes so we may as well fold that in :-)

Cheers,
Ben.
 

Re: [PATCH] Make u64 long long on all architectures (was: the printk problem)

[Benjamin Herrenschmidt]

On Tue, 2008-07-22 at 20:36 +1000, Michael Ellerman wrote:
> concordia powerpc(master) $ find arch/powerpc/ ! -name '*32.*' | xargs
> grep "%l" | grep -v "%ll" | wc -l
> 635
> 
> 
> Someone's gonna get a lot of git points for fixing all those. Might
> keep
> the speeling fix crowd busy for a 

But a bunch of those might actually be real longs, not u64's ...

Easier to do the change and build something like ppc6xx_defconfig to get
a better approximation.

Ben.