diff mbox series

staging: greybus: loopback: enclose macro statements in do-while loop

Message ID Y0wS4HQo9m/W/TrQ@debian-BULLSEYE-live-builder-AMD64
State New
Headers
Series staging: greybus: loopback: enclose macro statements in do-while loop |

Commit Message

Deepak R Varma Oct. 16, 2022, 2:19 p.m. UTC 
  Include multiple statements of macro definition inside do-while{0} loop
to avoid possible partial program execution. Issue reported by
checkpatch script:

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

Signed-off-by: Deepak R Varma <drv@mailo.com>
---
 drivers/staging/greybus/loopback.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--
2.30.2

Comments

Julia Lawall Oct. 16, 2022, 2:51 p.m. UTC | #1 
On Sun, 16 Oct 2022, Deepak R Varma wrote:

> Include multiple statements of macro definition inside do-while{0} loop
> to avoid possible partial program execution. Issue reported by
> checkpatch script:
>
> ERROR: Macros with multiple statements should be enclosed in a do - while loop

I don't think this change will compile.  See if you can figure out why
not.

julia

>
> Signed-off-by: Deepak R Varma <drv@mailo.com>
> ---
>  drivers/staging/greybus/loopback.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> index 1a61fce98056..37214cb43937 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)				\
> +do {								\
>  	gb_loopback_ro_stats_attr(field, min, u);		\
>  	gb_loopback_ro_stats_attr(field, max, u);		\
> -	gb_loopback_ro_avg_attr(field)
> +	gb_loopback_ro_avg_attr(field);				\
> +} while (0)
>
>  #define gb_loopback_attr(field, type)					\
>  static ssize_t field##_show(struct device *dev,				\
> --
> 2.30.2
>
>
>
>
>
Deepak R Varma Oct. 16, 2022, 3:02 p.m. UTC | #2 
On Sun, Oct 16, 2022 at 04:51:09PM +0200, Julia Lawall wrote:
>
>
> On Sun, 16 Oct 2022, Deepak R Varma wrote:
>
> > Include multiple statements of macro definition inside do-while{0} loop
> > to avoid possible partial program execution. Issue reported by
> > checkpatch script:
> >
> > ERROR: Macros with multiple statements should be enclosed in a do - while loop
>
> I don't think this change will compile.  See if you can figure out why
> not.

It did compile. I built the greybus driver and loaded it as well with the
modinfo tool. Can you please tell why you think it won't compile?

./drv

>
> julia
>
> >
> > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > ---
> >  drivers/staging/greybus/loopback.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> > index 1a61fce98056..37214cb43937 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)				\
> > +do {								\
> >  	gb_loopback_ro_stats_attr(field, min, u);		\
> >  	gb_loopback_ro_stats_attr(field, max, u);		\
> > -	gb_loopback_ro_avg_attr(field)
> > +	gb_loopback_ro_avg_attr(field);				\
> > +} while (0)
> >
> >  #define gb_loopback_attr(field, type)					\
> >  static ssize_t field##_show(struct device *dev,				\
> > --
> > 2.30.2
> >
> >
> >
> >
> >
Greg KH Oct. 16, 2022, 3:10 p.m. UTC | #3 
On Sun, Oct 16, 2022 at 10:19:12AM -0400, Deepak R Varma wrote:
> Include multiple statements of macro definition inside do-while{0} loop
> to avoid possible partial program execution. Issue reported by
> checkpatch script:
> 
> ERROR: Macros with multiple statements should be enclosed in a do - while loop
> 
> Signed-off-by: Deepak R Varma <drv@mailo.com>
> ---
>  drivers/staging/greybus/loopback.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> index 1a61fce98056..37214cb43937 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)				\
> +do {								\
>  	gb_loopback_ro_stats_attr(field, min, u);		\
>  	gb_loopback_ro_stats_attr(field, max, u);		\
> -	gb_loopback_ro_avg_attr(field)
> +	gb_loopback_ro_avg_attr(field);				\
> +} while (0)
> 
>  #define gb_loopback_attr(field, type)					\
>  static ssize_t field##_show(struct device *dev,				\
> --
> 2.30.2

Always test-build your changes before sending them out so you do not get
grumpy maintainers asking why you did not test-build your changes.

Also, don't bindly trust that checkpatch is always correct, you need to
read the C code to verify that it is a sane request.

thanks,

greg k-h
Julia Lawall Oct. 16, 2022, 3:10 p.m. UTC | #4 
On Sun, 16 Oct 2022, Deepak R Varma wrote:

> On Sun, Oct 16, 2022 at 04:51:09PM +0200, Julia Lawall wrote:
> >
> >
> > On Sun, 16 Oct 2022, Deepak R Varma wrote:
> >
> > > Include multiple statements of macro definition inside do-while{0} loop
> > > to avoid possible partial program execution. Issue reported by
> > > checkpatch script:
> > >
> > > ERROR: Macros with multiple statements should be enclosed in a do - while loop
> >
> > I don't think this change will compile.  See if you can figure out why
> > not.
>
> It did compile. I built the greybus driver and loaded it as well with the
> modinfo tool. Can you please tell why you think it won't compile?

