[PATCH v1] kunit: fix failure to build without printk

[PATCH v1] kunit: fix failure to build without printk

[Brendan Higgins]

Previously KUnit assumed that printk would always be present, which is
not a valid assumption to make. Fix that by ifdefing out functions which
directly depend on printk core functions similar to what dev_printk
does.

Reported-by: Randy Dunlap <rdunlap@infradead.org>
Link: https://lore.kernel.org/linux-kselftest/0352fae9-564f-4a97-715a-fabe016259df@kernel.org/T/#t
Cc: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
 include/kunit/test.h |  7 +++++++
 kunit/test.c         | 41 ++++++++++++++++++++++++-----------------
 2 files changed, 31 insertions(+), 17 deletions(-)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index 8b7eb03d4971..339af5f95c4a 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
 
 void kunit_cleanup(struct kunit *test);
 
+#ifdef CONFIG_PRINTK
 void __printf(3, 4) kunit_printk(const char *level,
 				 const struct kunit *test,
 				 const char *fmt, ...);
+#else
+static inline void __printf(3, 4) kunit_printk(const char *level,
+					       const struct kunit *test,
+					       const char *fmt, ...)
+{}
+#endif
 
 /**
  * kunit_info() - Prints an INFO level message associated with @test.
diff --git a/kunit/test.c b/kunit/test.c
index b2ca9b94c353..0aa1caf07a6b 100644
--- a/kunit/test.c
+++ b/kunit/test.c
@@ -16,6 +16,7 @@ static void kunit_set_failure(struct kunit *test)
 	WRITE_ONCE(test->success, false);
 }
 
+#ifdef CONFIG_PRINTK
 static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
 {
 	return vprintk_emit(0, level, NULL, 0, fmt, args);
@@ -40,6 +41,29 @@ static void kunit_vprintk(const struct kunit *test,
 	kunit_printk_emit(level[1] - '0', "\t# %s: %pV", test->name, vaf);
 }
 
+void kunit_printk(const char *level,
+		  const struct kunit *test,
+		  const char *fmt, ...)
+{
+	struct va_format vaf;
+	va_list args;
+
+	va_start(args, fmt);
+
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	kunit_vprintk(test, level, &vaf);
+
+	va_end(args);
+}
+#else /* CONFIG_PRINTK */
+static inline int kunit_printk_emit(int level, const char *fmt, ...)
+{
+	return 0;
+}
+#endif /* CONFIG_PRINTK */
+
 static void kunit_print_tap_version(void)
 {
 	static bool kunit_has_printed_tap_version;
@@ -504,20 +528,3 @@ void kunit_cleanup(struct kunit *test)
 		kunit_resource_free(test, resource);
 	}
 }
-
-void kunit_printk(const char *level,
-		  const struct kunit *test,
-		  const char *fmt, ...)
-{
-	struct va_format vaf;
-	va_list args;
-
-	va_start(args, fmt);
-
-	vaf.fmt = fmt;
-	vaf.va = &args;
-
-	kunit_vprintk(test, level, &vaf);
-
-	va_end(args);
-}
-- 
2.23.0.187.g17f5b7556c-goog

Re: [PATCH v1] kunit: fix failure to build without printk

[Randy Dunlap]

On 8/27/19 10:49 AM, Brendan Higgins wrote:
> Previously KUnit assumed that printk would always be present, which is
> not a valid assumption to make. Fix that by ifdefing out functions which
> directly depend on printk core functions similar to what dev_printk
> does.
> 
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Link: https://lore.kernel.org/linux-kselftest/0352fae9-564f-4a97-715a-fabe016259df@kernel.org/T/#t
> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>

Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested

Thanks.

> ---
>  include/kunit/test.h |  7 +++++++
>  kunit/test.c         | 41 ++++++++++++++++++++++++-----------------
>  2 files changed, 31 insertions(+), 17 deletions(-)



-- 
~Randy

Re: [PATCH v1] kunit: fix failure to build without printk

[shuah]

On 8/27/19 11:49 AM, Brendan Higgins wrote:
> Previously KUnit assumed that printk would always be present, which is
> not a valid assumption to make. Fix that by ifdefing out functions which
> directly depend on printk core functions similar to what dev_printk
> does.
> 
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Link: https://lore.kernel.org/linux-kselftest/0352fae9-564f-4a97-715a-fabe016259df@kernel.org/T/#t
> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---
>   include/kunit/test.h |  7 +++++++
>   kunit/test.c         | 41 ++++++++++++++++++++++++-----------------
>   2 files changed, 31 insertions(+), 17 deletions(-)
> 
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 8b7eb03d4971..339af5f95c4a 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
>   
>   void kunit_cleanup(struct kunit *test);
>   
> +#ifdef CONFIG_PRINTK

Please make this #if defined(CONFIG_PRINTK)

>   void __printf(3, 4) kunit_printk(const char *level,

Line these two up with const char *level,

>   				 const struct kunit *test,
>   				 const char *fmt, ...);
> +#else
> +static inline void __printf(3, 4) kunit_printk(const char *level,
> +					       const struct kunit *test,
> +					       const char *fmt, ...)

Same here.

> +{}

Either line this up or make it

const char *fmt, ...) { }

It is hard to read the way it is currently indented.

> +#endif
>   
>   /**
>    * kunit_info() - Prints an INFO level message associated with @test.
> diff --git a/kunit/test.c b/kunit/test.c
> index b2ca9b94c353..0aa1caf07a6b 100644
> --- a/kunit/test.c
> +++ b/kunit/test.c
> @@ -16,6 +16,7 @@ static void kunit_set_failure(struct kunit *test)
>   	WRITE_ONCE(test->success, false);
>   }
>   
> +#ifdef CONFIG_PRINTK

Same here - if defined

>   static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
>   {
>   	return vprintk_emit(0, level, NULL, 0, fmt, args);
> @@ -40,6 +41,29 @@ static void kunit_vprintk(const struct kunit *test,
>   	kunit_printk_emit(level[1] - '0', "\t# %s: %pV", test->name, vaf);
>   }
>   
> +void kunit_printk(const char *level,
> +		  const struct kunit *test,
> +		  const char *fmt, ...)

Line the arguments up.

> +{
> +	struct va_format vaf;
> +	va_list args;
> +
> +	va_start(args, fmt);
> +
> +	vaf.fmt = fmt;
> +	vaf.va = &args;
> +
> +	kunit_vprintk(test, level, &vaf);
> +
> +	va_end(args);
> +}
> +#else /* CONFIG_PRINTK */
> +static inline int kunit_printk_emit(int level, const char *fmt, ...)
> +{
> +	return 0;

Is there a reason to not use
> +} > +#endif /* CONFIG_PRINTK */
> +
>   static void kunit_print_tap_version(void)
>   {
>   	static bool kunit_has_printed_tap_version;
> @@ -504,20 +528,3 @@ void kunit_cleanup(struct kunit *test)
>   		kunit_resource_free(test, resource);
>   	}
>   }
> -
> -void kunit_printk(const char *level,
> -		  const struct kunit *test,
> -		  const char *fmt, ...) > -{
> -	struct va_format vaf;
> -	va_list args;
> -
> -	va_start(args, fmt);
> -
> -	vaf.fmt = fmt;
> -	vaf.va = &args;
> -
> -	kunit_vprintk(test, level, &vaf);
> -
> -	va_end(args);
> -}
> 

