[PATCH 0/3] lib/vsprintf: Doc improvements and clock support

[PATCH 0/3] lib/vsprintf: Doc improvements and clock support

[Geert Uytterhoeven]

From: Geert Uytterhoeven <geert+renesas@glider.be>

	Hi,

This patch series improves the documentation for printk() formats, and
adds support for printing clocks. The latter has always been a hassle if
you wanted to support both the common and legacy clock frameworks.

  - '%pC' and '%pCn' print the name (Common Clock Framework) or address
    (legacy clock framework) of a clock,
  - '%pCr' prints the current clock rate.

This is against next-20150226.

Thanks!

Geert Uytterhoeven (3):
  lib/vsprintf: Document %p parameters passed by reference
  lib/vsprintf: Move integer format types to the top
  lib/vsprintf: Add %pC{,n,r} format specifiers for clocks

 Documentation/printk-formats.txt | 49 ++++++++++++++++++++++++++++++----------
 lib/vsprintf.c                   | 37 ++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 12 deletions(-)

-- 
1.9.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 1/3] lib/vsprintf: Document %p parameters passed by reference

[Geert Uytterhoeven]

From: Geert Uytterhoeven <geert+renesas@glider.be>

Make sure all %p extensions that take parameters by references are
documented to do so.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
This has been sent before as a separate patch, and was updated for the
addition of %pT.

 Documentation/printk-formats.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 8858db8f8805bce7..06c8332f0cc09238 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -54,6 +54,7 @@ Struct Resources:
 
 	For printing struct resources. The 'R' and 'r' specifiers result in a
 	printed resource with ('R') or without ('r') a decoded flags member.
+	Passed by reference.
 
 Physical addresses types phys_addr_t:
 
@@ -132,6 +133,8 @@ MAC/FDDI addresses:
 	specifier to use reversed byte order suitable for visual interpretation
 	of Bluetooth addresses which are in the little endian order.
 
+	Passed by reference.
+
 IPv4 addresses:
 
 	%pI4	1.2.3.4
@@ -146,6 +149,8 @@ IPv4 addresses:
 	host, network, big or little endian order addresses respectively. Where
 	no specifier is provided the default network/big endian order is used.
 
+	Passed by reference.
+
 IPv6 addresses:
 
 	%pI6	0001:0002:0003:0004:0005:0006:0007:0008
@@ -160,6 +165,8 @@ IPv6 addresses:
 	print a compressed IPv6 address as described by
 	http://tools.ietf.org/html/rfc5952
 
+	Passed by reference.
+
 IPv4/IPv6 addresses (generic, with port, flowinfo, scope):
 
 	%pIS	1.2.3.4		or 0001:0002:0003:0004:0005:0006:0007:0008
@@ -186,6 +193,8 @@ IPv4/IPv6 addresses (generic, with port, flowinfo, scope):
 	specifiers can be used as well and are ignored in case of an IPv6
 	address.
 
+	Passed by reference.
+
 	Further examples:
 
 	%pISfc		1.2.3.4		or [1:2:3:4:5:6:7:8]/123456789
@@ -207,6 +216,8 @@ UUID/GUID addresses:
 	Where no additional specifiers are used the default little endian
 	order with lower case hex characters will be printed.
 
+	Passed by reference.
+
 dentry names:
 	%pd{,2,3,4}
 	%pD{,2,3,4}
@@ -216,12 +227,16 @@ dentry names:
 	equivalent of %s dentry->d_name.name we used to use, %pd<n> prints
 	n last components.  %pD does the same thing for struct file.
 
+	Passed by reference.
+
 task_struct comm name:
 
         %pT
 
         For printing task_struct->comm.
 
+	Passed by reference (NULL for "current").
+
 struct va_format:
 
 	%pV
@@ -237,6 +252,8 @@ struct va_format:
 	Do not use this feature without some mechanism to verify the
 	correctness of the format string and va_list arguments.
 
