Warn on macros with flow control statements

Warn on macros with flow control statements

[Sumitra Sharma]

Hi

I am working on the below chechpatch.pl check

WARNING: Macros with flow control statements should be avoided
#42: FILE: ./drivers/staging/qlge/qlge_devlink.c:42:
+#define FILL_SEG(seg_hdr, seg_regs)                                        \
+       do {                                                                \
+               err = qlge_fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \
+               if (err) {                                                  \
+                       kvfree(dump);                                       \
+                       return err;                                         \
+               }                                                           \
+       } while (0)


I was trying to change this macro into an inline function like this

static inline int FILL_SEG(struct devlink_fmsg *fmsg, struct mpi_coredump_segment_header *seg_hdr, u32 *seg_regs)
{
        int err =0;

        err = qlge_fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs);
        if (err) {
                kvfree(dump);
                return err;
        }
}

But I am not sure whether I am going in the right direction..or how to
move further in it. I am looking for some guidance.



Regards,

Sumitra

Re: Warn on macros with flow control statements

[Julia Lawall]

On Thu, 16 Mar 2023, Sumitra Sharma wrote:

>
> Hi
>
> I am working on the below chechpatch.pl check
>
> WARNING: Macros with flow control statements should be avoided
> #42: FILE: ./drivers/staging/qlge/qlge_devlink.c:42:
> +#define FILL_SEG(seg_hdr, seg_regs)                                        \
> +       do {                                                                \
> +               err = qlge_fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \
> +               if (err) {                                                  \
> +                       kvfree(dump);                                       \
> +                       return err;                                         \
> +               }                                                           \
> +       } while (0)
>
>
> I was trying to change this macro into an inline function like this
>
> static inline int FILL_SEG(struct devlink_fmsg *fmsg, struct mpi_coredump_segment_header *seg_hdr, u32 *seg_regs)
> {
>         int err =0;
>
>         err = qlge_fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs);
>         if (err) {
>                 kvfree(dump);
>                 return err;
>         }
> }

This is not a promising approach.  The return that you have here will just
return from the function that you have defined, while the return in the
macro was intended to return from the function that calls the macro.

The problem with the macro is that when you look at the uses, you don't
know that a return is possible.  But given that the macro is defined right
next to the function that uses it, and that somehow extracting the return
from the macro would be hackish and complex, the code is probably ok as it
is.

The macro is pretty unpleasant, also because it captures several variables
(fmsg and dump).

Maybe a solution would be to replace all the macro calls with:

err = err || qlge_fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs);

and then test for a non zero err value at the end.  But maybe wait for
someone else to weigh in before taking that approach..

julia

>
> But I am not sure whether I am going in the right direction..or how to
> move further in it. I am looking for some guidance.
>
>
>
> Regards,
>
> Sumitra
>
>

Re: Warn on macros with flow control statements

[Sumitra Sharma]

On Thu, Mar 16, 2023 at 09:26:31AM +0100, Julia Lawall wrote:
> 
> 
> On Thu, 16 Mar 2023, Sumitra Sharma wrote:
> 
> >
> > Hi
> >
> > I am working on the below chechpatch.pl check
> >
> > WARNING: Macros with flow control statements should be avoided
> > #42: FILE: ./drivers/staging/qlge/qlge_devlink.c:42:
> > +#define FILL_SEG(seg_hdr, seg_regs)                                        \
> > +       do {                                                                \
> > +               err = qlge_fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \
> > +               if (err) {                                                  \
> > +                       kvfree(dump);                                       \
> > +                       return err;                                         \
> > +               }                                                           \
> > +       } while (0)
> >
> >
> > I was trying to change this macro into an inline function like this
> >
> > static inline int FILL_SEG(struct devlink_fmsg *fmsg, struct mpi_coredump_segment_header *seg_hdr, u32 *seg_regs)
> > {
> >         int err =0;
> >
> >         err = qlge_fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs);
> >         if (err) {
> >                 kvfree(dump);
> >                 return err;
> >         }
> > }
> 
> This is not a promising approach.  The return that you have here will just
> return from the function that you have defined, while the return in the
> macro was intended to return from the function that calls the macro.
> 
> The problem with the macro is that when you look at the uses, you don't
> know that a return is possible.  But given that the macro is defined right
> next to the function that uses it, and that somehow extracting the return
> from the macro would be hackish and complex, the code is probably ok as it
> is.
> 
> The macro is pretty unpleasant, also because it captures several variables
> (fmsg and dump).
> 
> Maybe a solution would be to replace all the macro calls with:
> 
> err = err || qlge_fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs);
> 
> and then test for a non zero err value at the end.  But maybe wait for
> someone else to weigh in before taking that approach..
>

Hi julia,

I will wait for some more reviews. And after that, I will try
implementing your solution.

Regards,

Sumitra

> julia
> 
> >
> > But I am not sure whether I am going in the right direction..or how to
> > move further in it. I am looking for some guidance.
> >
> >
> >
> > Regards,
> >
> > Sumitra
> >
> >

Re: Warn on macros with flow control statements

[Ira Weiny]

Sumitra Sharma wrote:
> 
> Hi
> 
> I am working on the below chechpatch.pl check
> 
> WARNING: Macros with flow control statements should be avoided
> #42: FILE: ./drivers/staging/qlge/qlge_devlink.c:42:
> +#define FILL_SEG(seg_hdr, seg_regs)                                        \
> +       do {                                                                \
> +               err = qlge_fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \

This macro is doing a couple of things wrong.  As Julia said using fmsg here is
ugly too.

But the seg_hdr and seg_regs are really being substituted for members of dump.
I think I'd get rid of it as per below.

> +               if (err) {                                                  \
> +                       kvfree(dump);                                       \
> +                       return err;                                         \
> +               }                                                           \
> +       } while (0)
> 
> 
> I was trying to change this macro into an inline function like this
> 
> static inline int FILL_SEG(struct devlink_fmsg *fmsg, struct mpi_coredump_segment_header *seg_hdr, u32 *seg_regs)
> {
>         int err =0;
> 
>         err = qlge_fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs);
>         if (err) {
>                 kvfree(dump);
>                 return err;
>         }
> }
> 
> But I am not sure whether I am going in the right direction..or how to
> move further in it. I am looking for some guidance.
> 

Wow interesting code formatting.

And in the middle of all those FILL_SEG() calls it calls qlge_fill_seg_() directly...  :-/

...
        FILL_SEG(xfi_hss_pll_hdr, serdes_xfi_hss_pll);

        err = qlge_fill_seg_(fmsg, &dump->misc_nic_seg_hdr,
                             (u32 *)&dump->misc_nic_info);
        if (err) {
                kvfree(dump);
                return err;
        }

        FILL_SEG(intr_states_seg_hdr, intr_states);
...

Based on the flow control I would do something like this.  It is a bit more
verbose but is very clear what is happening.

Ira

22:31:10 > git di
diff --git a/drivers/staging/qlge/qlge_devlink.c b/drivers/staging/qlge/qlge_devlink.c
index 0ab02d6d3817..043ed989f9a0 100644
--- a/drivers/staging/qlge/qlge_devlink.c
+++ b/drivers/staging/qlge/qlge_devlink.c
@@ -39,15 +39,6 @@ static int qlge_fill_seg_(struct devlink_fmsg *fmsg,
        return err;
 }
 
-#define FILL_SEG(seg_hdr, seg_regs)                                        \
-       do {                                                                \
-               err = qlge_fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \
-               if (err) {                                                  \
-                       kvfree(dump);                                       \
-                       return err;                                         \
-               }                                                           \
-       } while (0)
-
 static int qlge_reporter_coredump(struct devlink_health_reporter *reporter,
                                  struct devlink_fmsg *fmsg, void *priv_ctx,
                                  struct netlink_ext_ack *extack)
@@ -85,7 +76,12 @@ static int qlge_reporter_coredump(struct devlink_health_reporter *reporter,
 
        qlge_soft_reset_mpi_risc(qdev);
 
-       FILL_SEG(core_regs_seg_hdr, mpi_core_regs);
+       err = qlge_fill_seg_(fmsg, &dump->core_regs_seg_hdr,
+                            dump->mpi_core_regs);
+       if (err)
+               goto out;
+
+...  and all these others too...
        FILL_SEG(test_logic_regs_seg_hdr, test_logic_regs);
        FILL_SEG(rmii_regs_seg_hdr, rmii_regs);
        FILL_SEG(fcmac1_regs_seg_hdr, fcmac1_regs);
@@ -117,10 +113,8 @@ static int qlge_reporter_coredump(struct devlink_health_reporter *reporter,
 
        err = qlge_fill_seg_(fmsg, &dump->misc_nic_seg_hdr,
                             (u32 *)&dump->misc_nic_info);
-       if (err) {
-               kvfree(dump);
-               return err;
-       }
+       if (err)
+               goto out;
 
        FILL_SEG(intr_states_seg_hdr, intr_states);
        FILL_SEG(cam_entries_seg_hdr, cam_entries);
@@ -139,6 +133,7 @@ static int qlge_reporter_coredump(struct devlink_health_reporter *reporter,
        FILL_SEG(xfi2_hss_pll_hdr, serdes2_xfi_hss_pll);
        FILL_SEG(sem_regs_seg_hdr, sem_regs);
 
+out:
        kvfree(dump);
        return err;
 }

Re: Warn on macros with flow control statements

[Julia Lawall]

> Based on the flow control I would do something like this.  It is a bit more
> verbose but is very clear what is happening.
>

Ira, what do you think of my suggestion of

err = err || qlge_fill_seg_(...);

It would avoid 40 lines of ifs, and would still avoid calling
qlge_fill_seg_ as soon as there is a failure.  On ther other hand, I don't
recall this being done elsewhere, so maybe all the ifs would be
preferable.

julia


> Ira
>
> 22:31:10 > git di
> diff --git a/drivers/staging/qlge/qlge_devlink.c b/drivers/staging/qlge/qlge_devlink.c
> index 0ab02d6d3817..043ed989f9a0 100644
> --- a/drivers/staging/qlge/qlge_devlink.c
> +++ b/drivers/staging/qlge/qlge_devlink.c
> @@ -39,15 +39,6 @@ static int qlge_fill_seg_(struct devlink_fmsg *fmsg,
>         return err;
>  }
>
> -#define FILL_SEG(seg_hdr, seg_regs)                                        \
> -       do {                                                                \
> -               err = qlge_fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \
> -               if (err) {                                                  \
> -                       kvfree(dump);                                       \
> -                       return err;                                         \
> -               }                                                           \
> -       } while (0)
> -
>  static int qlge_reporter_coredump(struct devlink_health_reporter *reporter,
>                                   struct devlink_fmsg *fmsg, void *priv_ctx,
>                                   struct netlink_ext_ack *extack)
> @@ -85,7 +76,12 @@ static int qlge_reporter_coredump(struct devlink_health_reporter *reporter,
>
>         qlge_soft_reset_mpi_risc(qdev);
>
> -       FILL_SEG(core_regs_seg_hdr, mpi_core_regs);
> +       err = qlge_fill_seg_(fmsg, &dump->core_regs_seg_hdr,
> +                            dump->mpi_core_regs);
> +       if (err)
> +               goto out;
> +
> +...  and all these others too...
>         FILL_SEG(test_logic_regs_seg_hdr, test_logic_regs);
>         FILL_SEG(rmii_regs_seg_hdr, rmii_regs);
>         FILL_SEG(fcmac1_regs_seg_hdr, fcmac1_regs);
> @@ -117,10 +113,8 @@ static int qlge_reporter_coredump(struct devlink_health_reporter *reporter,
>
>         err = qlge_fill_seg_(fmsg, &dump->misc_nic_seg_hdr,
>                              (u32 *)&dump->misc_nic_info);
> -       if (err) {
> -               kvfree(dump);
> -               return err;
> -       }
> +       if (err)
> +               goto out;
>
>         FILL_SEG(intr_states_seg_hdr, intr_states);
>         FILL_SEG(cam_entries_seg_hdr, cam_entries);
> @@ -139,6 +133,7 @@ static int qlge_reporter_coredump(struct devlink_health_reporter *reporter,
>         FILL_SEG(xfi2_hss_pll_hdr, serdes2_xfi_hss_pll);
>         FILL_SEG(sem_regs_seg_hdr, sem_regs);
>
> +out:
>         kvfree(dump);
>         return err;
>  }
>
>

Re: Warn on macros with flow control statements

[Ira Weiny]

Julia Lawall wrote:
> > Based on the flow control I would do something like this.  It is a bit more
> > verbose but is very clear what is happening.
> >
> 
> Ira, what do you think of my suggestion of
> 
> err = err || qlge_fill_seg_(...);
> 
> It would avoid 40 lines of ifs, and would still avoid calling
> qlge_fill_seg_ as soon as there is a failure.  On ther other hand, I don't
> recall this being done elsewhere, so maybe all the ifs would be
> preferable.

Oh sorry.  I miss read your suggestion and I thought it was going to
continue running qlge_fill_seg_() for all the other calls.  I read this
as:

err = err | qlge_fill_seg_(...);

Not sure why.  Just late I guess.

Yes your suggestion would work.  I do think it is a bit odd though.

Ira

Re: Warn on macros with flow control statements

[Sumitra Sharma]

On Fri, Mar 17, 2023 at 11:19:12PM -0700, Ira Weiny wrote:
> Julia Lawall wrote:
> > > Based on the flow control I would do something like this.  It is a bit more
> > > verbose but is very clear what is happening.
> > >
> > 
> > Ira, what do you think of my suggestion of
> > 
> > err = err || qlge_fill_seg_(...);
> > 
> > It would avoid 40 lines of ifs, and would still avoid calling
> > qlge_fill_seg_ as soon as there is a failure.  On ther other hand, I don't
> > recall this being done elsewhere, so maybe all the ifs would be
> > preferable.
> 
> Oh sorry.  I miss read your suggestion and I thought it was going to
> continue running qlge_fill_seg_() for all the other calls.  I read this
> as:
> 
> err = err | qlge_fill_seg_(...);
> 
> Not sure why.  Just late I guess.
> 
> Yes your suggestion would work.  I do think it is a bit odd though.
>

Hi Ira,

I would like to know why do you think this is odd? 

PS: I was trying to implement your suggestion and was able to connect to
what julia wrote here. 

For reference this is the link to your suggestion https://lore.kernel.org/outreachy/64154d438f0c8_28ae5229421@iweiny-mobl.notmuch/

Regards,
Sumitra

> Ira

Re: Warn on macros with flow control statements

[Julia Lawall]

On Wed, 22 Mar 2023, Sumitra Sharma wrote:

> On Fri, Mar 17, 2023 at 11:19:12PM -0700, Ira Weiny wrote:
> > Julia Lawall wrote:
> > > > Based on the flow control I would do something like this.  It is a bit more
> > > > verbose but is very clear what is happening.
> > > >
> > >
> > > Ira, what do you think of my suggestion of
> > >
> > > err = err || qlge_fill_seg_(...);
> > >
> > > It would avoid 40 lines of ifs, and would still avoid calling
> > > qlge_fill_seg_ as soon as there is a failure.  On ther other hand, I don't
> > > recall this being done elsewhere, so maybe all the ifs would be
> > > preferable.
> >
> > Oh sorry.  I miss read your suggestion and I thought it was going to
> > continue running qlge_fill_seg_() for all the other calls.  I read this
> > as:
> >
> > err = err | qlge_fill_seg_(...);
> >
> > Not sure why.  Just late I guess.
> >
> > Yes your suggestion would work.  I do think it is a bit odd though.
> >
>
> Hi Ira,
>
> I would like to know why do you think this is odd?

err = err || ... is not a very common pattern.  I looked through te full
Linux kernel for them, and found a few.

You can see one in the function rtl8723_dequeue_writeport in
staging/rtl8723bs/hal/rtl8723bs_xmit.c.  This one is completely useless,
so it's another cleanup opportunity :)

Some others are in loops, like (scripts/dtc/checks.c):

        for (i = 0; i < c->num_prereqs; i++) {
                struct check *prq = c->prereq[i];
                error = error || run_check(prq, dti);
                if (prq->status != PASSED) {
                        c->status = PREREQ;
                        check_msg(c, dti, NULL, NULL, "Failed prerequisite '%s'",
                                  c->prereq[i]->name);
                }
        }

The complete set of files containing this pattern is:

drivers/staging/rtl8723bs/hal/rtl8723bs_xmit.c
drivers/net/ethernet/intel/i40e/i40e_txrx.c
drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
scripts/dtc/checks.c
kernel/rcu/tree.c
arch/mips/pci/pci-legacy.c
drivers/s390/block/dasd_devmap.c
drivers/net/ethernet/intel/iavf/iavf_txrx.c

So not many.  But in your case the err = err || ... would be a lot omre
concise than the version that has ifs everywhere and it owuld be a little
more understandable than the version that hides the early returns in the
macro definition.

julia


>
> PS: I was trying to implement your suggestion and was able to connect to
> what julia wrote here.
>
> For reference this is the link to your suggestion https://lore.kernel.org/outreachy/64154d438f0c8_28ae5229421@iweiny-mobl.notmuch/
>
> Regards,
> Sumitra
>
> > Ira
>

Re: Warn on macros with flow control statements

[Ira Weiny]

Sumitra Sharma wrote:
> On Fri, Mar 17, 2023 at 11:19:12PM -0700, Ira Weiny wrote:
> > Julia Lawall wrote:
> > > > Based on the flow control I would do something like this.  It is a bit more
> > > > verbose but is very clear what is happening.
> > > >
> > > 
> > > Ira, what do you think of my suggestion of
> > > 
> > > err = err || qlge_fill_seg_(...);
> > > 
> > > It would avoid 40 lines of ifs, and would still avoid calling
> > > qlge_fill_seg_ as soon as there is a failure.  On ther other hand, I don't
> > > recall this being done elsewhere, so maybe all the ifs would be
> > > preferable.
> > 
> > Oh sorry.  I miss read your suggestion and I thought it was going to
> > continue running qlge_fill_seg_() for all the other calls.  I read this
> > as:
> > 
> > err = err | qlge_fill_seg_(...);
> > 
> > Not sure why.  Just late I guess.
> > 
> > Yes your suggestion would work.  I do think it is a bit odd though.
> >
> 
> Hi Ira,
> 
> I would like to know why do you think this is odd? 

First, it is _not_ wrong.  Just was odd in my head.

> 
> PS: I was trying to implement your suggestion and was able to connect to
> what julia wrote here. 
> 
> For reference this is the link to your suggestion https://lore.kernel.org/outreachy/64154d438f0c8_28ae5229421@iweiny-mobl.notmuch/

I think what Julia proposes is better given this is a pattern is used
elsewhere.  I've just not run across it myself.

I find it odd because the parser in my head is not used to it.  :-D  But
sometimes one has to _read_ the code carefully.  ;-)

Again, I think Julia's suggestion is best given that mine will result in a
lot more code in the end.  And when reading the function you really only
have to grok the first call.  Then the pattern holds through the function.

Ira

Re: Warn on macros with flow control statements

[Ira Weiny]

Julia Lawall wrote:
> 
> 
> On Wed, 22 Mar 2023, Sumitra Sharma wrote:
> 
> > On Fri, Mar 17, 2023 at 11:19:12PM -0700, Ira Weiny wrote:
> > > Julia Lawall wrote:
> > > > > Based on the flow control I would do something like this.  It is a bit more
> > > > > verbose but is very clear what is happening.
> > > > >
> > > >
> > > > Ira, what do you think of my suggestion of
> > > >
> > > > err = err || qlge_fill_seg_(...);
> > > >
> > > > It would avoid 40 lines of ifs, and would still avoid calling
> > > > qlge_fill_seg_ as soon as there is a failure.  On ther other hand, I don't
> > > > recall this being done elsewhere, so maybe all the ifs would be
> > > > preferable.
> > >
> > > Oh sorry.  I miss read your suggestion and I thought it was going to
> > > continue running qlge_fill_seg_() for all the other calls.  I read this
> > > as:
> > >
> > > err = err | qlge_fill_seg_(...);
> > >
> > > Not sure why.  Just late I guess.
> > >
> > > Yes your suggestion would work.  I do think it is a bit odd though.
> > >
> >
> > Hi Ira,
> >
> > I would like to know why do you think this is odd?
> 
> err = err || ... is not a very common pattern.  I looked through te full
> Linux kernel for them, and found a few.

I concede!  :-D

This is just not a pattern I'm used to so it looked odd to me.

> 
> You can see one in the function rtl8723_dequeue_writeport in
> staging/rtl8723bs/hal/rtl8723bs_xmit.c.  This one is completely useless,
> so it's another cleanup opportunity :)
>

Indeed.

> 
> Some others are in loops, like (scripts/dtc/checks.c):
> 
>         for (i = 0; i < c->num_prereqs; i++) {
>                 struct check *prq = c->prereq[i];
>                 error = error || run_check(prq, dti);
>                 if (prq->status != PASSED) {
>                         c->status = PREREQ;
>                         check_msg(c, dti, NULL, NULL, "Failed prerequisite '%s'",
>                                   c->prereq[i]->name);
>                 }
>         }
> 
> The complete set of files containing this pattern is:
> 
> drivers/staging/rtl8723bs/hal/rtl8723bs_xmit.c
> drivers/net/ethernet/intel/i40e/i40e_txrx.c
> drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> scripts/dtc/checks.c
> kernel/rcu/tree.c
> arch/mips/pci/pci-legacy.c
> drivers/s390/block/dasd_devmap.c
> drivers/net/ethernet/intel/iavf/iavf_txrx.c
> 
> So not many.  But in your case the err = err || ... would be a lot omre
> concise than the version that has ifs everywhere and it owuld be a little
> more understandable than the version that hides the early returns in the
> macro definition.

Yes much better I think.  Sorry for the delay in getting back to you all.
Ira