[PATCH] staging: greybus: Enclose multi-statement macro in do-while loop

[PATCH] staging: greybus: Enclose multi-statement macro in do-while loop

[Juan José Arboleda]

The macro `gb_loopback_stats_attrs` in the Greybus loopback driver
contains multiple statements, which should be enclosed in a do-while
loop to ensure proper execution and adhere to coding standards.

This patch modifies the `gb_loopback_stats_attrs` macro to enclose
its statements in a do-while loop, addressing the style error flagged
by checkpatch.pl.

Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com>
---
 drivers/staging/greybus/loopback.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
index 4313d3bbc23a..a97de86c67a5 100644
--- a/drivers/staging/greybus/loopback.c
+++ b/drivers/staging/greybus/loopback.c
@@ -163,9 +163,11 @@ static ssize_t name##_avg_show(struct device *dev,		\
 static DEVICE_ATTR_RO(name##_avg)
 
 #define gb_loopback_stats_attrs(field)				\
-	gb_loopback_ro_stats_attr(field, min, u);		\
-	gb_loopback_ro_stats_attr(field, max, u);		\
-	gb_loopback_ro_avg_attr(field)
+	do {							\
+		gb_loopback_ro_stats_attr(field, min, u);	\
+		gb_loopback_ro_stats_attr(field, max, u);	\
+		gb_loopback_ro_avg_attr(field);			\
+	} while (0)
 
 #define gb_loopback_attr(field, type)					\
 static ssize_t field##_show(struct device *dev,				\
-- 
2.45.2

Re: [PATCH] staging: greybus: Enclose multi-statement macro in do-while loop

[Greg KH]

On Wed, Jul 17, 2024 at 01:58:34AM -0500, Juan José Arboleda wrote:
> The macro `gb_loopback_stats_attrs` in the Greybus loopback driver
> contains multiple statements, which should be enclosed in a do-while
> loop to ensure proper execution and adhere to coding standards.
> 
> This patch modifies the `gb_loopback_stats_attrs` macro to enclose
> its statements in a do-while loop, addressing the style error flagged
> by checkpatch.pl.
> 
> Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com>
> ---
>  drivers/staging/greybus/loopback.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> index 4313d3bbc23a..a97de86c67a5 100644
> --- a/drivers/staging/greybus/loopback.c
> +++ b/drivers/staging/greybus/loopback.c
> @@ -163,9 +163,11 @@ static ssize_t name##_avg_show(struct device *dev,		\
>  static DEVICE_ATTR_RO(name##_avg)
>  
>  #define gb_loopback_stats_attrs(field)				\
> -	gb_loopback_ro_stats_attr(field, min, u);		\
> -	gb_loopback_ro_stats_attr(field, max, u);		\
> -	gb_loopback_ro_avg_attr(field)
> +	do {							\
> +		gb_loopback_ro_stats_attr(field, min, u);	\
> +		gb_loopback_ro_stats_attr(field, max, u);	\
> +		gb_loopback_ro_avg_attr(field);			\
> +	} while (0)
>  
>  #define gb_loopback_attr(field, type)					\
>  static ssize_t field##_show(struct device *dev,				\

Are you sure you test-built this change?

thanks,

greg k-h

Re: [PATCH] staging: greybus: Enclose multi-statement macro in do-while loop

[Juan José Arboleda]

On Wed, Jul 17, 2024 at 09:50:52AM +0200, Greg KH wrote:
> On Wed, Jul 17, 2024 at 01:58:34AM -0500, Juan José Arboleda wrote:
> > The macro `gb_loopback_stats_attrs` in the Greybus loopback driver
> > contains multiple statements, which should be enclosed in a do-while
> > loop to ensure proper execution and adhere to coding standards.
> > 
> > This patch modifies the `gb_loopback_stats_attrs` macro to enclose
> > its statements in a do-while loop, addressing the style error flagged
> > by checkpatch.pl.
> > 
> > Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com>
> > ---
> >  drivers/staging/greybus/loopback.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> > index 4313d3bbc23a..a97de86c67a5 100644
> > --- a/drivers/staging/greybus/loopback.c
> > +++ b/drivers/staging/greybus/loopback.c
> > @@ -163,9 +163,11 @@ static ssize_t name##_avg_show(struct device *dev,		\
> >  static DEVICE_ATTR_RO(name##_avg)
> >  
> >  #define gb_loopback_stats_attrs(field)				\
> > -	gb_loopback_ro_stats_attr(field, min, u);		\
> > -	gb_loopback_ro_stats_attr(field, max, u);		\
> > -	gb_loopback_ro_avg_attr(field)
> > +	do {							\
> > +		gb_loopback_ro_stats_attr(field, min, u);	\
> > +		gb_loopback_ro_stats_attr(field, max, u);	\
> > +		gb_loopback_ro_avg_attr(field);			\
> > +	} while (0)
> >  
> >  #define gb_loopback_attr(field, type)					\
> >  static ssize_t field##_show(struct device *dev,				\
> 
> Are you sure you test-built this change?
> 
> thanks,
> 
> greg k-h

Yes! I build and clean and build and clean and build again, everything seems
fine from the compiler side and posttest task as well. Everything seems ok.

Thanks,
- Juan José

Re: [PATCH] staging: greybus: Enclose multi-statement macro in do-while loop

[Greg KH]

On Wed, Jul 17, 2024 at 03:28:33AM -0500, Juan José Arboleda wrote:
> On Wed, Jul 17, 2024 at 09:50:52AM +0200, Greg KH wrote:
> > On Wed, Jul 17, 2024 at 01:58:34AM -0500, Juan José Arboleda wrote:
> > > The macro `gb_loopback_stats_attrs` in the Greybus loopback driver
> > > contains multiple statements, which should be enclosed in a do-while
> > > loop to ensure proper execution and adhere to coding standards.
> > > 
> > > This patch modifies the `gb_loopback_stats_attrs` macro to enclose
> > > its statements in a do-while loop, addressing the style error flagged
> > > by checkpatch.pl.
> > > 
> > > Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com>
> > > ---
> > >  drivers/staging/greybus/loopback.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> > > index 4313d3bbc23a..a97de86c67a5 100644
> > > --- a/drivers/staging/greybus/loopback.c
> > > +++ b/drivers/staging/greybus/loopback.c
> > > @@ -163,9 +163,11 @@ static ssize_t name##_avg_show(struct device *dev,		\
> > >  static DEVICE_ATTR_RO(name##_avg)
> > >  
> > >  #define gb_loopback_stats_attrs(field)				\
> > > -	gb_loopback_ro_stats_attr(field, min, u);		\
> > > -	gb_loopback_ro_stats_attr(field, max, u);		\
> > > -	gb_loopback_ro_avg_attr(field)
> > > +	do {							\
> > > +		gb_loopback_ro_stats_attr(field, min, u);	\
> > > +		gb_loopback_ro_stats_attr(field, max, u);	\
> > > +		gb_loopback_ro_avg_attr(field);			\
> > > +	} while (0)
> > >  
> > >  #define gb_loopback_attr(field, type)					\
> > >  static ssize_t field##_show(struct device *dev,				\
> > 
> > Are you sure you test-built this change?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Yes! I build and clean and build and clean and build again, everything seems
> fine from the compiler side and posttest task as well. Everything seems ok.

You did not actually build this file that you modified :(

Always test-build your changes.

thanks,

greg k-h

[PATCH v2] staging: greybus: break multi statement macro into multiple lines

[Juan José Arboleda]

The macro `gb_loopback_stats_attrs` in the Greybus loopback driver
contains multiple statements, which should be enclosed in a do-while
loop to ensure proper execution and adhere to coding standards.

The latter is not possible as the macro expansion will create invalid C
syntax. Theres is not such thing like inner function definition inside a
global scope do-while in C.

This patch nukes the `gb_loopback_stats_attrs` macro to enclose,
addressing the style error flagged by checkpatch.pl.

Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com>
---
Notes:

This new version uses a bit different approach so the commit message
will be a bit different as well.

P.S: Thanks for being that patience and apologies for that false
positive of building the non-included driver. Now it is tested-built.

 drivers/staging/greybus/loopback.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
index 4313d3bbc23a..88d86b084f28 100644
--- a/drivers/staging/greybus/loopback.c
+++ b/drivers/staging/greybus/loopback.c
@@ -162,11 +162,6 @@ static ssize_t name##_avg_show(struct device *dev,		\
 }									\
 static DEVICE_ATTR_RO(name##_avg)
 
-#define gb_loopback_stats_attrs(field)				\
-	gb_loopback_ro_stats_attr(field, min, u);		\
-	gb_loopback_ro_stats_attr(field, max, u);		\
-	gb_loopback_ro_avg_attr(field)
-
 #define gb_loopback_attr(field, type)					\
 static ssize_t field##_show(struct device *dev,				\
 			    struct device_attribute *attr,		\
@@ -268,15 +263,29 @@ static void gb_loopback_check_attr(struct gb_loopback *gb)
 }
 
 /* Time to send and receive one message */
-gb_loopback_stats_attrs(latency);
+gb_loopback_ro_stats_attr(latency, min, u);
+gb_loopback_ro_stats_attr(latency, max, u);
+gb_loopback_ro_avg_attr(latency);
+
 /* Number of requests sent per second on this cport */
-gb_loopback_stats_attrs(requests_per_second);
+gb_loopback_ro_stats_attr(requests_per_second, min, u);
+gb_loopback_ro_stats_attr(requests_per_second, max, u);
+gb_loopback_ro_avg_attr(requests_per_second);
+
 /* Quantity of data sent and received on this cport */
-gb_loopback_stats_attrs(throughput);
+gb_loopback_ro_stats_attr(throughput, min, u);
+gb_loopback_ro_stats_attr(throughput, max, u);
+gb_loopback_ro_avg_attr(throughput);
+
 /* Latency across the UniPro link from APBridge's perspective */
-gb_loopback_stats_attrs(apbridge_unipro_latency);
+gb_loopback_ro_stats_attr(apbridge_unipro_latency, min, u);
+gb_loopback_ro_stats_attr(apbridge_unipro_latency, max, u);
+gb_loopback_ro_avg_attr(apbridge_unipro_latency);
+
 /* Firmware induced overhead in the GPBridge */
-gb_loopback_stats_attrs(gbphy_firmware_latency);
+gb_loopback_ro_stats_attr(gbphy_firmware_latency, min, u);
+gb_loopback_ro_stats_attr(gbphy_firmware_latency, max, u);
+gb_loopback_ro_avg_attr(gbphy_firmware_latency);
 
 /* Number of errors encountered during loop */
 gb_loopback_ro_attr(error);
-- 
2.45.2

Re: [PATCH v2] staging: greybus: break multi statement macro into multiple lines

[Greg KH]

On Wed, Jul 17, 2024 at 10:32:07PM -0500, Juan José Arboleda wrote:
> The macro `gb_loopback_stats_attrs` in the Greybus loopback driver
> contains multiple statements, which should be enclosed in a do-while
> loop to ensure proper execution and adhere to coding standards.
> 
> The latter is not possible as the macro expansion will create invalid C
> syntax. Theres is not such thing like inner function definition inside a
> global scope do-while in C.
> 
> This patch nukes the `gb_loopback_stats_attrs` macro to enclose,
> addressing the style error flagged by checkpatch.pl.

The style error here is just wrong, the original code is fine.
Remember, checkpatch provides hints, it isn't always correct, and many
times, like this one, it's completely incorrect.

> 
> Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com>
> ---
> Notes:
> 
> This new version uses a bit different approach so the commit message
> will be a bit different as well.
> 
> P.S: Thanks for being that patience and apologies for that false
> positive of building the non-included driver. Now it is tested-built.
> 
>  drivers/staging/greybus/loopback.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> index 4313d3bbc23a..88d86b084f28 100644
> --- a/drivers/staging/greybus/loopback.c
> +++ b/drivers/staging/greybus/loopback.c
> @@ -162,11 +162,6 @@ static ssize_t name##_avg_show(struct device *dev,		\
>  }									\
>  static DEVICE_ATTR_RO(name##_avg)
>  
> -#define gb_loopback_stats_attrs(field)				\
> -	gb_loopback_ro_stats_attr(field, min, u);		\
> -	gb_loopback_ro_stats_attr(field, max, u);		\
> -	gb_loopback_ro_avg_attr(field)
> -
>  #define gb_loopback_attr(field, type)					\
>  static ssize_t field##_show(struct device *dev,				\
>  			    struct device_attribute *attr,		\
> @@ -268,15 +263,29 @@ static void gb_loopback_check_attr(struct gb_loopback *gb)
>  }
>  
>  /* Time to send and receive one message */
> -gb_loopback_stats_attrs(latency);
> +gb_loopback_ro_stats_attr(latency, min, u);
> +gb_loopback_ro_stats_attr(latency, max, u);
> +gb_loopback_ro_avg_attr(latency);

Unrolling the macro like this does fix the warning, but at the expense
of more code size and manual work.  We write code first for us to
understand, and second for the compiler.  The original code is just
fine, no need for a change at all.

sorry,

greg k-h

Re: [PATCH v2] staging: greybus: break multi statement macro into multiple lines

[Dan Carpenter]

On Wed, Jul 17, 2024 at 10:32:07PM -0500, Juan José Arboleda wrote:
> The macro `gb_loopback_stats_attrs` in the Greybus loopback driver
> contains multiple statements, which should be enclosed in a do-while
> loop to ensure proper execution and adhere to coding standards.
> 
> The latter is not possible as the macro expansion will create invalid C
> syntax. Theres is not such thing like inner function definition inside a
> global scope do-while in C.
> 
> This patch nukes the `gb_loopback_stats_attrs` macro to enclose,
> addressing the style error flagged by checkpatch.pl.
> 
> Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com>
> ---

Sorry, it's not really important to follow checkpatch.  Checkpatch is
just a Perl script and sometimes it gets things wrong.  It's a tool not
a king.

regards,
dan carpenter