Okay after reviewing this, I am not sure why you need to do all
this.

Why can't you just change the root function that throws the warn:

  static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
{
         return vprintk_emit(0, level, NULL, 0, fmt, args);
}

You aren'r really doing anything extra here, other than calling
vprintk_emit()

Unless I am missing something, can't you solve this problem by including
printk.h and let it handle the !CONFIG_PRINTK case?

thanks,
-- Shuah

Re: [PATCH v1] kunit: fix failure to build without printk

[Randy Dunlap]

On 8/27/19 1:21 PM, shuah wrote:
> On 8/27/19 11:49 AM, Brendan Higgins wrote:
>> Previously KUnit assumed that printk would always be present, which is
>> not a valid assumption to make. Fix that by ifdefing out functions which
>> directly depend on printk core functions similar to what dev_printk
>> does.
>>
>> Reported-by: Randy Dunlap <rdunlap@infradead.org>
>> Link: https://lore.kernel.org/linux-kselftest/0352fae9-564f-4a97-715a-fabe016259df@kernel.org/T/#t
>> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
>> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
>> ---
>>   include/kunit/test.h |  7 +++++++
>>   kunit/test.c         | 41 ++++++++++++++++++++++++-----------------
>>   2 files changed, 31 insertions(+), 17 deletions(-)
>>
>> diff --git a/include/kunit/test.h b/include/kunit/test.h
>> index 8b7eb03d4971..339af5f95c4a 100644
>> --- a/include/kunit/test.h
>> +++ b/include/kunit/test.h
>> @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
>>     void kunit_cleanup(struct kunit *test);
>>   +#ifdef CONFIG_PRINTK
> 
> Please make this #if defined(CONFIG_PRINTK)

explain why, please?

thanks.
-- 
~Randy

Re: [PATCH v1] kunit: fix failure to build without printk

[shuah]

On 8/27/19 2:53 PM, Randy Dunlap wrote:
> On 8/27/19 1:21 PM, shuah wrote:
>> On 8/27/19 11:49 AM, Brendan Higgins wrote:
>>> Previously KUnit assumed that printk would always be present, which is
>>> not a valid assumption to make. Fix that by ifdefing out functions which
>>> directly depend on printk core functions similar to what dev_printk
>>> does.
>>>
>>> Reported-by: Randy Dunlap <rdunlap@infradead.org>
>>> Link: https://lore.kernel.org/linux-kselftest/0352fae9-564f-4a97-715a-fabe016259df@kernel.org/T/#t
>>> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
>>> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
>>> ---
>>>    include/kunit/test.h |  7 +++++++
>>>    kunit/test.c         | 41 ++++++++++++++++++++++++-----------------
>>>    2 files changed, 31 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/include/kunit/test.h b/include/kunit/test.h
>>> index 8b7eb03d4971..339af5f95c4a 100644
>>> --- a/include/kunit/test.h
>>> +++ b/include/kunit/test.h
>>> @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
>>>      void kunit_cleanup(struct kunit *test);
>>>    +#ifdef CONFIG_PRINTK
>>
>> Please make this #if defined(CONFIG_PRINTK)
> 
> explain why, please?
> 
> thanks.
> 

This can be used to do compound logic. I have been using this style for
that reason starting a couple of years now. I seem to work in code paths
where I have to look for multiple config vars.

In this case, it probably doesn't matter as much either way.

thanks,
-- Shuah

Re: [PATCH v1] kunit: fix failure to build without printk

[Brendan Higgins]

On Tue, Aug 27, 2019 at 1:21 PM shuah <shuah@kernel.org> wrote:
>
> On 8/27/19 11:49 AM, Brendan Higgins wrote:
> > Previously KUnit assumed that printk would always be present, which is
> > not a valid assumption to make. Fix that by ifdefing out functions which
> > directly depend on printk core functions similar to what dev_printk
> > does.
> >
> > Reported-by: Randy Dunlap <rdunlap@infradead.org>
> > Link: https://lore.kernel.org/linux-kselftest/0352fae9-564f-4a97-715a-fabe016259df@kernel.org/T/#t
> > Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > ---
> >   include/kunit/test.h |  7 +++++++
> >   kunit/test.c         | 41 ++++++++++++++++++++++++-----------------
> >   2 files changed, 31 insertions(+), 17 deletions(-)
> >
> > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > index 8b7eb03d4971..339af5f95c4a 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
[...]
> Okay after reviewing this, I am not sure why you need to do all
> this.
>
> Why can't you just change the root function that throws the warn:
>
>   static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> {
>          return vprintk_emit(0, level, NULL, 0, fmt, args);
> }
>
> You aren'r really doing anything extra here, other than calling
> vprintk_emit()

Yeah, I did that a while ago. I think it was a combination of trying
to avoid an extra layer of adding and then removing the log level, and
that's what dev_printk and friends did.

But I think you are probably right. It's a lot of maintenance overhead
to get rid of that. Probably best to just use what the printk people
have.

> Unless I am missing something, can't you solve this problem by including
> printk.h and let it handle the !CONFIG_PRINTK case?

Randy, I hope you don't mind, but I am going to ask you to re-ack my
next revision since it basically addresses the problem in a totally
different way.

Re: [PATCH v1] kunit: fix failure to build without printk

[Brendan Higgins]