+	Passed by reference.
+
 u64 SHOULD be printed with %llu/%llx:
 
 	printk("%llu", u64_var);
-- 
1.9.1

[PATCH 2/3] lib/vsprintf: Move integer format types to the top

[Geert Uytterhoeven]

From: Geert Uytterhoeven <geert+renesas@glider.be>

Move the format types for 64-bit integers and configurable size integers
to the top, so they're next to the other integer format types.
While at it, add the missing format types for s32 and u32.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Documentation/printk-formats.txt | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 06c8332f0cc09238..11413036a63d1f7a 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -8,6 +8,21 @@ If variable is of Type,		use printk format specifier:
 		unsigned long long	%llu or %llx
 		size_t			%zu or %zx
 		ssize_t			%zd or %zx
+		s32			%d or %x
+		u32			%u or %x
+		s64			%lld or %llx
+		u64			%llu or %llx
+
+If <type> is dependent on a config option for its size (e.g., sector_t,
+blkcnt_t) or is architecture-dependent for its size (e.g., tcflag_t), use a
+format specifier of its largest possible type and explicitly cast to it.
+Example:
+
+	printk("test: sector number/total blocks: %llu/%llu\n",
+		(unsigned long long)sector, (unsigned long long)blockcount);
+
+Reminder: sizeof() result is of type size_t.
+
 
 Raw pointer value SHOULD be printed with %p. The kernel supports
 the following extended format specifiers for pointer types:
@@ -254,23 +269,6 @@ struct va_format:
 
 	Passed by reference.
 
-u64 SHOULD be printed with %llu/%llx:
-
-	printk("%llu", u64_var);
-
-s64 SHOULD be printed with %lld/%llx:
-
-	printk("%lld", s64_var);
-
-If <type> is dependent on a config option for its size (e.g., sector_t,
-blkcnt_t) or is architecture-dependent for its size (e.g., tcflag_t), use a
-format specifier of its largest possible type and explicitly cast to it.
-Example:
-
-	printk("test: sector number/total blocks: %llu/%llu\n",
-		(unsigned long long)sector, (unsigned long long)blockcount);
-
-Reminder: sizeof() result is of type size_t.
 
 Thank you for your cooperation and attention.
 
-- 
1.9.1

[PATCH 3/3] lib/vsprintf: Add %pC{,n,r} format specifiers for clocks

[Geert Uytterhoeven]

From: Geert Uytterhoeven <geert+renesas@glider.be>

Add format specifiers for printing struct clk:
  - '%pC' or '%pCn': name (Common Clock Framework) or address (legacy
    clock framework) of the clock,
  - '%pCr': rate of the clock.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Documentation/printk-formats.txt | 12 ++++++++++++
 lib/vsprintf.c                   | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 11413036a63d1f7a..c287a9cec42cd20b 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -269,6 +269,18 @@ struct va_format:
 
 	Passed by reference.
 
+struct clk:
+
+	%pC	pll1
+	%pCn	pll1
+	%pCr	1560000000
+
+	For printing struct clk structures. '%pC' and '%pCn' print the name
+	(Common Clock Framework) or address (legacy clock framework) of the
+	structure; '%pCr' prints the current clock rate.
+
+	Passed by reference.
+
 
 Thank you for your cooperation and attention.
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 86907982a7702331..c0ae8ad034efd2aa 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -17,6 +17,7 @@
  */
 
 #include <stdarg.h>
+#include <linux/clk-provider.h>
 #include <linux/module.h>	/* for KSYM_SYMBOL_LEN */
 #include <linux/types.h>
 #include <linux/string.h>
@@ -1337,6 +1338,30 @@ char *comm_name(char *buf, char *end, struct task_struct *tsk,
 	return string(buf, end, name, spec);
 }
 
