[PATCH 1/1] RDMA/rxe: Make pr_fmt work

[PATCH 1/1] RDMA/rxe: Make pr_fmt work

[Yanjun.Zhu]

From: Zhu Yanjun <yanjun.zhu@linux.dev>

If the definition of pr_fmt is before the header file. The pr_fmt
will be overwritten by the header file. So move the definition of
pr_fmt to the below of the header file.

Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
---
 drivers/infiniband/sw/rxe/rxe.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe.h b/drivers/infiniband/sw/rxe/rxe.h
index d8fb2c7af30a..dc2d8dd2f681 100644
--- a/drivers/infiniband/sw/rxe/rxe.h
+++ b/drivers/infiniband/sw/rxe/rxe.h
@@ -7,11 +7,6 @@
 #ifndef RXE_H
 #define RXE_H
 
-#ifdef pr_fmt
-#undef pr_fmt
-#endif
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/skbuff.h>
 
 #include <rdma/ib_verbs.h>
@@ -30,6 +25,11 @@
 #include "rxe_verbs.h"
 #include "rxe_loc.h"
 
+#ifdef pr_fmt
+#undef pr_fmt
+#endif
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 /*
  * Version 1 and Version 2 are identical on 64 bit machines, but on 32 bit
  * machines Version 2 has a different struct layout.
-- 
2.34.1

Re: [PATCH 1/1] RDMA/rxe: Make pr_fmt work

[Greg Sword]

On Sat, Mar 23, 2024 at 4:32 PM Yanjun.Zhu <yanjun.zhu@linux.dev> wrote:
>
> From: Zhu Yanjun <yanjun.zhu@linux.dev>
>
> If the definition of pr_fmt is before the header file. The pr_fmt
> will be overwritten by the header file. So move the definition of
> pr_fmt to the below of the header file.

Awesome. You fix my problem.

>
> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> ---
>  drivers/infiniband/sw/rxe/rxe.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe.h b/drivers/infiniband/sw/rxe/rxe.h
> index d8fb2c7af30a..dc2d8dd2f681 100644
> --- a/drivers/infiniband/sw/rxe/rxe.h
> +++ b/drivers/infiniband/sw/rxe/rxe.h
> @@ -7,11 +7,6 @@
>  #ifndef RXE_H
>  #define RXE_H
>
> -#ifdef pr_fmt
> -#undef pr_fmt
> -#endif
> -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> -
>  #include <linux/skbuff.h>
>
>  #include <rdma/ib_verbs.h>
> @@ -30,6 +25,11 @@
>  #include "rxe_verbs.h"
>  #include "rxe_loc.h"
>
> +#ifdef pr_fmt
> +#undef pr_fmt
> +#endif
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>  /*
>   * Version 1 and Version 2 are identical on 64 bit machines, but on 32 bit
>   * machines Version 2 has a different struct layout.
> --
> 2.34.1
>
>

Re: [PATCH 1/1] RDMA/rxe: Make pr_fmt work

[Jason Gunthorpe]

On Sat, Mar 23, 2024 at 09:31:39AM +0100, Yanjun.Zhu wrote:
> From: Zhu Yanjun <yanjun.zhu@linux.dev>
> 
> If the definition of pr_fmt is before the header file. The pr_fmt
> will be overwritten by the header file. So move the definition of
> pr_fmt to the below of the header file.

what header file?

Jason

Re: [PATCH 1/1] RDMA/rxe: Make pr_fmt work

[Zhu Yanjun]

在 2024/3/27 14:08, Jason Gunthorpe 写道:
> On Sat, Mar 23, 2024 at 09:31:39AM +0100, Yanjun.Zhu wrote:
>> From: Zhu Yanjun <yanjun.zhu@linux.dev>
>>
>> If the definition of pr_fmt is before the header file. The pr_fmt
>> will be overwritten by the header file. So move the definition of
>> pr_fmt to the below of the header file.
> what header file?

include/linux/printk.h

Because this driver will finally call printk function to output the 
logs, the header file include/linux/printk.h needs be included.

In include/linux/printk.h, pr_fmt is defined.

I made a simple checks. In about 130 header files, this pr_fmt is defined.

Zhu Yanjun

>
> Jason

Re: [PATCH 1/1] RDMA/rxe: Make pr_fmt work

[Jason Gunthorpe]

On Wed, Mar 27, 2024 at 08:40:54PM +0100, Zhu Yanjun wrote:
> 
> 在 2024/3/27 14:08, Jason Gunthorpe 写道:
> > On Sat, Mar 23, 2024 at 09:31:39AM +0100, Yanjun.Zhu wrote:
> > > From: Zhu Yanjun <yanjun.zhu@linux.dev>
> > > 
> > > If the definition of pr_fmt is before the header file. The pr_fmt
> > > will be overwritten by the header file. So move the definition of
> > > pr_fmt to the below of the header file.
> > what header file?
> 
> include/linux/printk.h
> 
> Because this driver will finally call printk function to output the logs,
> the header file include/linux/printk.h needs be included.
> 
> In include/linux/printk.h, pr_fmt is defined.

This doesn't make sense, printk.h has:

#ifndef pr_fmt
#define pr_fmt(fmt) fmt
#endif

Before or after printk.h should not have an impact.

Jason

Re: [PATCH 1/1] RDMA/rxe: Make pr_fmt work

[Zhu Yanjun]

在 2024/4/4 20:01, Zhu Yanjun 写道:
>
>
> 在 2024/4/4 16:59, Jason Gunthorpe 写道:
>> On Wed, Mar 27, 2024 at 08:40:54PM +0100, Zhu Yanjun wrote:
>>> 在 2024/3/27 14:08, Jason Gunthorpe 写道:
>>>> On Sat, Mar 23, 2024 at 09:31:39AM +0100, Yanjun.Zhu wrote:
>>>>> From: Zhu Yanjun<yanjun.zhu@linux.dev>
>>>>>
>>>>> If the definition of pr_fmt is before the header file. The pr_fmt
>>>>> will be overwritten by the header file. So move the definition of
>>>>> pr_fmt to the below of the header file.
>>>> what header file?
>>> include/linux/printk.h
>>>
>>> Because this driver will finally call printk function to output the logs,
>>> the header file include/linux/printk.h needs be included.
>>>
>>> In include/linux/printk.h, pr_fmt is defined.
>> This doesn't make sense, printk.h has:
>>
>> #ifndef pr_fmt
>> #define pr_fmt(fmt) fmt
>> #endif
>>
>> Before or after printk.h should not have an impact.


Sorry. The previous mail is not sent successfully. I resend it.

> #ifndef pr_fmt
>
> ...
>
> #endif
>
> The above will not undefine pr_fmt.
>
> #undef pr_fmt will undefine pr_fmt.
>
> This link explains the above in details.
>
https://www.techonthenet.com/c_language/directives/ifndef.php
>
> Thanks a lot for your comments.
>
> Zhu Yanjun
>
>> Jason

Re: [PATCH 1/1] RDMA/rxe: Make pr_fmt work

[Jason Gunthorpe]

On Thu, Apr 04, 2024 at 08:03:35PM +0200, Zhu Yanjun wrote:
> > > > Because this driver will finally call printk function to output the logs,
> > > > the header file include/linux/printk.h needs be included.
> > > > 
> > > > In include/linux/printk.h, pr_fmt is defined.
> > > This doesn't make sense, printk.h has:
> > > 
> > > #ifndef pr_fmt
> > > #define pr_fmt(fmt) fmt
> > > #endif
> > > 
> > > Before or after printk.h should not have an impact.
> 
> 
> Sorry. The previous mail is not sent successfully. I resend it.
> 
> > #ifndef pr_fmt
> > 
> > ...
> > 
> > #endif
> > 
> > The above will not undefine pr_fmt.
> > 
> > #undef pr_fmt will undefine pr_fmt.
> > 
> > This link explains the above in details.
> > 
> https://www.techonthenet.com/c_language/directives/ifndef.php

Why would you want to undefine it? The point is to #define it to the
rxe specific value. If it is already set to the rxe specific value
before including printk.h then it will work fine?

Jason

Re: [PATCH 1/1] RDMA/rxe: Make pr_fmt work

[Zhu Yanjun]

在 2024/4/5 1:59, Jason Gunthorpe 写道:
> On Thu, Apr 04, 2024 at 08:03:35PM +0200, Zhu Yanjun wrote:
>>>>> Because this driver will finally call printk function to output the logs,
>>>>> the header file include/linux/printk.h needs be included.
>>>>>
>>>>> In include/linux/printk.h, pr_fmt is defined.
>>>> This doesn't make sense, printk.h has:
>>>>
>>>> #ifndef pr_fmt
>>>> #define pr_fmt(fmt) fmt
>>>> #endif
>>>>
>>>> Before or after printk.h should not have an impact.
>>
>>
>> Sorry. The previous mail is not sent successfully. I resend it.
>>
>>> #ifndef pr_fmt
>>>
>>> ...
>>>
>>> #endif
>>>
>>> The above will not undefine pr_fmt.
>>>
>>> #undef pr_fmt will undefine pr_fmt.
>>>
>>> This link explains the above in details.
>>>
>> https://www.techonthenet.com/c_language/directives/ifndef.php
> 
> Why would you want to undefine it? The point is to #define it to the
> rxe specific value. If it is already set to the rxe specific value
> before including printk.h then it will work fine?

Got your point. There are about more 130 header files that define pr_fmt 
in the kernel include directory. And these header files are included one 
another. Is there any tool to clarify the including relationship between 
these header files?

Thanks a lot
Zhu Yanjun
> 
> Jason

Re: [PATCH 1/1] RDMA/rxe: Make pr_fmt work

[Jason Gunthorpe]

On Sat, Apr 06, 2024 at 06:35:21PM +0200, Zhu Yanjun wrote:

> Got your point. There are about more 130 header files that define pr_fmt in
> the kernel include directory. And these header files are included one
> another. Is there any tool to clarify the including relationship between
> these header files?

No tool I am aware of, but if headers define it then they should be
local to a subsystem and never included outside it. Such as what rxe
is doing.

Ideally they would be private headers not under include/

Jason

Re: [PATCH 1/1] RDMA/rxe: Make pr_fmt work

[Leon Romanovsky]

On Sat, Mar 23, 2024 at 09:31:39AM +0100, Yanjun.Zhu wrote:
> From: Zhu Yanjun <yanjun.zhu@linux.dev>
> 
> If the definition of pr_fmt is before the header file. The pr_fmt
> will be overwritten by the header file. So move the definition of
> pr_fmt to the below of the header file.
> 
> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> ---
>  drivers/infiniband/sw/rxe/rxe.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Let's just convert RXE to ibdev*() prints instead.

Thanks

Re: [PATCH 1/1] RDMA/rxe: Make pr_fmt work

[Zhu Yanjun]

在 2024/4/2 19:45, Leon Romanovsky 写道:
> On Sat, Mar 23, 2024 at 09:31:39AM +0100, Yanjun.Zhu wrote:
>> From: Zhu Yanjun <yanjun.zhu@linux.dev>
>>
>> If the definition of pr_fmt is before the header file. The pr_fmt
>> will be overwritten by the header file. So move the definition of
>> pr_fmt to the below of the header file.
>>
>> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
>> ---
>>   drivers/infiniband/sw/rxe/rxe.h | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> Let's just convert RXE to ibdev*() prints instead.

OK. I will do. But it takes me some time because it seems a big task.^_^

Zhu Yanjun

> 
> Thanks