[PATCH] MIPS: tlbex: undefine pr_define macro when appropriate

[PATCH] MIPS: tlbex: undefine pr_define macro when appropriate

[Enze Li]

The pr_define macro is used only within the output_pgtable_bits_defines
function, and it hasn't been used anywhere else so far.  Therefore, it
should be undefined when appropriate.

Signed-off-by: Enze Li <lienze@kylinos.cn>
---
 arch/mips/mm/tlbex.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
index 80e05ee98d62..510a7e316eb6 100644
--- a/arch/mips/mm/tlbex.c
+++ b/arch/mips/mm/tlbex.c
@@ -255,6 +255,8 @@ static void output_pgtable_bits_defines(void)
 	pr_define("_PAGE_DIRTY_SHIFT %d\n", _PAGE_DIRTY_SHIFT);
 	pr_define("_PFN_SHIFT %d\n", _PFN_SHIFT);
 	pr_debug("\n");
+
+#undef pr_define
 }
 
 static inline void dump_handler(const char *symbol, const void *start, const void *end)

base-commit: 7b50567bdcad8925ca1e075feb7171c12015afd1
-- 
2.39.2

Re: [PATCH] MIPS: tlbex: undefine pr_define macro when appropriate

[Thomas Bogendoerfer]

On Sat, Apr 01, 2023 at 02:10:25PM +0800, Enze Li wrote:
> The pr_define macro is used only within the output_pgtable_bits_defines
> function, and it hasn't been used anywhere else so far.  Therefore, it
> should be undefined when appropriate.
> 
> Signed-off-by: Enze Li <lienze@kylinos.cn>
> ---
>  arch/mips/mm/tlbex.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
> index 80e05ee98d62..510a7e316eb6 100644
> --- a/arch/mips/mm/tlbex.c
> +++ b/arch/mips/mm/tlbex.c
> @@ -255,6 +255,8 @@ static void output_pgtable_bits_defines(void)
>  	pr_define("_PAGE_DIRTY_SHIFT %d\n", _PAGE_DIRTY_SHIFT);
>  	pr_define("_PFN_SHIFT %d\n", _PFN_SHIFT);
>  	pr_debug("\n");
> +
> +#undef pr_define

I'm probably missing something... what problem are you fixing here ?

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] MIPS: tlbex: undefine pr_define macro when appropriate

[Enze Li]

On Sat, 2023-04-01 at 09:17 +0200, Thomas Bogendoerfer wrote:
> On Sat, Apr 01, 2023 at 02:10:25PM +0800, Enze Li wrote:
> > The pr_define macro is used only within the
> > output_pgtable_bits_defines
> > function, and it hasn't been used anywhere else so far.  Therefore,
> > it
> > should be undefined when appropriate.
> > 
> > Signed-off-by: Enze Li <lienze@kylinos.cn>
> > ---
> >  arch/mips/mm/tlbex.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
> > index 80e05ee98d62..510a7e316eb6 100644
> > --- a/arch/mips/mm/tlbex.c
> > +++ b/arch/mips/mm/tlbex.c
> > @@ -255,6 +255,8 @@ static void output_pgtable_bits_defines(void)
> >         pr_define("_PAGE_DIRTY_SHIFT %d\n", _PAGE_DIRTY_SHIFT);
> >         pr_define("_PFN_SHIFT %d\n", _PFN_SHIFT);
> >         pr_debug("\n");
> > +
> > +#undef pr_define
> 
> I'm probably missing something... what problem are you fixing here ?
> 
> Thomas.
> 

Hi Thomas,

Thanks for your review.  🙂

I'm trying to make the code more readable.  When I first looked at this
function, I was wondering why the pr_define macro was defined here.  It
looks more like a temporary definition, and I was also curious if it
might be used elsewhere.  After searching, I couldn't find any other
instances of it being used, which left me feeling confused.

To enhance readability, it would be good to undefine it at the end of
the function, indicating that it only works within this function, just
like what this patch did.  Alternatively, we can move this macro
definition to the beginning of the file, so that it can be used
throughout the module as shown below.

What do you think of these modifications?  Will they make it more
convenient for others who read this code?

Best Regards,
Enze

--- a/arch/mips/mm/tlbex.c
+++ b/arch/mips/mm/tlbex.c
@@ -37,6 +37,9 @@
 #include <asm/setup.h>
 #include <asm/tlbex.h>
 
+#define pr_define(fmt, ...)                                    \
+       pr_debug("#define " fmt, ##__VA_ARGS__)
+
 static int mips_xpa_disabled;
 
 static int __init xpa_disable(char *s)
@@ -231,9 +234,6 @@ static void uasm_bgezl_label(struct uasm_label **l,
u32 **p, int instance)
  */
 static void output_pgtable_bits_defines(void)
 {
-#define pr_define(fmt, ...)                                    \
-       pr_debug("#define " fmt, ##__VA_ARGS__)
-
        pr_debug("#include <asm/asm.h>\n");
        pr_debug("#include <asm/regdef.h>\n");
        pr_debug("\n");

Re: [PATCH] MIPS: tlbex: undefine pr_define macro when appropriate

[Thomas Bogendoerfer]

On Sat, Apr 01, 2023 at 05:14:28PM +0800, Enze Li wrote:
> On Sat, 2023-04-01 at 09:17 +0200, Thomas Bogendoerfer wrote:
> > On Sat, Apr 01, 2023 at 02:10:25PM +0800, Enze Li wrote:
> > > The pr_define macro is used only within the
> > > output_pgtable_bits_defines
> > > function, and it hasn't been used anywhere else so far.  Therefore,
> > > it
> > > should be undefined when appropriate.
> > > 
> > > Signed-off-by: Enze Li <lienze@kylinos.cn>
> > > ---
> > >  arch/mips/mm/tlbex.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
> > > index 80e05ee98d62..510a7e316eb6 100644
> > > --- a/arch/mips/mm/tlbex.c
> > > +++ b/arch/mips/mm/tlbex.c
> > > @@ -255,6 +255,8 @@ static void output_pgtable_bits_defines(void)
> > >         pr_define("_PAGE_DIRTY_SHIFT %d\n", _PAGE_DIRTY_SHIFT);
> > >         pr_define("_PFN_SHIFT %d\n", _PFN_SHIFT);
> > >         pr_debug("\n");
> > > +
> > > +#undef pr_define
> > 
> > I'm probably missing something... what problem are you fixing here ?
> > 
> > Thomas.
> > 
> 
> Hi Thomas,
> 
> Thanks for your review.  🙂
> 
> I'm trying to make the code more readable.  When I first looked at this
> function, I was wondering why the pr_define macro was defined here.  It
> looks more like a temporary definition, and I was also curious if it
> might be used elsewhere.  After searching, I couldn't find any other
> instances of it being used, which left me feeling confused.
> 
> To enhance readability, it would be good to undefine it at the end of

why is it more readable ?

> What do you think of these modifications?  Will they make it more
> convenient for others who read this code?

for me the missing undef doesn't hurt as there is no rule that local
#defines need to be undefined. And less lines distracting me is a plus

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]