+static noinline_for_stack
+char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
+	    const char *fmt)
+{
+	if (!clk)
+		return string(buf, end, NULL, spec);
+
+	switch (fmt[1]) {
+	case 'r':
+		return number(buf, end, clk_get_rate(clk), spec);
+
+	case 'n':
+	default:
+#ifdef CONFIG_COMMON_CLK
+		return string(buf, end, __clk_get_name(clk), spec);
+#else
+		spec.base = 16;
+		spec.field_width = sizeof(unsigned long) * 2 + 2;
+		spec.flags |= SPECIAL | SMALL | ZEROPAD;
+		return number(buf, end, (unsigned long)clk, spec);
+#endif
+	}
+}
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1420,6 +1445,11 @@ int kptr_restrict __read_mostly;
  * - 'd[234]' For a dentry name (optionally 2-4 last components)
  * - 'D[234]' Same as 'd' but for a struct file
  * - 'T' task_struct->comm
+ * - 'C' For a clock, it prints the name (Common Clock Framework) or address
+ *       (legacy clock framework) of the clock
+ * - 'Cn' For a clock, it prints the name (Common Clock Framework) or address
+ *        (legacy clock framework) of the clock
+ * - 'Cr' For a clock, it prints the current rate of the clock
  *
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a
@@ -1570,6 +1600,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 				   spec, fmt);
 	case 'T':
 		return comm_name(buf, end, ptr, spec, fmt);
+	case 'C':
+		return clock(buf, end, ptr, spec, fmt);
 	}
 	spec.flags |= SMALL;
 	if (spec.field_width == -1) {
@@ -1818,6 +1850,11 @@ qualifier:
  * %*pE[achnops] print an escaped buffer
  * %*ph[CDN] a variable-length hex string with a separator (supports up to 64
  *           bytes of the input)
+ * %pC output the name (Common Clock Framework) or address (legacy clock
+ *     framework) of a clock
+ * %pCn output the name (Common Clock Framework) or address (legacy clock
+ *      framework) of a clock
+ * %pCr output the current rate of a clock
  * %n is ignored
  *
  * ** Please update Documentation/printk-formats.txt when making changes **
-- 
1.9.1

Re: [PATCH 0/3] lib/vsprintf: Doc improvements and clock support

[Stephen Boyd]

On 02/26/15 03:13, Geert Uytterhoeven wrote:
> From: Geert Uytterhoeven <geert+renesas@glider.be>
>
> 	Hi,
>
> This patch series improves the documentation for printk() formats, and
> adds support for printing clocks. The latter has always been a hassle if
> you wanted to support both the common and legacy clock frameworks.

Would the hassle go away if everyone moved to the common framework? I'm
mostly trying to understand the justification.

Also, I wonder why we wouldn't want to add formats for all the other
handle based frameworks that we have nowdays (gpio, regulator, pinctrl,
etc.).

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 0/3] lib/vsprintf: Doc improvements and clock support

[Geert Uytterhoeven]

Hi Stephen,

On Fri, Feb 27, 2015 at 1:02 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 02/26/15 03:13, Geert Uytterhoeven wrote:
>> From: Geert Uytterhoeven <geert+renesas@glider.be>
>> This patch series improves the documentation for printk() formats, and
>> adds support for printing clocks. The latter has always been a hassle if
>> you wanted to support both the common and legacy clock frameworks.
>
> Would the hassle go away if everyone moved to the common framework? I'm

Not anymore, now clk->name is gone ;-)

> mostly trying to understand the justification.

As a driver writer, once in a while I need to print out names and rates for
clocks and their parents from driver debug code.
As before this involved including clk_private.h and having #ifdefs to
support both
legacy and CCF, that debug code was alwas stripped from drivers before
submission, and never went upstream.

> Also, I wonder why we wouldn't want to add formats for all the other
> handle based frameworks that we have nowdays (gpio, regulator, pinctrl,
> etc.).

If someone feels the need to print those, support can be added.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/3] lib/vsprintf: Add %pC{,n,r} format specifiers for clocks