Do you have a .o file for the .c file that you changed?

julia

>
> ./drv
>
> >
> > julia
> >
> > >
> > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > ---
> > >  drivers/staging/greybus/loopback.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> > > index 1a61fce98056..37214cb43937 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)				\
> > > +do {								\
> > >  	gb_loopback_ro_stats_attr(field, min, u);		\
> > >  	gb_loopback_ro_stats_attr(field, max, u);		\
> > > -	gb_loopback_ro_avg_attr(field)
> > > +	gb_loopback_ro_avg_attr(field);				\
> > > +} while (0)
> > >
> > >  #define gb_loopback_attr(field, type)					\
> > >  static ssize_t field##_show(struct device *dev,				\
> > > --
> > > 2.30.2
> > >
> > >
> > >
> > >
> > >
>
>
>
Deepak R Varma Oct. 16, 2022, 3:27 p.m. UTC | #5 
On Sun, Oct 16, 2022 at 05:10:17PM +0200, Julia Lawall wrote:
>
>
> On Sun, 16 Oct 2022, Deepak R Varma wrote:
>
> > On Sun, Oct 16, 2022 at 04:51:09PM +0200, Julia Lawall wrote:
> > >
> > >
> > > On Sun, 16 Oct 2022, Deepak R Varma wrote:
> > >
> > > > Include multiple statements of macro definition inside do-while{0} loop
> > > > to avoid possible partial program execution. Issue reported by
> > > > checkpatch script:
> > > >
> > > > ERROR: Macros with multiple statements should be enclosed in a do - while loop
> > >
> > > I don't think this change will compile.  See if you can figure out why
> > > not.
> >
> > It did compile. I built the greybus driver and loaded it as well with the
> > modinfo tool. Can you please tell why you think it won't compile?
>
> Do you have a .o file for the .c file that you changed?

I see many .o files and a greybus.ko as well, but not the loopback.o
Am I missing anything with my configuration? I did set Greybus Support to (M) in
the menuconfig.

Thank you,
./drv

>
> julia
>
> >
> > ./drv
> >
> > >
> > > julia
> > >
> > > >
> > > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > > ---
> > > >  drivers/staging/greybus/loopback.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> > > > index 1a61fce98056..37214cb43937 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)				\
> > > > +do {								\
> > > >  	gb_loopback_ro_stats_attr(field, min, u);		\
> > > >  	gb_loopback_ro_stats_attr(field, max, u);		\
> > > > -	gb_loopback_ro_avg_attr(field)
> > > > +	gb_loopback_ro_avg_attr(field);				\
> > > > +} while (0)
> > > >
> > > >  #define gb_loopback_attr(field, type)					\
> > > >  static ssize_t field##_show(struct device *dev,				\
> > > > --
> > > > 2.30.2
> > > >
> > > >
> > > >
> > > >
> > > >
> >
> >
> >
>
Julia Lawall Oct. 16, 2022, 3:37 p.m. UTC | #6 
On Sun, 16 Oct 2022, Deepak R Varma wrote:

> On Sun, Oct 16, 2022 at 05:10:17PM +0200, Julia Lawall wrote:
> >
> >
> > On Sun, 16 Oct 2022, Deepak R Varma wrote:
> >
> > > On Sun, Oct 16, 2022 at 04:51:09PM +0200, Julia Lawall wrote:
> > > >
> > > >
> > > > On Sun, 16 Oct 2022, Deepak R Varma wrote:
> > > >
> > > > > Include multiple statements of macro definition inside do-while{0} loop
> > > > > to avoid possible partial program execution. Issue reported by
> > > > > checkpatch script:
> > > > >
> > > > > ERROR: Macros with multiple statements should be enclosed in a do - while loop
> > > >
> > > > I don't think this change will compile.  See if you can figure out why
> > > > not.
> > >
> > > It did compile. I built the greybus driver and loaded it as well with the
> > > modinfo tool. Can you please tell why you think it won't compile?
> >
> > Do you have a .o file for the .c file that you changed?
>
> I see many .o files and a greybus.ko as well, but not the loopback.o
> Am I missing anything with my configuration? I did set Greybus Support to (M) in
> the menuconfig.

Something must be missing in the configuration.

With make allyesconfig, you can just compile the file you changed, eg make
drivers/staging/greybus/loopback.o and see if just that file compiles.

Sometimes you can's compile an individual file.  In that case, it may be
possible to do make linux/file/path/ (assuming your file is in
linux/file/path/foo.c).  The trailing / is essential.  make
linux/file/path will do nothing.

julia

>
> Thank you,
> ./drv
>
> >
> > julia
> >
> > >
> > > ./drv
> > >
> > > >
> > > > julia
> > > >
> > > > >
> > > > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > > > ---
> > > > >  drivers/staging/greybus/loopback.c | 4 +++-
> > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> > > > > index 1a61fce98056..37214cb43937 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)				\
> > > > > +do {								\
> > > > >  	gb_loopback_ro_stats_attr(field, min, u);		\
> > > > >  	gb_loopback_ro_stats_attr(field, max, u);		\
> > > > > -	gb_loopback_ro_avg_attr(field)
> > > > > +	gb_loopback_ro_avg_attr(field);				\
> > > > > +} while (0)
> > > > >
> > > > >  #define gb_loopback_attr(field, type)					\
> > > > >  static ssize_t field##_show(struct device *dev,				\
> > > > > --
> > > > > 2.30.2
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > >
> > >
> > >
> >
>
>
>
Greg KH Oct. 16, 2022, 3:40 p.m. UTC | #7 
On Sun, Oct 16, 2022 at 11:27:30AM -0400, Deepak R Varma wrote:
> On Sun, Oct 16, 2022 at 05:10:17PM +0200, Julia Lawall wrote:
> >
> >
> > On Sun, 16 Oct 2022, Deepak R Varma wrote:
> >
> > > On Sun, Oct 16, 2022 at 04:51:09PM +0200, Julia Lawall wrote:
> > > >
> > > >
> > > > On Sun, 16 Oct 2022, Deepak R Varma wrote:
> > > >
> > > > > Include multiple statements of macro definition inside do-while{0} loop
> > > > > to avoid possible partial program execution. Issue reported by
> > > > > checkpatch script:
> > > > >
> > > > > ERROR: Macros with multiple statements should be enclosed in a do - while loop
> > > >
> > > > I don't think this change will compile.  See if you can figure out why
> > > > not.
> > >
> > > It did compile. I built the greybus driver and loaded it as well with the
> > > modinfo tool. Can you please tell why you think it won't compile?
> >
> > Do you have a .o file for the .c file that you changed?
> 
> I see many .o files and a greybus.ko as well, but not the loopback.o
> Am I missing anything with my configuration? I did set Greybus Support to (M) in
> the menuconfig.

CONFIG_GREYBUS_LOOPBACK has to be enabled in order to build the
drivers/staging/greybus/loopback.c file.

A simple check would be to do:
	make drivers/staging/greybus/loopback.o

does that work with your change?

thanks,

greg k-h
Deepak R Varma Oct. 16, 2022, 3:50 p.m. UTC | #8 
On Sun, Oct 16, 2022 at 05:40:59PM +0200, Greg KH wrote:
> On Sun, Oct 16, 2022 at 11:27:30AM -0400, Deepak R Varma wrote:
> > On Sun, Oct 16, 2022 at 05:10:17PM +0200, Julia Lawall wrote:
> > >
> > >
> > > On Sun, 16 Oct 2022, Deepak R Varma wrote:
> > >
> > > > On Sun, Oct 16, 2022 at 04:51:09PM +0200, Julia Lawall wrote:
> > > > >
> > > > >
> > > > > On Sun, 16 Oct 2022, Deepak R Varma wrote:
> > > > >
> > > > > > Include multiple statements of macro definition inside do-while{0} loop
> > > > > > to avoid possible partial program execution. Issue reported by
> > > > > > checkpatch script:
> > > > > >
> > > > > > ERROR: Macros with multiple statements should be enclosed in a do - while loop
> > > > >
> > > > > I don't think this change will compile.  See if you can figure out why
> > > > > not.
> > > >
> > > > It did compile. I built the greybus driver and loaded it as well with the
> > > > modinfo tool. Can you please tell why you think it won't compile?
> > >
> > > Do you have a .o file for the .c file that you changed?
> >
> > I see many .o files and a greybus.ko as well, but not the loopback.o
> > Am I missing anything with my configuration? I did set Greybus Support to (M) in
> > the menuconfig.
>
> CONFIG_GREYBUS_LOOPBACK has to be enabled in order to build the
> drivers/staging/greybus/loopback.c file.
>
> A simple check would be to do:
> 	make drivers/staging/greybus/loopback.o
>
> does that work with your change?

No, it did not. I understand why it did not. My apologies for not looking into
the build of loopback.o file when the greybus module was rebuilt.

Please ignore my patch.

Thank you Julia and Greg for the feedback.
./drv



>
> thanks,
>
> greg k-h
>
Deepak R Varma Oct. 16, 2022, 4:14 p.m. UTC | #9 
On Sun, Oct 16, 2022 at 04:51:09PM +0200, Julia Lawall wrote:
>
>
> On Sun, 16 Oct 2022, Deepak R Varma wrote:
>
> > Include multiple statements of macro definition inside do-while{0} loop
> > to avoid possible partial program execution. Issue reported by
> > checkpatch script:
> >
> > ERROR: Macros with multiple statements should be enclosed in a do - while loop
>
> I don't think this change will compile.  See if you can figure out why
> not.

After trying a direct compile of the loopback.c file, it did not compile. The
kernel build ran did not compile the loopback.c file due to missing
configuration.

About this change, the macro expands to function declarations at compile time
and those can't be enclosed in do/while loop as these are not logical execution
instructions. So it won't compile.

I initially looked for "greybus driver" under the "main menu > drivers >
staging drivers" path, but not find any configurations for the driver. While
retuning back, I found "greybus support" config under "menu-menu > device
drivers" itself. I set it to "M" and build the module. I did not realise that
setting this parameter to "M" actually results in adding several configurations
under "staging drivers" path. I now understand that.

Thank you for your help. I will look for another sensible change and send my new
first patch.

./drv

>
> julia
>
> >
> > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > ---
> >  drivers/staging/greybus/loopback.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> > index 1a61fce98056..37214cb43937 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)				\
> > +do {								\
> >  	gb_loopback_ro_stats_attr(field, min, u);		\
> >  	gb_loopback_ro_stats_attr(field, max, u);		\
> > -	gb_loopback_ro_avg_attr(field)
> > +	gb_loopback_ro_avg_attr(field);				\
> > +} while (0)
> >
> >  #define gb_loopback_attr(field, type)					\
> >  static ssize_t field##_show(struct device *dev,				\
> > --
> > 2.30.2
> >
> >
> >
> >
> >
kernel test robot Oct. 19, 2022, 3:16 p.m. UTC | #10 
Hi Deepak,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on staging/staging-testing]

url:    https://github.com/intel-lab-lkp/linux/commits/Deepak-R-Varma/staging-greybus-loopback-enclose-macro-statements-in-do-while-loop/20221017-120025
patch link:    https://lore.kernel.org/r/Y0wS4HQo9m%2FW%2FTrQ%40debian-BULLSEYE-live-builder-AMD64
patch subject: [PATCH] staging: greybus: loopback: enclose macro statements in do-while loop
config: sparc64-randconfig-r005-20221017
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/5196dd42760e5aed5f688894b7753a4f70bd097b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Deepak-R-Varma/staging-greybus-loopback-enclose-macro-statements-in-do-while-loop/20221017-120025
        git checkout 5196dd42760e5aed5f688894b7753a4f70bd097b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc64 SHELL=/bin/bash M=drivers/staging/greybus

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/staging/greybus/loopback.c:166:1: error: expected identifier or '(' before 'do'
     166 | do {                                                            \
         | ^~
   drivers/staging/greybus/loopback.c:273:1: note: in expansion of macro 'gb_loopback_stats_attrs'
     273 | gb_loopback_stats_attrs(latency);
         | ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/staging/greybus/loopback.c:170:3: error: expected identifier or '(' before 'while'
     170 | } while (0)
         |   ^~~~~
   drivers/staging/greybus/loopback.c:273:1: note: in expansion of macro 'gb_loopback_stats_attrs'
     273 | gb_loopback_stats_attrs(latency);
         | ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/staging/greybus/loopback.c:166:1: error: expected identifier or '(' before 'do'
     166 | do {                                                            \
         | ^~
   drivers/staging/greybus/loopback.c:275:1: note: in expansion of macro 'gb_loopback_stats_attrs'
     275 | gb_loopback_stats_attrs(requests_per_second);
         | ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/staging/greybus/loopback.c:170:3: error: expected identifier or '(' before 'while'
     170 | } while (0)
         |   ^~~~~
   drivers/staging/greybus/loopback.c:275:1: note: in expansion of macro 'gb_loopback_stats_attrs'
     275 | gb_loopback_stats_attrs(requests_per_second);
         | ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/staging/greybus/loopback.c:166:1: error: expected identifier or '(' before 'do'
     166 | do {                                                            \
         | ^~
   drivers/staging/greybus/loopback.c:277:1: note: in expansion of macro 'gb_loopback_stats_attrs'
     277 | gb_loopback_stats_attrs(throughput);
         | ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/staging/greybus/loopback.c:170:3: error: expected identifier or '(' before 'while'
     170 | } while (0)
         |   ^~~~~
   drivers/staging/greybus/loopback.c:277:1: note: in expansion of macro 'gb_loopback_stats_attrs'
     277 | gb_loopback_stats_attrs(throughput);
         | ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/staging/greybus/loopback.c:166:1: error: expected identifier or '(' before 'do'
     166 | do {                                                            \
         | ^~
   drivers/staging/greybus/loopback.c:279:1: note: in expansion of macro 'gb_loopback_stats_attrs'
     279 | gb_loopback_stats_attrs(apbridge_unipro_latency);
         | ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/staging/greybus/loopback.c:170:3: error: expected identifier or '(' before 'while'
     170 | } while (0)
         |   ^~~~~
   drivers/staging/greybus/loopback.c:279:1: note: in expansion of macro 'gb_loopback_stats_attrs'
     279 | gb_loopback_stats_attrs(apbridge_unipro_latency);
         | ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/staging/greybus/loopback.c:166:1: error: expected identifier or '(' before 'do'
     166 | do {                                                            \
         | ^~
   drivers/staging/greybus/loopback.c:281:1: note: in expansion of macro 'gb_loopback_stats_attrs'
     281 | gb_loopback_stats_attrs(gbphy_firmware_latency);
         | ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/staging/greybus/loopback.c:170:3: error: expected identifier or '(' before 'while'
     170 | } while (0)
         |   ^~~~~
   drivers/staging/greybus/loopback.c:281:1: note: in expansion of macro 'gb_loopback_stats_attrs'
     281 | gb_loopback_stats_attrs(gbphy_firmware_latency);
         | ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/staging/greybus/loopback.c:319:10: error: 'dev_attr_latency_min' undeclared here (not in a function); did you mean 'dev_attr_timeout_min'?
     319 |         &dev_attr_latency_min.attr,
         |          ^~~~~~~~~~~~~~~~~~~~
         |          dev_attr_timeout_min
