[PATCH] staging: greybus: put macro in a do - while loop

[PATCH] staging: greybus: put macro in a do - while loop

[Dileep Sankhla]

Enclose the macro gb_loopback_stats_attrs defined with multiple
replacement statements in a do - while loop. This avoids possible
if/else logic defects and clears a checkpatch error.

ERROR: Macros with multiple statements should be enclosed in a do -
while loop

Signed-off-by: Dileep Sankhla <dileepsankhla.ds@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 bb33379b5297..eb5a7a20f5a3 100644
--- a/drivers/staging/greybus/loopback.c
+++ b/drivers/staging/greybus/loopback.c
@@ -162,9 +162,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.40.1

Re: [PATCH] staging: greybus: put macro in a do - while loop

[Greg KH]

On Sun, Feb 25, 2024 at 02:10:17PM +0530, Dileep Sankhla wrote:
> Enclose the macro gb_loopback_stats_attrs defined with multiple
> replacement statements in a do - while loop. This avoids possible
> if/else logic defects and clears a checkpatch error.
> 
> ERROR: Macros with multiple statements should be enclosed in a do -
> while loop
> 
> Signed-off-by: Dileep Sankhla <dileepsankhla.ds@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 bb33379b5297..eb5a7a20f5a3 100644
> --- a/drivers/staging/greybus/loopback.c
> +++ b/drivers/staging/greybus/loopback.c
> @@ -162,9 +162,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)

Did you test build this?

>  #define gb_loopback_attr(field, type)					\
>  static ssize_t field##_show(struct device *dev,				\

Why did you only change one if you thought this was a valid change?

thanks,

greg k-h

Re: [PATCH] staging: greybus: put macro in a do - while loop

[Dileep Sankhla]

On Sun, Feb 25, 2024 at 2:26 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> Did you test build this?

Hello Greg,

Yes. No new warning/error was encountered on building the kernel.

> >  #define gb_loopback_attr(field, type)                                        \
> >  static ssize_t field##_show(struct device *dev,                              \
>
> Why did you only change one if you thought this was a valid change?

1. As per my C background, I think no other macros in the above source
code file need to be enclosed in a do - while loop.
2. I am writing the patch because of the Eudyptula Challenge, and I
have to fix "one coding style problem" in any of the files in
drivers/staging/. The above one was one of them.

Regards,
Dileep

Re: [PATCH] staging: greybus: put macro in a do - while loop

[Alex Elder]

On 2/25/24 3:49 AM, Dileep Sankhla wrote:
> On Sun, Feb 25, 2024 at 2:26 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>> Did you test build this?
> 
> Hello Greg,
> 
> Yes. No new warning/error was encountered on building the kernel.

Then your build must not have been compiling your changed
code, because the result of your change produces code that
will not compile successfully.

If you look at where gb_loopback_stats_attrs() is called, it's
used only at outer scope, in "drivers/staging/greybus/loopback.c".

Adding do { ... } while() at outer scope is nonsensical.

> 
>>>   #define gb_loopback_attr(field, type)                                        \
>>>   static ssize_t field##_show(struct device *dev,                              \
>>
>> Why did you only change one if you thought this was a valid change?
> 
> 1. As per my C background, I think no other macros in the above source
> code file need to be enclosed in a do - while loop.

gb_loopback_stats_attrs() must *not* be enclosed in a do..while loop.

> 2. I am writing the patch because of the Eudyptula Challenge, and I
> have to fix "one coding style problem" in any of the files in
> drivers/staging/. The above one was one of them.

I support the challenge.  But you need to be sure your fix actually
works, and in particular (in this case) that it compiles correctly.

					-Alex

> 
> Regards,
> Dileep