On Tue, Aug 27, 2019 at 2:03 PM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> On Tue, Aug 27, 2019 at 1:21 PM shuah <shuah@kernel.org> wrote:
> >
> > On 8/27/19 11:49 AM, Brendan Higgins wrote:
> > > Previously KUnit assumed that printk would always be present, which is
> > > not a valid assumption to make. Fix that by ifdefing out functions which
> > > directly depend on printk core functions similar to what dev_printk
> > > does.
> > >
> > > Reported-by: Randy Dunlap <rdunlap@infradead.org>
> > > Link: https://lore.kernel.org/linux-kselftest/0352fae9-564f-4a97-715a-fabe016259df@kernel.org/T/#t
> > > Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> > > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > > ---
> > >   include/kunit/test.h |  7 +++++++
> > >   kunit/test.c         | 41 ++++++++++++++++++++++++-----------------
> > >   2 files changed, 31 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > > index 8b7eb03d4971..339af5f95c4a 100644
> > > --- a/include/kunit/test.h
> > > +++ b/include/kunit/test.h
> > > @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
> [...]
> > Okay after reviewing this, I am not sure why you need to do all
> > this.
> >
> > Why can't you just change the root function that throws the warn:
> >
> >   static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> > {
> >          return vprintk_emit(0, level, NULL, 0, fmt, args);
> > }
> >
> > You aren'r really doing anything extra here, other than calling
> > vprintk_emit()
>
> Yeah, I did that a while ago. I think it was a combination of trying
> to avoid an extra layer of adding and then removing the log level, and
> that's what dev_printk and friends did.
>
> But I think you are probably right. It's a lot of maintenance overhead
> to get rid of that. Probably best to just use what the printk people
> have.
>
> > Unless I am missing something, can't you solve this problem by including
> > printk.h and let it handle the !CONFIG_PRINTK case?
>
> Randy, I hope you don't mind, but I am going to ask you to re-ack my
> next revision since it basically addresses the problem in a totally
> different way.

Actually, scratch that. Checkpatch doesn't like me calling printk
without using a KERN_<LEVEL>.

Now that I am thinking back to when I wrote this. I think it also
might not like using a dynamic KERN_<LEVEL> either (printk("%s my
message", KERN_INFO)).

I am going to have to do some more investigation.

Re: [PATCH v1] kunit: fix failure to build without printk

[Brendan Higgins]

On Tue, Aug 27, 2019 at 2:09 PM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> On Tue, Aug 27, 2019 at 2:03 PM Brendan Higgins
> <brendanhiggins@google.com> wrote:
> >
> > On Tue, Aug 27, 2019 at 1:21 PM shuah <shuah@kernel.org> wrote:
> > >
> > > On 8/27/19 11:49 AM, Brendan Higgins wrote:
> > > > Previously KUnit assumed that printk would always be present, which is
> > > > not a valid assumption to make. Fix that by ifdefing out functions which
> > > > directly depend on printk core functions similar to what dev_printk
> > > > does.
> > > >
> > > > Reported-by: Randy Dunlap <rdunlap@infradead.org>
> > > > Link: https://lore.kernel.org/linux-kselftest/0352fae9-564f-4a97-715a-fabe016259df@kernel.org/T/#t
> > > > Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> > > > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > > > ---
> > > >   include/kunit/test.h |  7 +++++++
> > > >   kunit/test.c         | 41 ++++++++++++++++++++++++-----------------
> > > >   2 files changed, 31 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > > > index 8b7eb03d4971..339af5f95c4a 100644
> > > > --- a/include/kunit/test.h
> > > > +++ b/include/kunit/test.h
> > > > @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
> > [...]
> > > Okay after reviewing this, I am not sure why you need to do all
> > > this.
> > >
> > > Why can't you just change the root function that throws the warn:
> > >
> > >   static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> > > {
> > >          return vprintk_emit(0, level, NULL, 0, fmt, args);
> > > }
> > >
> > > You aren'r really doing anything extra here, other than calling
> > > vprintk_emit()
> >
> > Yeah, I did that a while ago. I think it was a combination of trying
> > to avoid an extra layer of adding and then removing the log level, and
> > that's what dev_printk and friends did.
> >
> > But I think you are probably right. It's a lot of maintenance overhead
> > to get rid of that. Probably best to just use what the printk people
> > have.
> >
> > > Unless I am missing something, can't you solve this problem by including
> > > printk.h and let it handle the !CONFIG_PRINTK case?
> >
> > Randy, I hope you don't mind, but I am going to ask you to re-ack my
> > next revision since it basically addresses the problem in a totally
> > different way.
>
> Actually, scratch that. Checkpatch doesn't like me calling printk
> without using a KERN_<LEVEL>.
>
> Now that I am thinking back to when I wrote this. I think it also
> might not like using a dynamic KERN_<LEVEL> either (printk("%s my
> message", KERN_INFO)).
>
> I am going to have to do some more investigation.

Alright, I am pretty sure it is safe to do printk("%smessage", KERN_<LEVEL>);

Looking at the printk implementation, it appears to do the format
before it checks the log level:

https://elixir.bootlin.com/linux/v5.2.10/source/kernel/printk/printk.c#L1907

So I am pretty sure we can do it either with the vprintk_emit or with printk.

So it appears that we have to weigh the following trade-offs:

Using vprintk_emit:

Pros:
 - That's what dev_printk uses.
 - No checkpatch warnings.

Cons:
 - Harder to maintain (requires ifdefery).

Using printk:

Pros:
 - Easier to maintain.

Cons:
 - I am less confident that it is correct (I am not 100% certain that
printk is intended to be used this way).
 - Checkpatch warnings.
 - Extra layer of string formatting.

My preference is to go the vprintk_emit route since I am more
confident that it is right, but I don't have a strong preference.

Re: [PATCH v1] kunit: fix failure to build without printk

[shuah]

On 8/27/19 3:36 PM, Brendan Higgins wrote:
> On Tue, Aug 27, 2019 at 2:09 PM Brendan Higgins
> <brendanhiggins@google.com> wrote:
>>
>> On Tue, Aug 27, 2019 at 2:03 PM Brendan Higgins
>> <brendanhiggins@google.com> wrote:
>>>
>>> On Tue, Aug 27, 2019 at 1:21 PM shuah <shuah@kernel.org> wrote:
>>>>
>>>> On 8/27/19 11:49 AM, Brendan Higgins wrote:
>>>>> Previously KUnit assumed that printk would always be present, which is
>>>>> not a valid assumption to make. Fix that by ifdefing out functions which
>>>>> directly depend on printk core functions similar to what dev_printk
>>>>> does.
>>>>>
>>>>> Reported-by: Randy Dunlap <rdunlap@infradead.org>
>>>>> Link: https://lore.kernel.org/linux-kselftest/0352fae9-564f-4a97-715a-fabe016259df@kernel.org/T/#t
>>>>> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
>>>>> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
>>>>> ---
>>>>>    include/kunit/test.h |  7 +++++++
>>>>>    kunit/test.c         | 41 ++++++++++++++++++++++++-----------------
>>>>>    2 files changed, 31 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/include/kunit/test.h b/include/kunit/test.h
>>>>> index 8b7eb03d4971..339af5f95c4a 100644
>>>>> --- a/include/kunit/test.h
>>>>> +++ b/include/kunit/test.h
>>>>> @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
>>> [...]
>>>> Okay after reviewing this, I am not sure why you need to do all
>>>> this.
>>>>
>>>> Why can't you just change the root function that throws the warn:
>>>>
>>>>    static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
>>>> {
>>>>           return vprintk_emit(0, level, NULL, 0, fmt, args);
>>>> }
>>>>
>>>> You aren'r really doing anything extra here, other than calling
>>>> vprintk_emit()
>>>
>>> Yeah, I did that a while ago. I think it was a combination of trying
>>> to avoid an extra layer of adding and then removing the log level, and
>>> that's what dev_printk and friends did.
>>>
>>> But I think you are probably right. It's a lot of maintenance overhead
>>> to get rid of that. Probably best to just use what the printk people
>>> have.
>>>
>>>> Unless I am missing something, can't you solve this problem by including
>>>> printk.h and let it handle the !CONFIG_PRINTK case?
>>>
>>> Randy, I hope you don't mind, but I am going to ask you to re-ack my
>>> next revision since it basically addresses the problem in a totally
>>> different way.
>>
>> Actually, scratch that. Checkpatch doesn't like me calling printk
>> without using a KERN_<LEVEL>.
>>
>> Now that I am thinking back to when I wrote this. I think it also
>> might not like using a dynamic KERN_<LEVEL> either (printk("%s my
>> message", KERN_INFO)).
>>
>> I am going to have to do some more investigation.
> 
> Alright, I am pretty sure it is safe to do printk("%smessage", KERN_<LEVEL>);
> 
> Looking at the printk implementation, it appears to do the format
> before it checks the log level:
> 
> https://elixir.bootlin.com/linux/v5.2.10/source/kernel/printk/printk.c#L1907
> 
> So I am pretty sure we can do it either with the vprintk_emit or with printk.

Let me see if we are on the same page first. I am asking if you can
just include printk.h for vprintk_emit() define for both CONFIG_PRINTK
and !CONFIG_PRINTK cases.

I am not asking you to use printk() in place of vprintk_emit().
It is perfectly fine to use vprintk_emit()

> 
> So it appears that we have to weigh the following trade-offs:
> 
> Using vprintk_emit:
> 
> Pros:
>   - That's what dev_printk uses.

Not sure what you mean by this. I am suggesting if you can just
call vprintk_emit() and include printk.h and not have to ifdef
around all the other callers of kunit_vprintk_emit()

Yes. There is the other issue of why do you need the complexity
of having kunit_vprintk_emit() at all.

thanks,
-- Shuah

Re: [PATCH v1] kunit: fix failure to build without printk

[Brendan Higgins]

On Tue, Aug 27, 2019 at 3:00 PM shuah <shuah@kernel.org> wrote:
>
> On 8/27/19 3:36 PM, Brendan Higgins wrote:
> > On Tue, Aug 27, 2019 at 2:09 PM Brendan Higgins
> > <brendanhiggins@google.com> wrote:
> >>
> >> On Tue, Aug 27, 2019 at 2:03 PM Brendan Higgins
> >> <brendanhiggins@google.com> wrote:
> >>>
> >>> On Tue, Aug 27, 2019 at 1:21 PM shuah <shuah@kernel.org> wrote:
> >>>>
> >>>> On 8/27/19 11:49 AM, Brendan Higgins wrote:
> >>>>> Previously KUnit assumed that printk would always be present, which is
> >>>>> not a valid assumption to make. Fix that by ifdefing out functions which
> >>>>> directly depend on printk core functions similar to what dev_printk
> >>>>> does.
> >>>>>
> >>>>> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> >>>>> Link: https://lore.kernel.org/linux-kselftest/0352fae9-564f-4a97-715a-fabe016259df@kernel.org/T/#t
> >>>>> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> >>>>> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> >>>>> ---
> >>>>>    include/kunit/test.h |  7 +++++++
> >>>>>    kunit/test.c         | 41 ++++++++++++++++++++++++-----------------
> >>>>>    2 files changed, 31 insertions(+), 17 deletions(-)
> >>>>>
> >>>>> diff --git a/include/kunit/test.h b/include/kunit/test.h
> >>>>> index 8b7eb03d4971..339af5f95c4a 100644
> >>>>> --- a/include/kunit/test.h
> >>>>> +++ b/include/kunit/test.h
> >>>>> @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
> >>> [...]
> >>>> Okay after reviewing this, I am not sure why you need to do all
> >>>> this.
> >>>>
> >>>> Why can't you just change the root function that throws the warn:
> >>>>
> >>>>    static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> >>>> {
> >>>>           return vprintk_emit(0, level, NULL, 0, fmt, args);
> >>>> }
> >>>>
> >>>> You aren'r really doing anything extra here, other than calling
> >>>> vprintk_emit()
> >>>
> >>> Yeah, I did that a while ago. I think it was a combination of trying
> >>> to avoid an extra layer of adding and then removing the log level, and
> >>> that's what dev_printk and friends did.
> >>>
> >>> But I think you are probably right. It's a lot of maintenance overhead
> >>> to get rid of that. Probably best to just use what the printk people
> >>> have.
> >>>
> >>>> Unless I am missing something, can't you solve this problem by including
> >>>> printk.h and let it handle the !CONFIG_PRINTK case?
> >>>
> >>> Randy, I hope you don't mind, but I am going to ask you to re-ack my
> >>> next revision since it basically addresses the problem in a totally
> >>> different way.
> >>
> >> Actually, scratch that. Checkpatch doesn't like me calling printk
> >> without using a KERN_<LEVEL>.
> >>
> >> Now that I am thinking back to when I wrote this. I think it also
> >> might not like using a dynamic KERN_<LEVEL> either (printk("%s my
> >> message", KERN_INFO)).
> >>
> >> I am going to have to do some more investigation.
> >
> > Alright, I am pretty sure it is safe to do printk("%smessage", KERN_<LEVEL>);
> >
> > Looking at the printk implementation, it appears to do the format
> > before it checks the log level:
> >
> > https://elixir.bootlin.com/linux/v5.2.10/source/kernel/printk/printk.c#L1907
> >
> > So I am pretty sure we can do it either with the vprintk_emit or with printk.
>
> Let me see if we are on the same page first. I am asking if you can
> just include printk.h for vprintk_emit() define for both CONFIG_PRINTK
> and !CONFIG_PRINTK cases.

Ah sorry, I misunderstood you.

No, that doesn't work. I tried including linux/printk.h, and I get the
same error.

The reason for this is that vprintk_emit() is only defined when CONFIG_PRINTK=y:

https://elixir.bootlin.com/linux/latest/ident/vprintk_emit

> I am not asking you to use printk() in place of vprintk_emit().
> It is perfectly fine to use vprintk_emit()

Okay, cool.

> >
> > So it appears that we have to weigh the following trade-offs:
> >
> > Using vprintk_emit:
> >
> > Pros:
> >   - That's what dev_printk uses.
>
> Not sure what you mean by this. I am suggesting if you can just
> call vprintk_emit() and include printk.h and not have to ifdef
> around all the other callers of kunit_vprintk_emit()

Oh, I was just saying that I heavily based my implementation of
kunit_printk on dev_printk. So I have a high degree of confidence that
it is okay to use it the way that I am using it.

> Yes. There is the other issue of why do you need the complexity
> of having kunit_vprintk_emit() at all.

Right, and the problem with the alternative, is there is no good
kernel API for logging with the log level set dynamically. printk
prefers to have it as a string prefix on the format string, but I
cannot do that because I need to add my own prefix to the format
string.

So, I guess I should just go ahead and address the earlier comments on
this patch?

RE: [PATCH v1] kunit: fix failure to build without printk

[Tim.Bird]

> -----Original Message-----
> From: Brendan Higgins
> 
> On Tue, Aug 27, 2019 at 3:00 PM shuah <shuah@kernel.org> wrote:
> >
> > On 8/27/19 3:36 PM, Brendan Higgins wrote:
> > > On Tue, Aug 27, 2019 at 2:09 PM Brendan Higgins
> > > <brendanhiggins@google.com> wrote:
> > >>
> > >> On Tue, Aug 27, 2019 at 2:03 PM Brendan Higgins
> > >> <brendanhiggins@google.com> wrote:
> > >>>
> > >>> On Tue, Aug 27, 2019 at 1:21 PM shuah <shuah@kernel.org> wrote:
> > >>>>
> > >>>> On 8/27/19 11:49 AM, Brendan Higgins wrote:
> > >>>>> Previously KUnit assumed that printk would always be present,
> which is
> > >>>>> not a valid assumption to make. Fix that by ifdefing out functions
> which
> > >>>>> directly depend on printk core functions similar to what dev_printk
> > >>>>> does.
> > >>>>>
> > >>>>> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> > >>>>> Link: https://lore.kernel.org/linux-kselftest/0352fae9-564f-4a97-
> 715a-fabe016259df@kernel.org/T/#t
> > >>>>> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> > >>>>> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > >>>>> ---
> > >>>>>    include/kunit/test.h |  7 +++++++
> > >>>>>    kunit/test.c         | 41 ++++++++++++++++++++++++-----------------
> > >>>>>    2 files changed, 31 insertions(+), 17 deletions(-)
> > >>>>>
> > >>>>> diff --git a/include/kunit/test.h b/include/kunit/test.h
> > >>>>> index 8b7eb03d4971..339af5f95c4a 100644
> > >>>>> --- a/include/kunit/test.h
> > >>>>> +++ b/include/kunit/test.h
> > >>>>> @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit
> *test, size_t size, gfp_t gfp)
> > >>> [...]
> > >>>> Okay after reviewing this, I am not sure why you need to do all
> > >>>> this.
> > >>>>
> > >>>> Why can't you just change the root function that throws the warn:
> > >>>>
> > >>>>    static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> > >>>> {
> > >>>>           return vprintk_emit(0, level, NULL, 0, fmt, args);
> > >>>> }
> > >>>>
> > >>>> You aren'r really doing anything extra here, other than calling
> > >>>> vprintk_emit()
> > >>>
> > >>> Yeah, I did that a while ago. I think it was a combination of trying
> > >>> to avoid an extra layer of adding and then removing the log level, and
> > >>> that's what dev_printk and friends did.
> > >>>
> > >>> But I think you are probably right. It's a lot of maintenance overhead
> > >>> to get rid of that. Probably best to just use what the printk people
> > >>> have.
> > >>>
> > >>>> Unless I am missing something, can't you solve this problem by
> including
> > >>>> printk.h and let it handle the !CONFIG_PRINTK case?
> > >>>
> > >>> Randy, I hope you don't mind, but I am going to ask you to re-ack my
> > >>> next revision since it basically addresses the problem in a totally
> > >>> different way.
> > >>
> > >> Actually, scratch that. Checkpatch doesn't like me calling printk
> > >> without using a KERN_<LEVEL>.
> > >>
> > >> Now that I am thinking back to when I wrote this. I think it also
> > >> might not like using a dynamic KERN_<LEVEL> either (printk("%s my
> > >> message", KERN_INFO)).
> > >>
> > >> I am going to have to do some more investigation.
> > >
> > > Alright, I am pretty sure it is safe to do printk("%smessage",
> KERN_<LEVEL>);
> > >
> > > Looking at the printk implementation, it appears to do the format
> > > before it checks the log level:
> > >
> > >
> https://elixir.bootlin.com/linux/v5.2.10/source/kernel/printk/printk.c#L1907
> > >
> > > So I am pretty sure we can do it either with the vprintk_emit or with
> printk.
> >
> > Let me see if we are on the same page first. I am asking if you can
> > just include printk.h for vprintk_emit() define for both CONFIG_PRINTK
> > and !CONFIG_PRINTK cases.
> 
> Ah sorry, I misunderstood you.
> 
> No, that doesn't work. I tried including linux/printk.h, and I get the
> same error.
> 
> The reason for this is that vprintk_emit() is only defined when
> CONFIG_PRINTK=y:
> 
> https://elixir.bootlin.com/linux/latest/ident/vprintk_emit

Ugh.  That's just a bug in include/linux/printk.h

There should be a stub definition for vprintk_emit() in the #else part 
of #ifdef CONFIG_PRINTK.

You shouldn't be dealing with whether printk is present or not
in the kunit code.  All the printk-related routines are supposed
to evaporate themselves, based on the conditional in
include/linux/printk.h

That should be fixed there instead of in your code.

Let me know if you'd like me to submit a patch for that.  I only hesitate
because your patch depends on it, and IMHO it makes more sense to
include it in your batch than separately. Otherwise there's a patch race
condition.
 -- Tim

Re: [PATCH v1] kunit: fix failure to build without printk

[Brendan Higgins]

On Tue, Aug 27, 2019 at 3:38 PM <Tim.Bird@sony.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Brendan Higgins
> >
> > On Tue, Aug 27, 2019 at 3:00 PM shuah <shuah@kernel.org> wrote:
> > >
> > > On 8/27/19 3:36 PM, Brendan Higgins wrote:
> > > > On Tue, Aug 27, 2019 at 2:09 PM Brendan Higgins
> > > > <brendanhiggins@google.com> wrote:
> > > >>
> > > >> On Tue, Aug 27, 2019 at 2:03 PM Brendan Higgins
> > > >> <brendanhiggins@google.com> wrote:
> > > >>>
> > > >>> On Tue, Aug 27, 2019 at 1:21 PM shuah <shuah@kernel.org> wrote:
> > > >>>>
> > > >>>> On 8/27/19 11:49 AM, Brendan Higgins wrote:
> > > >>>>> Previously KUnit assumed that printk would always be present,
> > which is
> > > >>>>> not a valid assumption to make. Fix that by ifdefing out functions
> > which
> > > >>>>> directly depend on printk core functions similar to what dev_printk
> > > >>>>> does.
> > > >>>>>
> > > >>>>> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> > > >>>>> Link: https://lore.kernel.org/linux-kselftest/0352fae9-564f-4a97-
> > 715a-fabe016259df@kernel.org/T/#t
> > > >>>>> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> > > >>>>> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > > >>>>> ---
> > > >>>>>    include/kunit/test.h |  7 +++++++
> > > >>>>>    kunit/test.c         | 41 ++++++++++++++++++++++++-----------------
> > > >>>>>    2 files changed, 31 insertions(+), 17 deletions(-)
> > > >>>>>
> > > >>>>> diff --git a/include/kunit/test.h b/include/kunit/test.h
> > > >>>>> index 8b7eb03d4971..339af5f95c4a 100644
> > > >>>>> --- a/include/kunit/test.h
> > > >>>>> +++ b/include/kunit/test.h
> > > >>>>> @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit
> > *test, size_t size, gfp_t gfp)
> > > >>> [...]
> > > >>>> Okay after reviewing this, I am not sure why you need to do all
> > > >>>> this.
> > > >>>>
> > > >>>> Why can't you just change the root function that throws the warn:
> > > >>>>
> > > >>>>    static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> > > >>>> {
> > > >>>>           return vprintk_emit(0, level, NULL, 0, fmt, args);
> > > >>>> }
> > > >>>>
> > > >>>> You aren'r really doing anything extra here, other than calling
> > > >>>> vprintk_emit()
> > > >>>
> > > >>> Yeah, I did that a while ago. I think it was a combination of trying
> > > >>> to avoid an extra layer of adding and then removing the log level, and
> > > >>> that's what dev_printk and friends did.
> > > >>>
> > > >>> But I think you are probably right. It's a lot of maintenance overhead
> > > >>> to get rid of that. Probably best to just use what the printk people
> > > >>> have.
> > > >>>
> > > >>>> Unless I am missing something, can't you solve this problem by
> > including
> > > >>>> printk.h and let it handle the !CONFIG_PRINTK case?
> > > >>>
> > > >>> Randy, I hope you don't mind, but I am going to ask you to re-ack my
> > > >>> next revision since it basically addresses the problem in a totally
> > > >>> different way.
> > > >>
> > > >> Actually, scratch that. Checkpatch doesn't like me calling printk
> > > >> without using a KERN_<LEVEL>.
> > > >>
> > > >> Now that I am thinking back to when I wrote this. I think it also
> > > >> might not like using a dynamic KERN_<LEVEL> either (printk("%s my
> > > >> message", KERN_INFO)).
> > > >>
> > > >> I am going to have to do some more investigation.
> > > >
> > > > Alright, I am pretty sure it is safe to do printk("%smessage",
> > KERN_<LEVEL>);
> > > >
> > > > Looking at the printk implementation, it appears to do the format
> > > > before it checks the log level:
> > > >
> > > >
> > https://elixir.bootlin.com/linux/v5.2.10/source/kernel/printk/printk.c#L1907
> > > >
> > > > So I am pretty sure we can do it either with the vprintk_emit or with
> > printk.
> > >
> > > Let me see if we are on the same page first. I am asking if you can
> > > just include printk.h for vprintk_emit() define for both CONFIG_PRINTK
> > > and !CONFIG_PRINTK cases.
> >
> > Ah sorry, I misunderstood you.
> >
> > No, that doesn't work. I tried including linux/printk.h, and I get the
> > same error.
> >
> > The reason for this is that vprintk_emit() is only defined when
> > CONFIG_PRINTK=y:
> >
> > https://elixir.bootlin.com/linux/latest/ident/vprintk_emit
>
> Ugh.  That's just a bug in include/linux/printk.h
>
> There should be a stub definition for vprintk_emit() in the #else part
> of #ifdef CONFIG_PRINTK.
>
> You shouldn't be dealing with whether printk is present or not
> in the kunit code.  All the printk-related routines are supposed
> to evaporate themselves, based on the conditional in
> include/linux/printk.h
>
> That should be fixed there instead of in your code.

Alright. That makes sense.

I will submit a patch to fix it.

Sorry for not suggesting that, I just assumed that it was my mistake
in how I was using printk.

> Let me know if you'd like me to submit a patch for that.  I only hesitate
> because your patch depends on it, and IMHO it makes more sense to
> include it in your batch than separately. Otherwise there's a patch race
> condition.

Thanks for clearing up the confusion!

Re: [PATCH v1] kunit: fix failure to build without printk

[shuah]

On 8/27/19 4:16 PM, Brendan Higgins wrote:
> On Tue, Aug 27, 2019 at 3:00 PM shuah <shuah@kernel.org> wrote:
>>
>> On 8/27/19 3:36 PM, Brendan Higgins wrote:
>>> On Tue, Aug 27, 2019 at 2:09 PM Brendan Higgins
>>> <brendanhiggins@google.com> wrote:
>>>>
>>>> On Tue, Aug 27, 2019 at 2:03 PM Brendan Higgins
>>>> <brendanhiggins@google.com> wrote:
>>>>>
>>>>> On Tue, Aug 27, 2019 at 1:21 PM shuah <shuah@kernel.org> wrote:
>>>>>>
>>>>>> On 8/27/19 11:49 AM, Brendan Higgins wrote:
>>>>>>> Previously KUnit assumed that printk would always be present, which is
>>>>>>> not a valid assumption to make. Fix that by ifdefing out functions which
>>>>>>> directly depend on printk core functions similar to what dev_printk
>>>>>>> does.
>>>>>>>
>>>>>>> Reported-by: Randy Dunlap <rdunlap@infradead.org>
>>>>>>> Link: https://lore.kernel.org/linux-kselftest/0352fae9-564f-4a97-715a-fabe016259df@kernel.org/T/#t
>>>>>>> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
>>>>>>> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
>>>>>>> ---
>>>>>>>     include/kunit/test.h |  7 +++++++
>>>>>>>     kunit/test.c         | 41 ++++++++++++++++++++++++-----------------
>>>>>>>     2 files changed, 31 insertions(+), 17 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/kunit/test.h b/include/kunit/test.h
>>>>>>> index 8b7eb03d4971..339af5f95c4a 100644
>>>>>>> --- a/include/kunit/test.h
>>>>>>> +++ b/include/kunit/test.h
>>>>>>> @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
>>>>> [...]
>>>>>> Okay after reviewing this, I am not sure why you need to do all
>>>>>> this.
>>>>>>
>>>>>> Why can't you just change the root function that throws the warn:
>>>>>>
>>>>>>     static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
>>>>>> {
>>>>>>            return vprintk_emit(0, level, NULL, 0, fmt, args);
>>>>>> }
>>>>>>
>>>>>> You aren'r really doing anything extra here, other than calling
>>>>>> vprintk_emit()
>>>>>
>>>>> Yeah, I did that a while ago. I think it was a combination of trying
>>>>> to avoid an extra layer of adding and then removing the log level, and
>>>>> that's what dev_printk and friends did.
>>>>>
>>>>> But I think you are probably right. It's a lot of maintenance overhead
>>>>> to get rid of that. Probably best to just use what the printk people
>>>>> have.
>>>>>
>>>>>> Unless I am missing something, can't you solve this problem by including
>>>>>> printk.h and let it handle the !CONFIG_PRINTK case?
>>>>>
>>>>> Randy, I hope you don't mind, but I am going to ask you to re-ack my
>>>>> next revision since it basically addresses the problem in a totally
>>>>> different way.
>>>>
>>>> Actually, scratch that. Checkpatch doesn't like me calling printk
>>>> without using a KERN_<LEVEL>.
>>>>
>>>> Now that I am thinking back to when I wrote this. I think it also
>>>> might not like using a dynamic KERN_<LEVEL> either (printk("%s my
>>>> message", KERN_INFO)).
>>>>
>>>> I am going to have to do some more investigation.
>>>
>>> Alright, I am pretty sure it is safe to do printk("%smessage", KERN_<LEVEL>);
>>>
>>> Looking at the printk implementation, it appears to do the format
>>> before it checks the log level:
>>>
>>> https://elixir.bootlin.com/linux/v5.2.10/source/kernel/printk/printk.c#L1907
>>>
>>> So I am pretty sure we can do it either with the vprintk_emit or with printk.
>>
>> Let me see if we are on the same page first. I am asking if you can
>> just include printk.h for vprintk_emit() define for both CONFIG_PRINTK
>> and !CONFIG_PRINTK cases.
> 
> Ah sorry, I misunderstood you.
> 
> No, that doesn't work. I tried including linux/printk.h, and I get the
> same error.
> 
> The reason for this is that vprintk_emit() is only defined when CONFIG_PRINTK=y:
> 

This is the real problem here. printk.h defines several for
!CONFIG_PRINTK case.

> https://elixir.bootlin.com/linux/latest/ident/vprintk_emit
> 
>> I am not asking you to use printk() in place of vprintk_emit().
>> It is perfectly fine to use vprintk_emit()
> 
> Okay, cool.
> 
>>>
>>> So it appears that we have to weigh the following trade-offs:
>>>
>>> Using vprintk_emit:
>>>
>>> Pros:
>>>    - That's what dev_printk uses.
>>
>> Not sure what you mean by this. I am suggesting if you can just
>> call vprintk_emit() and include printk.h and not have to ifdef
>> around all the other callers of kunit_vprintk_emit()
> 
> Oh, I was just saying that I heavily based my implementation of
> kunit_printk on dev_printk. So I have a high degree of confidence that
> it is okay to use it the way that I am using it.
> 
>> Yes. There is the other issue of why do you need the complexity
>> of having kunit_vprintk_emit() at all.
> 
> Right, and the problem with the alternative, is there is no good
> kernel API for logging with the log level set dynamically. printk
> prefers to have it as a string prefix on the format string, but I
> cannot do that because I need to add my own prefix to the format
> string.
> 
> So, I guess I should just go ahead and address the earlier comments on
> this patch?
> 

So what does your code do in the case of !CONFIG_PRINTK. From my read of
it, it returns 0 from kunit_printk_emit(0 from my read of it. What I am
saying is this is a lot of indirection instead of fixing the leaf
function which is kunit_vprintk_emit().

+#else /* CONFIG_PRINTK */
+static inline int kunit_printk_emit(int level, const char *fmt, ...)
+{
+	return 0;
+}
+#endif /* CONFIG_PRINTK */

Does the following work?

#if defined CONFIG_PRINTK
static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
{
         return vprintk_emit(0, level, NULL, 0, fmt, args);
}
#else
static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
{
        return 0;
}
#endif

I think the real problem is in the printk.h with its missing define for
vprintk_emit() for !CONFIG_PRINTK case. There seem to only one call for
this in drivers/base/core.c in CONFIG_PRINTK path. Unless I am totally
missing some context for why there is no stub for vprintk_emit() for
!CONFIG_PRINTK case

thanks,
-- Shuah

Re: [PATCH v1] kunit: fix failure to build without printk

[Brendan Higgins]

On Tue, Aug 27, 2019 at 3:55 PM shuah <shuah@kernel.org> wrote:
>
> On 8/27/19 4:16 PM, Brendan Higgins wrote:
> > On Tue, Aug 27, 2019 at 3:00 PM shuah <shuah@kernel.org> wrote:
> >>
> >> On 8/27/19 3:36 PM, Brendan Higgins wrote:
> >>> On Tue, Aug 27, 2019 at 2:09 PM Brendan Higgins
> >>> <brendanhiggins@google.com> wrote:
> >>>>
> >>>> On Tue, Aug 27, 2019 at 2:03 PM Brendan Higgins
> >>>> <brendanhiggins@google.com> wrote:
> >>>>>
> >>>>> On Tue, Aug 27, 2019 at 1:21 PM shuah <shuah@kernel.org> wrote:
> >>>>>>
> >>>>>> On 8/27/19 11:49 AM, Brendan Higgins wrote:
> >>>>>>> Previously KUnit assumed that printk would always be present, which is
> >>>>>>> not a valid assumption to make. Fix that by ifdefing out functions which
> >>>>>>> directly depend on printk core functions similar to what dev_printk
> >>>>>>> does.
> >>>>>>>
> >>>>>>> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> >>>>>>> Link: https://lore.kernel.org/linux-kselftest/0352fae9-564f-4a97-715a-fabe016259df@kernel.org/T/#t
> >>>>>>> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> >>>>>>> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> >>>>>>> ---
> >>>>>>>     include/kunit/test.h |  7 +++++++
> >>>>>>>     kunit/test.c         | 41 ++++++++++++++++++++++++-----------------
> >>>>>>>     2 files changed, 31 insertions(+), 17 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/include/kunit/test.h b/include/kunit/test.h
> >>>>>>> index 8b7eb03d4971..339af5f95c4a 100644
> >>>>>>> --- a/include/kunit/test.h
> >>>>>>> +++ b/include/kunit/test.h
> >>>>>>> @@ -339,9 +339,16 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
> >>>>> [...]
> >>>>>> Okay after reviewing this, I am not sure why you need to do all
> >>>>>> this.
> >>>>>>
> >>>>>> Why can't you just change the root function that throws the warn:
> >>>>>>
> >>>>>>     static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> >>>>>> {
> >>>>>>            return vprintk_emit(0, level, NULL, 0, fmt, args);
> >>>>>> }
> >>>>>>
> >>>>>> You aren'r really doing anything extra here, other than calling
> >>>>>> vprintk_emit()
> >>>>>
> >>>>> Yeah, I did that a while ago. I think it was a combination of trying
> >>>>> to avoid an extra layer of adding and then removing the log level, and
> >>>>> that's what dev_printk and friends did.
> >>>>>
> >>>>> But I think you are probably right. It's a lot of maintenance overhead
> >>>>> to get rid of that. Probably best to just use what the printk people
> >>>>> have.
> >>>>>
> >>>>>> Unless I am missing something, can't you solve this problem by including
> >>>>>> printk.h and let it handle the !CONFIG_PRINTK case?
> >>>>>
> >>>>> Randy, I hope you don't mind, but I am going to ask you to re-ack my
> >>>>> next revision since it basically addresses the problem in a totally
> >>>>> different way.
> >>>>
> >>>> Actually, scratch that. Checkpatch doesn't like me calling printk
> >>>> without using a KERN_<LEVEL>.
> >>>>
> >>>> Now that I am thinking back to when I wrote this. I think it also
> >>>> might not like using a dynamic KERN_<LEVEL> either (printk("%s my
> >>>> message", KERN_INFO)).
> >>>>
> >>>> I am going to have to do some more investigation.
> >>>
> >>> Alright, I am pretty sure it is safe to do printk("%smessage", KERN_<LEVEL>);
> >>>
> >>> Looking at the printk implementation, it appears to do the format
> >>> before it checks the log level:
> >>>
> >>> https://elixir.bootlin.com/linux/v5.2.10/source/kernel/printk/printk.c#L1907
> >>>
> >>> So I am pretty sure we can do it either with the vprintk_emit or with printk.
> >>
> >> Let me see if we are on the same page first. I am asking if you can
> >> just include printk.h for vprintk_emit() define for both CONFIG_PRINTK
> >> and !CONFIG_PRINTK cases.
> >
> > Ah sorry, I misunderstood you.
> >
> > No, that doesn't work. I tried including linux/printk.h, and I get the
> > same error.
> >
> > The reason for this is that vprintk_emit() is only defined when CONFIG_PRINTK=y:
> >
>
> This is the real problem here. printk.h defines several for
> !CONFIG_PRINTK case.

Yeah, Tim pointed that out.

I think both of you are right, I should be filing my fix against them.

> > https://elixir.bootlin.com/linux/latest/ident/vprintk_emit
> >
> >> I am not asking you to use printk() in place of vprintk_emit().
> >> It is perfectly fine to use vprintk_emit()
> >
> > Okay, cool.
> >
> >>>
> >>> So it appears that we have to weigh the following trade-offs:
> >>>
> >>> Using vprintk_emit:
> >>>
> >>> Pros:
> >>>    - That's what dev_printk uses.
> >>
> >> Not sure what you mean by this. I am suggesting if you can just
> >> call vprintk_emit() and include printk.h and not have to ifdef
> >> around all the other callers of kunit_vprintk_emit()
> >
> > Oh, I was just saying that I heavily based my implementation of
> > kunit_printk on dev_printk. So I have a high degree of confidence that
> > it is okay to use it the way that I am using it.
> >
> >> Yes. There is the other issue of why do you need the complexity
> >> of having kunit_vprintk_emit() at all.
> >
> > Right, and the problem with the alternative, is there is no good
> > kernel API for logging with the log level set dynamically. printk
> > prefers to have it as a string prefix on the format string, but I
> > cannot do that because I need to add my own prefix to the format
> > string.
> >
> > So, I guess I should just go ahead and address the earlier comments on
> > this patch?
> >
>
> So what does your code do in the case of !CONFIG_PRINTK. From my read of
> it, it returns 0 from kunit_printk_emit(0 from my read of it. What I am
> saying is this is a lot of indirection instead of fixing the leaf
> function which is kunit_vprintk_emit().

Agreed. My apologies, as I mentioned in response to Tim, I just
assumed I was using it wrong.

> +#else /* CONFIG_PRINTK */
> +static inline int kunit_printk_emit(int level, const char *fmt, ...)
> +{
> +       return 0;
> +}
> +#endif /* CONFIG_PRINTK */
>
> Does the following work?
>
> #if defined CONFIG_PRINTK
> static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> {
>          return vprintk_emit(0, level, NULL, 0, fmt, args);
> }
> #else
> static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
> {
>         return 0;
> }
> #endif
>
> I think the real problem is in the printk.h with its missing define for
> vprintk_emit() for !CONFIG_PRINTK case. There seem to only one call for
> this in drivers/base/core.c in CONFIG_PRINTK path. Unless I am totally
> missing some context for why there is no stub for vprintk_emit() for
> !CONFIG_PRINTK case

Agreed.

Sorry again for the confusion.

Thanks!

Re: [PATCH v1] kunit: fix failure to build without printk

[Stephen Boyd]

Quoting Brendan Higgins (2019-08-27 10:49:32)
> Previously KUnit assumed that printk would always be present, which is
> not a valid assumption to make. Fix that by ifdefing out functions which
> directly depend on printk core functions similar to what dev_printk
> does.
> 
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Link: https://lore.kernel.org/linux-kselftest/0352fae9-564f-4a97-715a-fabe016259df@kernel.org/T/#t
> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---

Does kunit itself have any meaning if printk doesn't work? Why not just
depend on CONFIG_PRINTK for now?

Re: [PATCH v1] kunit: fix failure to build without printk

[Brendan Higgins]

On Tue, Aug 27, 2019 at 2:46 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-08-27 10:49:32)
> > Previously KUnit assumed that printk would always be present, which is
> > not a valid assumption to make. Fix that by ifdefing out functions which
> > directly depend on printk core functions similar to what dev_printk
> > does.
> >
> > Reported-by: Randy Dunlap <rdunlap@infradead.org>
> > Link: https://lore.kernel.org/linux-kselftest/0352fae9-564f-4a97-715a-fabe016259df@kernel.org/T/#t
> > Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > ---
>
> Does kunit itself have any meaning if printk doesn't work? Why not just
> depend on CONFIG_PRINTK for now?

I was thinking about that, but I figured it is probably easier in the
long run to make sure it always works without printk.

It also just seemed like the right thing to do, but I suppose that's
not a very good reason.

I am fine with any of the three options: depend on CONFIG_PRINTK - as
suggested by Stephen, just use printk - as suggested by Shuah, or
continue to use vprintk_emit as I have been doing. However, my
preference is the vprintk_emit option.

Anyone have any strong opinions on the matter?