[Andrew Morton]

On Thu, 26 Feb 2015 12:13:03 +0100 Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> From: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Add format specifiers for printing struct clk:
>   - '%pC' or '%pCn': name (Common Clock Framework) or address (legacy
>     clock framework) of the clock,
>   - '%pCr': rate of the clock.
> 
> ...
>
> +static noinline_for_stack
> +char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
> +	    const char *fmt)
> +{
> +	if (!clk)
> +		return string(buf, end, NULL, spec);
> +
> +	switch (fmt[1]) {
> +	case 'r':
> +		return number(buf, end, clk_get_rate(clk), spec);
> +
> +	case 'n':
> +	default:
> +#ifdef CONFIG_COMMON_CLK
> +		return string(buf, end, __clk_get_name(clk), spec);
> +#else
> +		spec.base = 16;
> +		spec.field_width = sizeof(unsigned long) * 2 + 2;
> +		spec.flags |= SPECIAL | SMALL | ZEROPAD;
> +		return number(buf, end, (unsigned long)clk, spec);
> +#endif
> +	}
> +}

Seems a bit cruel to teeny systems which don't implement clock.  How does
this look?  Saves 160 bytes in each powerpc build!

static noinline_for_stack
char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
	    const char *fmt)
{
#ifdef CONFIG_HAVE_CLK
	if (clk) {
		switch (fmt[1]) {
		case 'r':
			return number(buf, end, clk_get_rate(clk), spec);

		case 'n':
		default:
#ifdef CONFIG_COMMON_CLK
			return string(buf, end, __clk_get_name(clk), spec);
#else
			spec.base = 16;
			spec.field_width = sizeof(unsigned long) * 2 + 2;
			spec.flags |= SPECIAL | SMALL | ZEROPAD;
			return number(buf, end, (unsigned long)clk, spec);
#endif
		}
	}
#endif	/* CONFIG_HAVE_CLK */

	return string(buf, end, NULL, spec);
}