>> drivers/staging/greybus/loopback.c:320:10: error: 'dev_attr_latency_max' undeclared here (not in a function); did you mean 'dev_attr_timeout_max'?
     320 |         &dev_attr_latency_max.attr,
         |          ^~~~~~~~~~~~~~~~~~~~
         |          dev_attr_timeout_max
>> drivers/staging/greybus/loopback.c:321:10: error: 'dev_attr_latency_avg' undeclared here (not in a function)
     321 |         &dev_attr_latency_avg.attr,
         |          ^~~~~~~~~~~~~~~~~~~~
>> drivers/staging/greybus/loopback.c:322:10: error: 'dev_attr_requests_per_second_min' undeclared here (not in a function)
     322 |         &dev_attr_requests_per_second_min.attr,
         |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/staging/greybus/loopback.c:323:10: error: 'dev_attr_requests_per_second_max' undeclared here (not in a function)
     323 |         &dev_attr_requests_per_second_max.attr,
         |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/staging/greybus/loopback.c:324:10: error: 'dev_attr_requests_per_second_avg' undeclared here (not in a function)
     324 |         &dev_attr_requests_per_second_avg.attr,
         |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/staging/greybus/loopback.c:325:10: error: 'dev_attr_throughput_min' undeclared here (not in a function); did you mean 'dev_attr_timeout_min'?
     325 |         &dev_attr_throughput_min.attr,
         |          ^~~~~~~~~~~~~~~~~~~~~~~
         |          dev_attr_timeout_min
>> drivers/staging/greybus/loopback.c:326:10: error: 'dev_attr_throughput_max' undeclared here (not in a function); did you mean 'dev_attr_timeout_max'?
     326 |         &dev_attr_throughput_max.attr,
         |          ^~~~~~~~~~~~~~~~~~~~~~~
         |          dev_attr_timeout_max
>> drivers/staging/greybus/loopback.c:327:10: error: 'dev_attr_throughput_avg' undeclared here (not in a function)
     327 |         &dev_attr_throughput_avg.attr,
         |          ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/staging/greybus/loopback.c:328:10: error: 'dev_attr_apbridge_unipro_latency_min' undeclared here (not in a function)
     328 |         &dev_attr_apbridge_unipro_latency_min.attr,
         |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/staging/greybus/loopback.c:329:10: error: 'dev_attr_apbridge_unipro_latency_max' undeclared here (not in a function)
     329 |         &dev_attr_apbridge_unipro_latency_max.attr,
         |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/staging/greybus/loopback.c:330:10: error: 'dev_attr_apbridge_unipro_latency_avg' undeclared here (not in a function)
     330 |         &dev_attr_apbridge_unipro_latency_avg.attr,
         |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/staging/greybus/loopback.c:331:10: error: 'dev_attr_gbphy_firmware_latency_min' undeclared here (not in a function)
     331 |         &dev_attr_gbphy_firmware_latency_min.attr,
         |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/staging/greybus/loopback.c:332:10: error: 'dev_attr_gbphy_firmware_latency_max' undeclared here (not in a function)
     332 |         &dev_attr_gbphy_firmware_latency_max.attr,
         |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/staging/greybus/loopback.c:333:10: error: 'dev_attr_gbphy_firmware_latency_avg' undeclared here (not in a function)
     333 |         &dev_attr_gbphy_firmware_latency_avg.attr,
         |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +166 drivers/staging/greybus/loopback.c

   164	
   165	#define gb_loopback_stats_attrs(field)				\
 > 166	do {								\
   167		gb_loopback_ro_stats_attr(field, min, u);		\
   168		gb_loopback_ro_stats_attr(field, max, u);		\
   169		gb_loopback_ro_avg_attr(field);				\
 > 170	} while (0)
   171	
   172	#define gb_loopback_attr(field, type)					\
   173	static ssize_t field##_show(struct device *dev,				\
   174				    struct device_attribute *attr,		\
   175				    char *buf)					\
   176	{									\
   177		struct gb_loopback *gb = dev_get_drvdata(dev);			\
   178		return sprintf(buf, "%" #type "\n", gb->field);			\
   179	}									\
   180	static ssize_t field##_store(struct device *dev,			\
   181				    struct device_attribute *attr,		\
   182				    const char *buf,				\
   183				    size_t len)					\
   184	{									\
   185		int ret;							\
   186		struct gb_loopback *gb = dev_get_drvdata(dev);			\
   187		mutex_lock(&gb->mutex);						\
   188		ret = sscanf(buf, "%"#type, &gb->field);			\
   189		if (ret != 1)							\
   190			len = -EINVAL;						\
   191		else								\
   192			gb_loopback_check_attr(gb, bundle);			\
   193		mutex_unlock(&gb->mutex);					\
   194		return len;							\
   195	}									\
   196	static DEVICE_ATTR_RW(field)
   197	
   198	#define gb_dev_loopback_ro_attr(field, conn)				\
   199	static ssize_t field##_show(struct device *dev,		\
   200				    struct device_attribute *attr,		\
   201				    char *buf)					\
   202	{									\
   203		struct gb_loopback *gb = dev_get_drvdata(dev);			\
   204		return sprintf(buf, "%u\n", gb->field);				\
   205	}									\
   206	static DEVICE_ATTR_RO(field)
   207	
   208	#define gb_dev_loopback_rw_attr(field, type)				\
   209	static ssize_t field##_show(struct device *dev,				\
   210				    struct device_attribute *attr,		\
   211				    char *buf)					\
   212	{									\
   213		struct gb_loopback *gb = dev_get_drvdata(dev);			\
   214		return sprintf(buf, "%" #type "\n", gb->field);			\
   215	}									\
   216	static ssize_t field##_store(struct device *dev,			\
   217				    struct device_attribute *attr,		\
   218				    const char *buf,				\
   219				    size_t len)					\
   220	{									\
   221		int ret;							\
   222		struct gb_loopback *gb = dev_get_drvdata(dev);			\
   223		mutex_lock(&gb->mutex);						\
   224		ret = sscanf(buf, "%"#type, &gb->field);			\
   225		if (ret != 1)							\
   226			len = -EINVAL;						\
   227		else								\
   228			gb_loopback_check_attr(gb);		\
   229		mutex_unlock(&gb->mutex);					\
   230		return len;							\
   231	}									\
   232	static DEVICE_ATTR_RW(field)
   233	
   234	static void gb_loopback_reset_stats(struct gb_loopback *gb);
   235	static void gb_loopback_check_attr(struct gb_loopback *gb)
   236	{
   237		if (gb->us_wait > GB_LOOPBACK_US_WAIT_MAX)
   238			gb->us_wait = GB_LOOPBACK_US_WAIT_MAX;
   239		if (gb->size > gb_dev.size_max)
   240			gb->size = gb_dev.size_max;
   241		gb->requests_timedout = 0;
   242		gb->requests_completed = 0;
   243		gb->iteration_count = 0;
   244		gb->send_count = 0;
   245		gb->error = 0;
   246	
   247		if (kfifo_depth < gb->iteration_max) {
   248			dev_warn(gb->dev,
   249				 "cannot log bytes %u kfifo_depth %u\n",
   250				 gb->iteration_max, kfifo_depth);
   251		}
   252		kfifo_reset_out(&gb->kfifo_lat);
   253	
   254		switch (gb->type) {
   255		case GB_LOOPBACK_TYPE_PING:
   256		case GB_LOOPBACK_TYPE_TRANSFER:
   257		case GB_LOOPBACK_TYPE_SINK:
   258			gb->jiffy_timeout = usecs_to_jiffies(gb->timeout);
   259			if (!gb->jiffy_timeout)
   260				gb->jiffy_timeout = GB_LOOPBACK_TIMEOUT_MIN;
   261			else if (gb->jiffy_timeout > GB_LOOPBACK_TIMEOUT_MAX)
   262				gb->jiffy_timeout = GB_LOOPBACK_TIMEOUT_MAX;
   263			gb_loopback_reset_stats(gb);
   264			wake_up(&gb->wq);
   265			break;
   266		default:
   267			gb->type = 0;
   268			break;
   269		}
   270	}
   271	
   272	/* Time to send and receive one message */
   273	gb_loopback_stats_attrs(latency);
   274	/* Number of requests sent per second on this cport */
   275	gb_loopback_stats_attrs(requests_per_second);
   276	/* Quantity of data sent and received on this cport */
   277	gb_loopback_stats_attrs(throughput);
   278	/* Latency across the UniPro link from APBridge's perspective */
   279	gb_loopback_stats_attrs(apbridge_unipro_latency);
   280	/* Firmware induced overhead in the GPBridge */
   281	gb_loopback_stats_attrs(gbphy_firmware_latency);
   282	
   283	/* Number of errors encountered during loop */
   284	gb_loopback_ro_attr(error);
   285	/* Number of requests successfully completed async */
   286	gb_loopback_ro_attr(requests_completed);
   287	/* Number of requests timed out async */
   288	gb_loopback_ro_attr(requests_timedout);
   289	/* Timeout minimum in useconds */
   290	gb_loopback_ro_attr(timeout_min);
   291	/* Timeout minimum in useconds */
   292	gb_loopback_ro_attr(timeout_max);
   293	
   294	/*
   295	 * Type of loopback message to send based on protocol type definitions
   296	 * 0 => Don't send message
   297	 * 2 => Send ping message continuously (message without payload)
   298	 * 3 => Send transfer message continuously (message with payload,
   299	 *					   payload returned in response)
   300	 * 4 => Send a sink message (message with payload, no payload in response)
   301	 */
   302	gb_dev_loopback_rw_attr(type, d);
   303	/* Size of transfer message payload: 0-4096 bytes */
   304	gb_dev_loopback_rw_attr(size, u);
   305	/* Time to wait between two messages: 0-1000 ms */
   306	gb_dev_loopback_rw_attr(us_wait, d);
   307	/* Maximum iterations for a given operation: 1-(2^32-1), 0 implies infinite */
   308	gb_dev_loopback_rw_attr(iteration_max, u);
   309	/* The current index of the for (i = 0; i < iteration_max; i++) loop */
   310	gb_dev_loopback_ro_attr(iteration_count, false);
   311	/* A flag to indicate synchronous or asynchronous operations */
   312	gb_dev_loopback_rw_attr(async, u);
   313	/* Timeout of an individual asynchronous request */
   314	gb_dev_loopback_rw_attr(timeout, u);
   315	/* Maximum number of in-flight operations before back-off */
   316	gb_dev_loopback_rw_attr(outstanding_operations_max, u);
   317	
   318	static struct attribute *loopback_attrs[] = {
 > 319		&dev_attr_latency_min.attr,
 > 320		&dev_attr_latency_max.attr,
 > 321		&dev_attr_latency_avg.attr,
 > 322		&dev_attr_requests_per_second_min.attr,
 > 323		&dev_attr_requests_per_second_max.attr,
 > 324		&dev_attr_requests_per_second_avg.attr,
 > 325		&dev_attr_throughput_min.attr,
 > 326		&dev_attr_throughput_max.attr,
 > 327		&dev_attr_throughput_avg.attr,
 > 328		&dev_attr_apbridge_unipro_latency_min.attr,
 > 329		&dev_attr_apbridge_unipro_latency_max.attr,
 > 330		&dev_attr_apbridge_unipro_latency_avg.attr,
 > 331		&dev_attr_gbphy_firmware_latency_min.attr,
 > 332		&dev_attr_gbphy_firmware_latency_max.attr,
 > 333		&dev_attr_gbphy_firmware_latency_avg.attr,
   334		&dev_attr_type.attr,
   335		&dev_attr_size.attr,
   336		&dev_attr_us_wait.attr,
   337		&dev_attr_iteration_count.attr,
   338		&dev_attr_iteration_max.attr,
   339		&dev_attr_async.attr,
   340		&dev_attr_error.attr,
   341		&dev_attr_requests_completed.attr,
   342		&dev_attr_requests_timedout.attr,
   343		&dev_attr_timeout.attr,
   344		&dev_attr_outstanding_operations_max.attr,
   345		&dev_attr_timeout_min.attr,
   346		&dev_attr_timeout_max.attr,
   347		NULL,
   348	};
   349	ATTRIBUTE_GROUPS(loopback);
   350
Deepak R Varma Oct. 19, 2022, 3:28 p.m. UTC | #11 
On Sun, Oct 16, 2022 at 11:50:45AM -0400, Deepak R Varma wrote:
> On Sun, Oct 16, 2022 at 05:40:59PM +0200, Greg KH wrote:
> > On Sun, Oct 16, 2022 at 11:27:30AM -0400, Deepak R Varma wrote:
> > > On Sun, Oct 16, 2022 at 05:10:17PM +0200, Julia Lawall wrote:
> > > >
> > > >
> > > > On Sun, 16 Oct 2022, Deepak R Varma wrote:
> > > >
> > > > > On Sun, Oct 16, 2022 at 04:51:09PM +0200, Julia Lawall wrote:
> > > > > >
> > > > > >
> > > > > > On Sun, 16 Oct 2022, Deepak R Varma wrote:
> > > > > >
> > > > > > > Include multiple statements of macro definition inside do-while{0} loop
> > > > > > > to avoid possible partial program execution. Issue reported by
> > > > > > > checkpatch script:
> > > > > > >
> > > > > > > ERROR: Macros with multiple statements should be enclosed in a do - while loop
> > > > > >
> > > > > > I don't think this change will compile.  See if you can figure out why
> > > > > > not.
> > > > >
> > > > > It did compile. I built the greybus driver and loaded it as well with the
> > > > > modinfo tool. Can you please tell why you think it won't compile?
> > > >
> > > > Do you have a .o file for the .c file that you changed?
> > >
> > > I see many .o files and a greybus.ko as well, but not the loopback.o
> > > Am I missing anything with my configuration? I did set Greybus Support to (M) in
> > > the menuconfig.
> >
> > CONFIG_GREYBUS_LOOPBACK has to be enabled in order to build the
> > drivers/staging/greybus/loopback.c file.
> >
> > A simple check would be to do:
> > 	make drivers/staging/greybus/loopback.o
> >
> > does that work with your change?
>
> No, it did not. I understand why it did not. My apologies for not looking into
> the build of loopback.o file when the greybus module was rebuilt.
>
> Please ignore my patch.

I just received a message from Kernel Test Robot that this patch failed to
compile. I had requested to drop/ignore this patch. However, looks like it
included in the staging-testing tree????

Let me know anything required from me to fix the bot complaint. Thank you.

>
> Thank you Julia and Greg for the feedback.
> ./drv
>
>
>
> >
> > thanks,
> >
> > greg k-h
> >
>
>
>
Greg KH Oct. 20, 2022, 3:40 p.m. UTC | #12 
On Wed, Oct 19, 2022 at 08:58:17PM +0530, Deepak R Varma wrote:
> On Sun, Oct 16, 2022 at 11:50:45AM -0400, Deepak R Varma wrote:
> > On Sun, Oct 16, 2022 at 05:40:59PM +0200, Greg KH wrote:
> > > On Sun, Oct 16, 2022 at 11:27:30AM -0400, Deepak R Varma wrote:
> > > > On Sun, Oct 16, 2022 at 05:10:17PM +0200, Julia Lawall wrote:
> > > > >
> > > > >
> > > > > On Sun, 16 Oct 2022, Deepak R Varma wrote:
> > > > >
> > > > > > On Sun, Oct 16, 2022 at 04:51:09PM +0200, Julia Lawall wrote:
> > > > > > >
> > > > > > >
> > > > > > > On Sun, 16 Oct 2022, Deepak R Varma wrote:
> > > > > > >
> > > > > > > > Include multiple statements of macro definition inside do-while{0} loop
> > > > > > > > to avoid possible partial program execution. Issue reported by
> > > > > > > > checkpatch script:
> > > > > > > >
> > > > > > > > ERROR: Macros with multiple statements should be enclosed in a do - while loop
> > > > > > >
> > > > > > > I don't think this change will compile.  See if you can figure out why
> > > > > > > not.
> > > > > >
> > > > > > It did compile. I built the greybus driver and loaded it as well with the
> > > > > > modinfo tool. Can you please tell why you think it won't compile?
> > > > >
> > > > > Do you have a .o file for the .c file that you changed?
> > > >
> > > > I see many .o files and a greybus.ko as well, but not the loopback.o
> > > > Am I missing anything with my configuration? I did set Greybus Support to (M) in
> > > > the menuconfig.
> > >
> > > CONFIG_GREYBUS_LOOPBACK has to be enabled in order to build the
> > > drivers/staging/greybus/loopback.c file.
> > >
> > > A simple check would be to do:
> > > 	make drivers/staging/greybus/loopback.o
> > >
> > > does that work with your change?
> >
> > No, it did not. I understand why it did not. My apologies for not looking into
> > the build of loopback.o file when the greybus module was rebuilt.
> >
> > Please ignore my patch.
> 
> I just received a message from Kernel Test Robot that this patch failed to
> compile. I had requested to drop/ignore this patch. However, looks like it
> included in the staging-testing tree????

It just took a while to get to your patch, it picked it up off of the
mailing list, the change is not applied anywhere.

> Let me know anything required from me to fix the bot complaint. Thank you.

There's nothing to do as it's long-gone from my queue.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
index 1a61fce98056..37214cb43937 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)				\
+do {								\
 	gb_loopback_ro_stats_attr(field, min, u);		\
 	gb_loopback_ro_stats_attr(field, max, u);		\
-	gb_loopback_ro_avg_attr(field)
+	gb_loopback_ro_avg_attr(field);				\
+} while (0)

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