diff -puN lib/vsprintf.c~lib-vsprintf-add-%pcnr-format-specifiers-for-clocks-fix lib/vsprintf.c
--- a/lib/vsprintf.c~lib-vsprintf-add-%pcnr-format-specifiers-for-clocks-fix
+++ a/lib/vsprintf.c
@@ -1320,24 +1320,27 @@ static noinline_for_stack
 char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
 	    const char *fmt)
 {
-	if (!clk)
-		return string(buf, end, NULL, spec);
+#ifdef CONFIG_HAVE_CLK
+	if (clk) {
+		switch (fmt[1]) {
+		case 'r':
+			return number(buf, end, clk_get_rate(clk), spec);
 
-	switch (fmt[1]) {
-	case 'r':
-		return number(buf, end, clk_get_rate(clk), spec);
-
-	case 'n':
-	default:
-#ifdef CONFIG_COMMON_CLK
-		return string(buf, end, __clk_get_name(clk), spec);
-#else
-		spec.base = 16;
-		spec.field_width = sizeof(unsigned long) * 2 + 2;
-		spec.flags |= SPECIAL | SMALL | ZEROPAD;
-		return number(buf, end, (unsigned long)clk, spec);
-#endif
+		case 'n':
+		default:
+#ifdef CONFIG_COMMON_CLK
+			return string(buf, end, __clk_get_name(clk), spec);
+#else
+			spec.base = 16;
+			spec.field_width = sizeof(unsigned long) * 2 + 2;
+			spec.flags |= SPECIAL | SMALL | ZEROPAD;
+			return number(buf, end, (unsigned long)clk, spec);
+#endif
+		}
 	}
+#endif	/* CONFIG_HAVE_CLK */
+
+	return string(buf, end, NULL, spec);
 }
 
 int kptr_restrict __read_mostly;
_

Re: [PATCH 3/3] lib/vsprintf: Add %pC{,n,r} format specifiers for clocks

[Joe Perches]

On Fri, 2015-02-27 at 14:18 -0800, Andrew Morton wrote:
> On Thu, 26 Feb 2015 12:13:03 +0100 Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > Add format specifiers for printing struct clk:
> >   - '%pC' or '%pCn': name (Common Clock Framework) or address (legacy
> >     clock framework) of the clock,
> >   - '%pCr': rate of the clock.
[]
> Seems a bit cruel to teeny systems which don't implement clock.  How does
> this look?  Saves 160 bytes in each powerpc build!

Does this still emit a pointer value for those systems?
Glancing at it, does it just emit NULL?

Re: [PATCH 3/3] lib/vsprintf: Add %pC{,n,r} format specifiers for clocks

[Geert Uytterhoeven]

Hi Joe,

On Fri, Feb 27, 2015 at 11:55 PM, Joe Perches <joe@perches.com> wrote:
> On Fri, 2015-02-27 at 14:18 -0800, Andrew Morton wrote:
>> On Thu, 26 Feb 2015 12:13:03 +0100 Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> > Add format specifiers for printing struct clk:
>> >   - '%pC' or '%pCn': name (Common Clock Framework) or address (legacy
>> >     clock framework) of the clock,
>> >   - '%pCr': rate of the clock.
> []
>> Seems a bit cruel to teeny systems which don't implement clock.  How does
>> this look?  Saves 160 bytes in each powerpc build!
>
> Does this still emit a pointer value for those systems?
> Glancing at it, does it just emit NULL?

Yes, there's a "return string(buf, end, NULL, spec);" at the end.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/3] lib/vsprintf: Add %pC{,n,r} format specifiers for clocks

[Geert Uytterhoeven]

Hi Andrew,

On Fri, Feb 27, 2015 at 11:18 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Thu, 26 Feb 2015 12:13:03 +0100 Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
>> From: Geert Uytterhoeven <geert+renesas@glider.be>
>>
>> Add format specifiers for printing struct clk:
>>   - '%pC' or '%pCn': name (Common Clock Framework) or address (legacy
>>     clock framework) of the clock,
>>   - '%pCr': rate of the clock.
>>
>> ...
>>
>> +static noinline_for_stack
>> +char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
>> +         const char *fmt)
>> +{
>> +     if (!clk)
>> +             return string(buf, end, NULL, spec);
>> +
>> +     switch (fmt[1]) {
>> +     case 'r':
>> +             return number(buf, end, clk_get_rate(clk), spec);
>> +
>> +     case 'n':
>> +     default:
>> +#ifdef CONFIG_COMMON_CLK
>> +             return string(buf, end, __clk_get_name(clk), spec);
>> +#else
>> +             spec.base = 16;
>> +             spec.field_width = sizeof(unsigned long) * 2 + 2;
>> +             spec.flags |= SPECIAL | SMALL | ZEROPAD;
>> +             return number(buf, end, (unsigned long)clk, spec);
>> +#endif
>> +     }
>> +}
>
> Seems a bit cruel to teeny systems which don't implement clock.  How does
> this look?  Saves 160 bytes in each powerpc build!

> diff -puN lib/vsprintf.c~lib-vsprintf-add-%pcnr-format-specifiers-for-clocks-fix lib/vsprintf.c
> --- a/lib/vsprintf.c~lib-vsprintf-add-%pcnr-format-specifiers-for-clocks-fix
> +++ a/lib/vsprintf.c
> @@ -1320,24 +1320,27 @@ static noinline_for_stack
>  char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
>             const char *fmt)
>  {
> -       if (!clk)
> -               return string(buf, end, NULL, spec);
> +#ifdef CONFIG_HAVE_CLK
> +       if (clk) {

On second thought, you can achieve the same result without #ifdef
with this 2-line change:

-       if (!clk)
+       if (!IS_ENABLED(CONFIG_HAVE_CLK) || !clk)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds