The gb_loopback_stats_attrs macro, (defined in loopback.c,) is a
multiline macro whose statements were not enclosed in a do while
loop.
This patch adds a do while loop around the statements of the said
macro.
Signed-off-by: Shreyansh Chouhan <[email protected]>
---
drivers/staging/greybus/loopback.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
index 2471448ba42a..c88ef3e894fa 100644
--- a/drivers/staging/greybus/loopback.c
+++ b/drivers/staging/greybus/loopback.c
@@ -162,10 +162,12 @@ 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_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); \
+ } while (0)
#define gb_loopback_attr(field, type) \
static ssize_t field##_show(struct device *dev, \
--
2.31.1
On Fri, May 14, 2021 at 07:00:39PM +0530, Shreyansh Chouhan wrote:
> The gb_loopback_stats_attrs macro, (defined in loopback.c,) is a
> multiline macro whose statements were not enclosed in a do while
> loop.
>
> This patch adds a do while loop around the statements of the said
> macro.
>
> Signed-off-by: Shreyansh Chouhan <[email protected]>
> ---
> drivers/staging/greybus/loopback.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> index 2471448ba42a..c88ef3e894fa 100644
> --- a/drivers/staging/greybus/loopback.c
> +++ b/drivers/staging/greybus/loopback.c
> @@ -162,10 +162,12 @@ 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_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); \
> + } while (0)
>
> #define gb_loopback_attr(field, type) \
> static ssize_t field##_show(struct device *dev, \
> --
> 2.31.1
>
>
Did you test build this change?
On Fri, May 14, 2021 at 07:18:38PM +0530, Shreyansh Chouhan wrote:
> On Fri, May 14, 2021 at 03:36:25PM +0200, Greg KH wrote:
> > On Fri, May 14, 2021 at 07:00:39PM +0530, Shreyansh Chouhan wrote:
> > > The gb_loopback_stats_attrs macro, (defined in loopback.c,) is a
> > > multiline macro whose statements were not enclosed in a do while
> > > loop.
> > >
> > > This patch adds a do while loop around the statements of the said
> > > macro.
> > >
> > > Signed-off-by: Shreyansh Chouhan <[email protected]>
> > > ---
> > > drivers/staging/greybus/loopback.c | 10 ++++++----
> > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> > > index 2471448ba42a..c88ef3e894fa 100644
> > > --- a/drivers/staging/greybus/loopback.c
> > > +++ b/drivers/staging/greybus/loopback.c
> > > @@ -162,10 +162,12 @@ 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_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); \
> > > + } while (0)
> > >
> > > #define gb_loopback_attr(field, type) \
> > > static ssize_t field##_show(struct device *dev, \
> > > --
> > > 2.31.1
> > >
> > >
> >
> > Did you test build this change?
>
> I built the module using make -C . M=drivers/staging/greybus to test
> build it. I didn't get any errors.
Really? Can you provide the full build output for this file with your
change? I don't think you really built this file for the obvious
reasons...
thanks,
greg k-h
On Fri, May 14, 2021 at 04:05:32PM +0200, Greg KH wrote:
> On Fri, May 14, 2021 at 07:18:38PM +0530, Shreyansh Chouhan wrote:
> > On Fri, May 14, 2021 at 03:36:25PM +0200, Greg KH wrote:
> > > On Fri, May 14, 2021 at 07:00:39PM +0530, Shreyansh Chouhan wrote:
> > > > The gb_loopback_stats_attrs macro, (defined in loopback.c,) is a
> > > > multiline macro whose statements were not enclosed in a do while
> > > > loop.
> > > >
> > > > This patch adds a do while loop around the statements of the said
> > > > macro.
> > > >
> > > > Signed-off-by: Shreyansh Chouhan <[email protected]>
> > > > ---
> > > > drivers/staging/greybus/loopback.c | 10 ++++++----
> > > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> > > > index 2471448ba42a..c88ef3e894fa 100644
> > > > --- a/drivers/staging/greybus/loopback.c
> > > > +++ b/drivers/staging/greybus/loopback.c
> > > > @@ -162,10 +162,12 @@ 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_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); \
> > > > + } while (0)
> > > >
> > > > #define gb_loopback_attr(field, type) \
> > > > static ssize_t field##_show(struct device *dev, \
> > > > --
> > > > 2.31.1
> > > >
> > > >
> > >
> > > Did you test build this change?
> >
> > I built the module using make -C . M=drivers/staging/greybus to test
> > build it. I didn't get any errors.
>
> Really? Can you provide the full build output for this file with your
> change? I don't think you really built this file for the obvious
> reasons...
I ran make -C . M=drivers/staging/greybus
I got a three line output saying:
make: Entering directory '/work/linux'
MODPOST drivers/staging/greybus//Module.symvers
make: Leaving directory '/work/linux'
I just tried rebuilding the kernel with CONFIG_GREYBUS=m, and now I can
see what you are talking about. Why weren't these errors reported when I
ran the previous make command? Does that too check for the config
variables even when I specifically asked it to build a module?
Also sorry about this. I will be more careful next time.
>
> thanks,
>
> greg k-h
Regards,
Shreyansh Chouhan
On Fri, May 14, 2021 at 04:30:23PM +0200, Greg KH wrote:
> On Fri, May 14, 2021 at 07:53:57PM +0530, Shreyansh Chouhan wrote:
> > On Fri, May 14, 2021 at 04:05:32PM +0200, Greg KH wrote:
> > > On Fri, May 14, 2021 at 07:18:38PM +0530, Shreyansh Chouhan wrote:
> > > > On Fri, May 14, 2021 at 03:36:25PM +0200, Greg KH wrote:
> > > > > On Fri, May 14, 2021 at 07:00:39PM +0530, Shreyansh Chouhan wrote:
> > > > > > The gb_loopback_stats_attrs macro, (defined in loopback.c,) is a
> > > > > > multiline macro whose statements were not enclosed in a do while
> > > > > > loop.
> > > > > >
> > > > > > This patch adds a do while loop around the statements of the said
> > > > > > macro.
> > > > > >
> > > > > > Signed-off-by: Shreyansh Chouhan <[email protected]>
> > > > > > ---
> > > > > > drivers/staging/greybus/loopback.c | 10 ++++++----
> > > > > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> > > > > > index 2471448ba42a..c88ef3e894fa 100644
> > > > > > --- a/drivers/staging/greybus/loopback.c
> > > > > > +++ b/drivers/staging/greybus/loopback.c
> > > > > > @@ -162,10 +162,12 @@ 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_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); \
> > > > > > + } while (0)
> > > > > >
> > > > > > #define gb_loopback_attr(field, type) \
> > > > > > static ssize_t field##_show(struct device *dev, \
> > > > > > --
> > > > > > 2.31.1
> > > > > >
> > > > > >
> > > > >
> > > > > Did you test build this change?
> > > >
> > > > I built the module using make -C . M=drivers/staging/greybus to test
> > > > build it. I didn't get any errors.
> > >
> > > Really? Can you provide the full build output for this file with your
> > > change? I don't think you really built this file for the obvious
> > > reasons...
> >
> > I ran make -C . M=drivers/staging/greybus
> >
> > I got a three line output saying:
> > make: Entering directory '/work/linux'
> > MODPOST drivers/staging/greybus//Module.symvers
> > make: Leaving directory '/work/linux'
> >
> > I just tried rebuilding the kernel with CONFIG_GREYBUS=m, and now I can
> > see what you are talking about. Why weren't these errors reported when I
> > ran the previous make command? Does that too check for the config
> > variables even when I specifically asked it to build a module?
>
> You were just asking it to build a subdirectory, not a specific
> individual file, and when you do that it looks at the configuration
> settings.
>
I see.
> It's always good to ensure that you actually build the files you modify
> before sending patches out.
Sorry, I googled about building a single module, and thought running
that command would have built it. Moreover, since the change was so
simple I didn't suspect anything when it got built correctly the first
time around.
I didn't look at how/where was the macro called and missed a very
obvious error. Now that I have looked at it, the only way I can think of
fixing this is changing the macro to a (inline?) function. Will
that be a desirable change?
And yes, I will definitely be more careful in the future.
>
> thanks,
>
> greg k-h
On Fri, May 14, 2021 at 08:42:16PM +0530, Shreyansh Chouhan wrote:
> On Fri, May 14, 2021 at 04:30:23PM +0200, Greg KH wrote:
> > On Fri, May 14, 2021 at 07:53:57PM +0530, Shreyansh Chouhan wrote:
> > > On Fri, May 14, 2021 at 04:05:32PM +0200, Greg KH wrote:
> > > > On Fri, May 14, 2021 at 07:18:38PM +0530, Shreyansh Chouhan wrote:
> > > > > On Fri, May 14, 2021 at 03:36:25PM +0200, Greg KH wrote:
> > > > > > On Fri, May 14, 2021 at 07:00:39PM +0530, Shreyansh Chouhan wrote:
> > > > > > > The gb_loopback_stats_attrs macro, (defined in loopback.c,) is a
> > > > > > > multiline macro whose statements were not enclosed in a do while
> > > > > > > loop.
> > > > > > >
> > > > > > > This patch adds a do while loop around the statements of the said
> > > > > > > macro.
> > > > > > >
> > > > > > > Signed-off-by: Shreyansh Chouhan <[email protected]>
> > > > > > > ---
> > > > > > > drivers/staging/greybus/loopback.c | 10 ++++++----
> > > > > > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> > > > > > > index 2471448ba42a..c88ef3e894fa 100644
> > > > > > > --- a/drivers/staging/greybus/loopback.c
> > > > > > > +++ b/drivers/staging/greybus/loopback.c
> > > > > > > @@ -162,10 +162,12 @@ 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_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); \
> > > > > > > + } while (0)
> > > > > > >
> > > > > > > #define gb_loopback_attr(field, type) \
> > > > > > > static ssize_t field##_show(struct device *dev, \
> > > > > > > --
> > > > > > > 2.31.1
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > Did you test build this change?
> > > > >
> > > > > I built the module using make -C . M=drivers/staging/greybus to test
> > > > > build it. I didn't get any errors.
> > > >
> > > > Really? Can you provide the full build output for this file with your
> > > > change? I don't think you really built this file for the obvious
> > > > reasons...
> > >
> > > I ran make -C . M=drivers/staging/greybus
> > >
> > > I got a three line output saying:
> > > make: Entering directory '/work/linux'
> > > MODPOST drivers/staging/greybus//Module.symvers
> > > make: Leaving directory '/work/linux'
> > >
> > > I just tried rebuilding the kernel with CONFIG_GREYBUS=m, and now I can
> > > see what you are talking about. Why weren't these errors reported when I
> > > ran the previous make command? Does that too check for the config
> > > variables even when I specifically asked it to build a module?
> >
> > You were just asking it to build a subdirectory, not a specific
> > individual file, and when you do that it looks at the configuration
> > settings.
> >
>
> I see.
>
> > It's always good to ensure that you actually build the files you modify
> > before sending patches out.
>
> Sorry, I googled about building a single module, and thought running
> that command would have built it. Moreover, since the change was so
> simple I didn't suspect anything when it got built correctly the first
> time around.
>
> I didn't look at how/where was the macro called and missed a very
> obvious error. Now that I have looked at it, the only way I can think of
> fixing this is changing the macro to a (inline?) function. Will
> that be a desirable change?
No, it can't be a function, the code is fine as-is, checkpatch is just a
perl script and does not always know what needs to be done.
thanks,
greg k-h
On Fri, May 14, 2021 at 05:30:45PM +0200, Greg KH wrote:
> On Fri, May 14, 2021 at 08:42:16PM +0530, Shreyansh Chouhan wrote:
> > On Fri, May 14, 2021 at 04:30:23PM +0200, Greg KH wrote:
> > > On Fri, May 14, 2021 at 07:53:57PM +0530, Shreyansh Chouhan wrote:
> > > > On Fri, May 14, 2021 at 04:05:32PM +0200, Greg KH wrote:
> > > > > On Fri, May 14, 2021 at 07:18:38PM +0530, Shreyansh Chouhan wrote:
> > > > > > On Fri, May 14, 2021 at 03:36:25PM +0200, Greg KH wrote:
> > > > > > > On Fri, May 14, 2021 at 07:00:39PM +0530, Shreyansh Chouhan wrote:
> > > > > > > > The gb_loopback_stats_attrs macro, (defined in loopback.c,) is a
> > > > > > > > multiline macro whose statements were not enclosed in a do while
> > > > > > > > loop.
> > > > > > > >
> > > > > > > > This patch adds a do while loop around the statements of the said
> > > > > > > > macro.
> > > > > > > >
> > > > > > > > Signed-off-by: Shreyansh Chouhan <[email protected]>
> > > > > > > > ---
> > > > > > > > drivers/staging/greybus/loopback.c | 10 ++++++----
> > > > > > > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> > > > > > > > index 2471448ba42a..c88ef3e894fa 100644
> > > > > > > > --- a/drivers/staging/greybus/loopback.c
> > > > > > > > +++ b/drivers/staging/greybus/loopback.c
> > > > > > > > @@ -162,10 +162,12 @@ 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_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); \
> > > > > > > > + } while (0)
> > > > > > > >
> > > > > > > > #define gb_loopback_attr(field, type) \
> > > > > > > > static ssize_t field##_show(struct device *dev, \
> > > > > > > > --
> > > > > > > > 2.31.1
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > > Did you test build this change?
> > > > > >
> > > > > > I built the module using make -C . M=drivers/staging/greybus to test
> > > > > > build it. I didn't get any errors.
> > > > >
> > > > > Really? Can you provide the full build output for this file with your
> > > > > change? I don't think you really built this file for the obvious
> > > > > reasons...
> > > >
> > > > I ran make -C . M=drivers/staging/greybus
> > > >
> > > > I got a three line output saying:
> > > > make: Entering directory '/work/linux'
> > > > MODPOST drivers/staging/greybus//Module.symvers
> > > > make: Leaving directory '/work/linux'
> > > >
> > > > I just tried rebuilding the kernel with CONFIG_GREYBUS=m, and now I can
> > > > see what you are talking about. Why weren't these errors reported when I
> > > > ran the previous make command? Does that too check for the config
> > > > variables even when I specifically asked it to build a module?
> > >
> > > You were just asking it to build a subdirectory, not a specific
> > > individual file, and when you do that it looks at the configuration
> > > settings.
> > >
> >
> > I see.
> >
> > > It's always good to ensure that you actually build the files you modify
> > > before sending patches out.
> >
> > Sorry, I googled about building a single module, and thought running
> > that command would have built it. Moreover, since the change was so
> > simple I didn't suspect anything when it got built correctly the first
> > time around.
> >
> > I didn't look at how/where was the macro called and missed a very
> > obvious error. Now that I have looked at it, the only way I can think of
> > fixing this is changing the macro to a (inline?) function. Will
> > that be a desirable change?
>
> No, it can't be a function, the code is fine as-is, checkpatch is just a
> perl script and does not always know what needs to be done.
>
I see. Thanks a lot for answering my queries.
Also sorry for the noise.
> thanks,
>
> greg k-h
Regards,
Shreyansh Chouhan
On Fri, May 14, 2021 at 01:53:49PM -0500, Alex Elder wrote:
> On 5/14/21 10:56 AM, Joe Perches wrote:
> > On Fri, 2021-05-14 at 17:30 +0200, Greg KH wrote:
> > > On Fri, May 14, 2021 at 08:42:16PM +0530, Shreyansh Chouhan wrote:
> > []
> > > > I didn't look at how/where was the macro called and missed a very
> > > > obvious error. Now that I have looked at it, the only way I can think of
> > > > fixing this is changing the macro to a (inline?) function. Will
> > > > that be a desirable change?
> > >
> > > No, it can't be a function, the code is fine as-is, checkpatch is just a
> > > perl script and does not always know what needs to be done.
> >
> > true.
> >
> > perhaps better though to rename these declaring macros to start with declare_
>
> I don't disagree with your suggestion, but it's not clear it
> would have prevented submission of the erroneous initial patch
> (nor future ones from people who blindly follow checkpatch.pl
> suggestions).
>
Well if it makes any difference, I think such a name would at least make
things a little more clear. Also, adding a comment to the macro definition
might help with the problem of future erronous patches.
Regards,
Shreyansh Chouhan
> -Alex
>
> PS Lots of negatives in that sentence.
>
> > Something like this:
> > (with miscellaneous realigning of the macros line ending continuations \)
> > ---
> > drivers/staging/greybus/loopback.c | 72 +++++++++++++++++++-------------------
> > 1 file changed, 36 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> > index 2471448ba42a..dc399792f35f 100644
> > --- a/drivers/staging/greybus/loopback.c
> > +++ b/drivers/staging/greybus/loopback.c
> > @@ -119,18 +119,18 @@ module_param(kfifo_depth, uint, 0444);
> > #define GB_LOOPBACK_US_WAIT_MAX 1000000
> > /* interface sysfs attributes */
> > -#define gb_loopback_ro_attr(field) \
> > -static ssize_t field##_show(struct device *dev, \
> > +#define declare_gb_loopback_ro_attr(field) \
> > +static ssize_t field##_show(struct device *dev, \
> > struct device_attribute *attr, \
> > char *buf) \
> > { \
> > struct gb_loopback *gb = dev_get_drvdata(dev); \
> > - return sprintf(buf, "%u\n", gb->field); \
> > + return sprintf(buf, "%u\n", gb->field); \
> > } \
> > static DEVICE_ATTR_RO(field)
> > -#define gb_loopback_ro_stats_attr(name, field, type) \
> > -static ssize_t name##_##field##_show(struct device *dev, \
> > +#define declare_gb_loopback_ro_stats_attr(name, field, type) \
> > +static ssize_t name##_##field##_show(struct device *dev, \
> > struct device_attribute *attr, \
> > char *buf) \
> > { \
> > @@ -142,8 +142,8 @@ static ssize_t name##_##field##_show(struct device *dev, \
> > } \
> > static DEVICE_ATTR_RO(name##_##field)
> > -#define gb_loopback_ro_avg_attr(name) \
> > -static ssize_t name##_avg_show(struct device *dev, \
> > +#define declare_gb_loopback_ro_avg_attr(name) \
> > +static ssize_t name##_avg_show(struct device *dev, \
> > struct device_attribute *attr, \
> > char *buf) \
> > { \
> > @@ -151,8 +151,8 @@ static ssize_t name##_avg_show(struct device *dev, \
> > struct gb_loopback *gb; \
> > u64 avg, rem; \
> > u32 count; \
> > - gb = dev_get_drvdata(dev); \
> > - stats = &gb->name; \
> > + gb = dev_get_drvdata(dev); \
> > + stats = &gb->name; \
> > count = stats->count ? stats->count : 1; \
> > avg = stats->sum + count / 2000000; /* round closest */ \
> > rem = do_div(avg, count); \
> > @@ -162,12 +162,12 @@ 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 declare_gb_loopback_stats_attrs(field) \
> > + declare_gb_loopback_ro_stats_attr(field, min, u); \
> > + declare_gb_loopback_ro_stats_attr(field, max, u); \
> > + declare_gb_loopback_ro_avg_attr(field)
> > -#define gb_loopback_attr(field, type) \
> > +#define declare_gb_loopback_attr(field, type) \
> > static ssize_t field##_show(struct device *dev, \
> > struct device_attribute *attr, \
> > char *buf) \
> > @@ -193,8 +193,8 @@ static ssize_t field##_store(struct device *dev, \
> > } \
> > static DEVICE_ATTR_RW(field)
> > -#define gb_dev_loopback_ro_attr(field, conn) \
> > -static ssize_t field##_show(struct device *dev, \
> > +#define declare_gb_dev_loopback_ro_attr(field, conn) \
> > +static ssize_t field##_show(struct device *dev, \
> > struct device_attribute *attr, \
> > char *buf) \
> > { \
> > @@ -203,7 +203,7 @@ static ssize_t field##_show(struct device *dev, \
> > } \
> > static DEVICE_ATTR_RO(field)
> > -#define gb_dev_loopback_rw_attr(field, type) \
> > +#define declare_gb_dev_loopback_rw_attr(field, type) \
> > static ssize_t field##_show(struct device *dev, \
> > struct device_attribute *attr, \
> > char *buf) \
> > @@ -223,7 +223,7 @@ static ssize_t field##_store(struct device *dev, \
> > if (ret != 1) \
> > len = -EINVAL; \
> > else \
> > - gb_loopback_check_attr(gb); \
> > + gb_loopback_check_attr(gb); \
> > mutex_unlock(&gb->mutex); \
> > return len; \
> > } \
> > @@ -268,26 +268,26 @@ static void gb_loopback_check_attr(struct gb_loopback *gb)
> > }
> > /* Time to send and receive one message */
> > -gb_loopback_stats_attrs(latency);
> > +declare_gb_loopback_stats_attrs(latency);
> > /* Number of requests sent per second on this cport */
> > -gb_loopback_stats_attrs(requests_per_second);
> > +declare_gb_loopback_stats_attrs(requests_per_second);
> > /* Quantity of data sent and received on this cport */
> > -gb_loopback_stats_attrs(throughput);
> > +declare_gb_loopback_stats_attrs(throughput);
> > /* Latency across the UniPro link from APBridge's perspective */
> > -gb_loopback_stats_attrs(apbridge_unipro_latency);
> > +declare_gb_loopback_stats_attrs(apbridge_unipro_latency);
> > /* Firmware induced overhead in the GPBridge */
> > -gb_loopback_stats_attrs(gbphy_firmware_latency);
> > +declare_gb_loopback_stats_attrs(gbphy_firmware_latency);
> > /* Number of errors encountered during loop */
> > -gb_loopback_ro_attr(error);
> > +declare_gb_loopback_ro_attr(error);
> > /* Number of requests successfully completed async */
> > -gb_loopback_ro_attr(requests_completed);
> > +declare_gb_loopback_ro_attr(requests_completed);
> > /* Number of requests timed out async */
> > -gb_loopback_ro_attr(requests_timedout);
> > +declare_gb_loopback_ro_attr(requests_timedout);
> > /* Timeout minimum in useconds */
> > -gb_loopback_ro_attr(timeout_min);
> > +declare_gb_loopback_ro_attr(timeout_min);
> > /* Timeout minimum in useconds */
> > -gb_loopback_ro_attr(timeout_max);
> > +declare_gb_loopback_ro_attr(timeout_max);
> > /*
> > * Type of loopback message to send based on protocol type definitions
> > @@ -297,21 +297,21 @@ gb_loopback_ro_attr(timeout_max);
> > * payload returned in response)
> > * 4 => Send a sink message (message with payload, no payload in response)
> > */
> > -gb_dev_loopback_rw_attr(type, d);
> > +declare_gb_dev_loopback_rw_attr(type, d);
> > /* Size of transfer message payload: 0-4096 bytes */
> > -gb_dev_loopback_rw_attr(size, u);
> > +declare_gb_dev_loopback_rw_attr(size, u);
> > /* Time to wait between two messages: 0-1000 ms */
> > -gb_dev_loopback_rw_attr(us_wait, d);
> > +declare_gb_dev_loopback_rw_attr(us_wait, d);
> > /* Maximum iterations for a given operation: 1-(2^32-1), 0 implies infinite */
> > -gb_dev_loopback_rw_attr(iteration_max, u);
> > +declare_gb_dev_loopback_rw_attr(iteration_max, u);
> > /* The current index of the for (i = 0; i < iteration_max; i++) loop */
> > -gb_dev_loopback_ro_attr(iteration_count, false);
> > +declare_gb_dev_loopback_ro_attr(iteration_count, false);
> > /* A flag to indicate synchronous or asynchronous operations */
> > -gb_dev_loopback_rw_attr(async, u);
> > +declare_gb_dev_loopback_rw_attr(async, u);
> > /* Timeout of an individual asynchronous request */
> > -gb_dev_loopback_rw_attr(timeout, u);
> > +declare_gb_dev_loopback_rw_attr(timeout, u);
> > /* Maximum number of in-flight operations before back-off */
> > -gb_dev_loopback_rw_attr(outstanding_operations_max, u);
> > +declare_gb_dev_loopback_rw_attr(outstanding_operations_max, u);
> > static struct attribute *loopback_attrs[] = {
> > &dev_attr_latency_min.attr,
> >
>
On Fri, May 14, 2021 at 07:53:57PM +0530, Shreyansh Chouhan wrote:
> On Fri, May 14, 2021 at 04:05:32PM +0200, Greg KH wrote:
> > On Fri, May 14, 2021 at 07:18:38PM +0530, Shreyansh Chouhan wrote:
> > > On Fri, May 14, 2021 at 03:36:25PM +0200, Greg KH wrote:
> > > > On Fri, May 14, 2021 at 07:00:39PM +0530, Shreyansh Chouhan wrote:
> > > > > The gb_loopback_stats_attrs macro, (defined in loopback.c,) is a
> > > > > multiline macro whose statements were not enclosed in a do while
> > > > > loop.
> > > > >
> > > > > This patch adds a do while loop around the statements of the said
> > > > > macro.
> > > > >
> > > > > Signed-off-by: Shreyansh Chouhan <[email protected]>
> > > > > ---
> > > > > drivers/staging/greybus/loopback.c | 10 ++++++----
> > > > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> > > > > index 2471448ba42a..c88ef3e894fa 100644
> > > > > --- a/drivers/staging/greybus/loopback.c
> > > > > +++ b/drivers/staging/greybus/loopback.c
> > > > > @@ -162,10 +162,12 @@ 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_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); \
> > > > > + } while (0)
> > > > >
> > > > > #define gb_loopback_attr(field, type) \
> > > > > static ssize_t field##_show(struct device *dev, \
> > > > > --
> > > > > 2.31.1
> > > > >
> > > > >
> > > >
> > > > Did you test build this change?
> > >
> > > I built the module using make -C . M=drivers/staging/greybus to test
> > > build it. I didn't get any errors.
> >
> > Really? Can you provide the full build output for this file with your
> > change? I don't think you really built this file for the obvious
> > reasons...
>
> I ran make -C . M=drivers/staging/greybus
>
> I got a three line output saying:
> make: Entering directory '/work/linux'
> MODPOST drivers/staging/greybus//Module.symvers
> make: Leaving directory '/work/linux'
>
> I just tried rebuilding the kernel with CONFIG_GREYBUS=m, and now I can
> see what you are talking about. Why weren't these errors reported when I
> ran the previous make command? Does that too check for the config
> variables even when I specifically asked it to build a module?
You were just asking it to build a subdirectory, not a specific
individual file, and when you do that it looks at the configuration
settings.
It's always good to ensure that you actually build the files you modify
before sending patches out.
thanks,
greg k-h
On Fri, May 14, 2021 at 03:36:25PM +0200, Greg KH wrote:
> On Fri, May 14, 2021 at 07:00:39PM +0530, Shreyansh Chouhan wrote:
> > The gb_loopback_stats_attrs macro, (defined in loopback.c,) is a
> > multiline macro whose statements were not enclosed in a do while
> > loop.
> >
> > This patch adds a do while loop around the statements of the said
> > macro.
> >
> > Signed-off-by: Shreyansh Chouhan <[email protected]>
> > ---
> > drivers/staging/greybus/loopback.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> > index 2471448ba42a..c88ef3e894fa 100644
> > --- a/drivers/staging/greybus/loopback.c
> > +++ b/drivers/staging/greybus/loopback.c
> > @@ -162,10 +162,12 @@ 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_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); \
> > + } while (0)
> >
> > #define gb_loopback_attr(field, type) \
> > static ssize_t field##_show(struct device *dev, \
> > --
> > 2.31.1
> >
> >
>
> Did you test build this change?
I built the module using make -C . M=drivers/staging/greybus to test
build it. I didn't get any errors.
Regards,
Shreyansh Chouhan
On Fri, 2021-05-14 at 17:30 +0200, Greg KH wrote:
> On Fri, May 14, 2021 at 08:42:16PM +0530, Shreyansh Chouhan wrote:
[]
> > I didn't look at how/where was the macro called and missed a very
> > obvious error. Now that I have looked at it, the only way I can think of
> > fixing this is changing the macro to a (inline?) function. Will
> > that be a desirable change?
>
> No, it can't be a function, the code is fine as-is, checkpatch is just a
> perl script and does not always know what needs to be done.
true.
perhaps better though to rename these declaring macros to start with declare_
Something like this:
(with miscellaneous realigning of the macros line ending continuations \)
---
drivers/staging/greybus/loopback.c | 72 +++++++++++++++++++-------------------
1 file changed, 36 insertions(+), 36 deletions(-)
diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
index 2471448ba42a..dc399792f35f 100644
--- a/drivers/staging/greybus/loopback.c
+++ b/drivers/staging/greybus/loopback.c
@@ -119,18 +119,18 @@ module_param(kfifo_depth, uint, 0444);
#define GB_LOOPBACK_US_WAIT_MAX 1000000
/* interface sysfs attributes */
-#define gb_loopback_ro_attr(field) \
-static ssize_t field##_show(struct device *dev, \
+#define declare_gb_loopback_ro_attr(field) \
+static ssize_t field##_show(struct device *dev, \
struct device_attribute *attr, \
char *buf) \
{ \
struct gb_loopback *gb = dev_get_drvdata(dev); \
- return sprintf(buf, "%u\n", gb->field); \
+ return sprintf(buf, "%u\n", gb->field); \
} \
static DEVICE_ATTR_RO(field)
-#define gb_loopback_ro_stats_attr(name, field, type) \
-static ssize_t name##_##field##_show(struct device *dev, \
+#define declare_gb_loopback_ro_stats_attr(name, field, type) \
+static ssize_t name##_##field##_show(struct device *dev, \
struct device_attribute *attr, \
char *buf) \
{ \
@@ -142,8 +142,8 @@ static ssize_t name##_##field##_show(struct device *dev, \
} \
static DEVICE_ATTR_RO(name##_##field)
-#define gb_loopback_ro_avg_attr(name) \
-static ssize_t name##_avg_show(struct device *dev, \
+#define declare_gb_loopback_ro_avg_attr(name) \
+static ssize_t name##_avg_show(struct device *dev, \
struct device_attribute *attr, \
char *buf) \
{ \
@@ -151,8 +151,8 @@ static ssize_t name##_avg_show(struct device *dev, \
struct gb_loopback *gb; \
u64 avg, rem; \
u32 count; \
- gb = dev_get_drvdata(dev); \
- stats = &gb->name; \
+ gb = dev_get_drvdata(dev); \
+ stats = &gb->name; \
count = stats->count ? stats->count : 1; \
avg = stats->sum + count / 2000000; /* round closest */ \
rem = do_div(avg, count); \
@@ -162,12 +162,12 @@ 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 declare_gb_loopback_stats_attrs(field) \
+ declare_gb_loopback_ro_stats_attr(field, min, u); \
+ declare_gb_loopback_ro_stats_attr(field, max, u); \
+ declare_gb_loopback_ro_avg_attr(field)
-#define gb_loopback_attr(field, type) \
+#define declare_gb_loopback_attr(field, type) \
static ssize_t field##_show(struct device *dev, \
struct device_attribute *attr, \
char *buf) \
@@ -193,8 +193,8 @@ static ssize_t field##_store(struct device *dev, \
} \
static DEVICE_ATTR_RW(field)
-#define gb_dev_loopback_ro_attr(field, conn) \
-static ssize_t field##_show(struct device *dev, \
+#define declare_gb_dev_loopback_ro_attr(field, conn) \
+static ssize_t field##_show(struct device *dev, \
struct device_attribute *attr, \
char *buf) \
{ \
@@ -203,7 +203,7 @@ static ssize_t field##_show(struct device *dev, \
} \
static DEVICE_ATTR_RO(field)
-#define gb_dev_loopback_rw_attr(field, type) \
+#define declare_gb_dev_loopback_rw_attr(field, type) \
static ssize_t field##_show(struct device *dev, \
struct device_attribute *attr, \
char *buf) \
@@ -223,7 +223,7 @@ static ssize_t field##_store(struct device *dev, \
if (ret != 1) \
len = -EINVAL; \
else \
- gb_loopback_check_attr(gb); \
+ gb_loopback_check_attr(gb); \
mutex_unlock(&gb->mutex); \
return len; \
} \
@@ -268,26 +268,26 @@ static void gb_loopback_check_attr(struct gb_loopback *gb)
}
/* Time to send and receive one message */
-gb_loopback_stats_attrs(latency);
+declare_gb_loopback_stats_attrs(latency);
/* Number of requests sent per second on this cport */
-gb_loopback_stats_attrs(requests_per_second);
+declare_gb_loopback_stats_attrs(requests_per_second);
/* Quantity of data sent and received on this cport */
-gb_loopback_stats_attrs(throughput);
+declare_gb_loopback_stats_attrs(throughput);
/* Latency across the UniPro link from APBridge's perspective */
-gb_loopback_stats_attrs(apbridge_unipro_latency);
+declare_gb_loopback_stats_attrs(apbridge_unipro_latency);
/* Firmware induced overhead in the GPBridge */
-gb_loopback_stats_attrs(gbphy_firmware_latency);
+declare_gb_loopback_stats_attrs(gbphy_firmware_latency);
/* Number of errors encountered during loop */
-gb_loopback_ro_attr(error);
+declare_gb_loopback_ro_attr(error);
/* Number of requests successfully completed async */
-gb_loopback_ro_attr(requests_completed);
+declare_gb_loopback_ro_attr(requests_completed);
/* Number of requests timed out async */
-gb_loopback_ro_attr(requests_timedout);
+declare_gb_loopback_ro_attr(requests_timedout);
/* Timeout minimum in useconds */
-gb_loopback_ro_attr(timeout_min);
+declare_gb_loopback_ro_attr(timeout_min);
/* Timeout minimum in useconds */
-gb_loopback_ro_attr(timeout_max);
+declare_gb_loopback_ro_attr(timeout_max);
/*
* Type of loopback message to send based on protocol type definitions
@@ -297,21 +297,21 @@ gb_loopback_ro_attr(timeout_max);
* payload returned in response)
* 4 => Send a sink message (message with payload, no payload in response)
*/
-gb_dev_loopback_rw_attr(type, d);
+declare_gb_dev_loopback_rw_attr(type, d);
/* Size of transfer message payload: 0-4096 bytes */
-gb_dev_loopback_rw_attr(size, u);
+declare_gb_dev_loopback_rw_attr(size, u);
/* Time to wait between two messages: 0-1000 ms */
-gb_dev_loopback_rw_attr(us_wait, d);
+declare_gb_dev_loopback_rw_attr(us_wait, d);
/* Maximum iterations for a given operation: 1-(2^32-1), 0 implies infinite */
-gb_dev_loopback_rw_attr(iteration_max, u);
+declare_gb_dev_loopback_rw_attr(iteration_max, u);
/* The current index of the for (i = 0; i < iteration_max; i++) loop */
-gb_dev_loopback_ro_attr(iteration_count, false);
+declare_gb_dev_loopback_ro_attr(iteration_count, false);
/* A flag to indicate synchronous or asynchronous operations */
-gb_dev_loopback_rw_attr(async, u);
+declare_gb_dev_loopback_rw_attr(async, u);
/* Timeout of an individual asynchronous request */
-gb_dev_loopback_rw_attr(timeout, u);
+declare_gb_dev_loopback_rw_attr(timeout, u);
/* Maximum number of in-flight operations before back-off */
-gb_dev_loopback_rw_attr(outstanding_operations_max, u);
+declare_gb_dev_loopback_rw_attr(outstanding_operations_max, u);
static struct attribute *loopback_attrs[] = {
&dev_attr_latency_min.attr,
On Fri, May 14, 2021 at 08:56:10AM -0700, Joe Perches wrote:
> On Fri, 2021-05-14 at 17:30 +0200, Greg KH wrote:
> > On Fri, May 14, 2021 at 08:42:16PM +0530, Shreyansh Chouhan wrote:
> []
> > > I didn't look at how/where was the macro called and missed a very
> > > obvious error. Now that I have looked at it, the only way I can think of
> > > fixing this is changing the macro to a (inline?) function. Will
> > > that be a desirable change?
> >
> > No, it can't be a function, the code is fine as-is, checkpatch is just a
> > perl script and does not always know what needs to be done.
>
> true.
>
> perhaps better though to rename these declaring macros to start with declare_
>
> Something like this:
Thanks! I can definitely help with this. I'll send the patch in a bit.
Since it seemed as if the original patch wasn't really required, I canceled
the moderator approval request for the greybus-dev mailing list. I
thought I'd rather not bother the moderator and the rest of the list
with these patches. I thought I should mention this since it could cause
some confusion. I will start a new thread for the other patch.
Regards,
Shreyansh Chouhan
On Fri, May 14, 2021 at 08:56:10AM -0700, Joe Perches wrote:
> On Fri, 2021-05-14 at 17:30 +0200, Greg KH wrote:
> > On Fri, May 14, 2021 at 08:42:16PM +0530, Shreyansh Chouhan wrote:
> []
> > > I didn't look at how/where was the macro called and missed a very
> > > obvious error. Now that I have looked at it, the only way I can think of
> > > fixing this is changing the macro to a (inline?) function. Will
> > > that be a desirable change?
> >
> > No, it can't be a function, the code is fine as-is, checkpatch is just a
> > perl script and does not always know what needs to be done.
>
> true.
>
> perhaps better though to rename these declaring macros to start with declare_
>
> Something like this:
>
Can I mention you in the 'Suggested-by' tag for the commit? (Since you
suggested the idea for this patch.)
Regards,
Shreyansh Chouhan
On 5/14/21 10:56 AM, Joe Perches wrote:
> On Fri, 2021-05-14 at 17:30 +0200, Greg KH wrote:
>> On Fri, May 14, 2021 at 08:42:16PM +0530, Shreyansh Chouhan wrote:
> []
>>> I didn't look at how/where was the macro called and missed a very
>>> obvious error. Now that I have looked at it, the only way I can think of
>>> fixing this is changing the macro to a (inline?) function. Will
>>> that be a desirable change?
>>
>> No, it can't be a function, the code is fine as-is, checkpatch is just a
>> perl script and does not always know what needs to be done.
>
> true.
>
> perhaps better though to rename these declaring macros to start with declare_
I don't disagree with your suggestion, but it's not clear it
would have prevented submission of the erroneous initial patch
(nor future ones from people who blindly follow checkpatch.pl
suggestions).
-Alex
PS Lots of negatives in that sentence.
> Something like this:
> (with miscellaneous realigning of the macros line ending continuations \)
> ---
> drivers/staging/greybus/loopback.c | 72 +++++++++++++++++++-------------------
> 1 file changed, 36 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> index 2471448ba42a..dc399792f35f 100644
> --- a/drivers/staging/greybus/loopback.c
> +++ b/drivers/staging/greybus/loopback.c
> @@ -119,18 +119,18 @@ module_param(kfifo_depth, uint, 0444);
> #define GB_LOOPBACK_US_WAIT_MAX 1000000
>
> /* interface sysfs attributes */
> -#define gb_loopback_ro_attr(field) \
> -static ssize_t field##_show(struct device *dev, \
> +#define declare_gb_loopback_ro_attr(field) \
> +static ssize_t field##_show(struct device *dev, \
> struct device_attribute *attr, \
> char *buf) \
> { \
> struct gb_loopback *gb = dev_get_drvdata(dev); \
> - return sprintf(buf, "%u\n", gb->field); \
> + return sprintf(buf, "%u\n", gb->field); \
> } \
> static DEVICE_ATTR_RO(field)
>
> -#define gb_loopback_ro_stats_attr(name, field, type) \
> -static ssize_t name##_##field##_show(struct device *dev, \
> +#define declare_gb_loopback_ro_stats_attr(name, field, type) \
> +static ssize_t name##_##field##_show(struct device *dev, \
> struct device_attribute *attr, \
> char *buf) \
> { \
> @@ -142,8 +142,8 @@ static ssize_t name##_##field##_show(struct device *dev, \
> } \
> static DEVICE_ATTR_RO(name##_##field)
>
> -#define gb_loopback_ro_avg_attr(name) \
> -static ssize_t name##_avg_show(struct device *dev, \
> +#define declare_gb_loopback_ro_avg_attr(name) \
> +static ssize_t name##_avg_show(struct device *dev, \
> struct device_attribute *attr, \
> char *buf) \
> { \
> @@ -151,8 +151,8 @@ static ssize_t name##_avg_show(struct device *dev, \
> struct gb_loopback *gb; \
> u64 avg, rem; \
> u32 count; \
> - gb = dev_get_drvdata(dev); \
> - stats = &gb->name; \
> + gb = dev_get_drvdata(dev); \
> + stats = &gb->name; \
> count = stats->count ? stats->count : 1; \
> avg = stats->sum + count / 2000000; /* round closest */ \
> rem = do_div(avg, count); \
> @@ -162,12 +162,12 @@ 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 declare_gb_loopback_stats_attrs(field) \
> + declare_gb_loopback_ro_stats_attr(field, min, u); \
> + declare_gb_loopback_ro_stats_attr(field, max, u); \
> + declare_gb_loopback_ro_avg_attr(field)
>
> -#define gb_loopback_attr(field, type) \
> +#define declare_gb_loopback_attr(field, type) \
> static ssize_t field##_show(struct device *dev, \
> struct device_attribute *attr, \
> char *buf) \
> @@ -193,8 +193,8 @@ static ssize_t field##_store(struct device *dev, \
> } \
> static DEVICE_ATTR_RW(field)
>
> -#define gb_dev_loopback_ro_attr(field, conn) \
> -static ssize_t field##_show(struct device *dev, \
> +#define declare_gb_dev_loopback_ro_attr(field, conn) \
> +static ssize_t field##_show(struct device *dev, \
> struct device_attribute *attr, \
> char *buf) \
> { \
> @@ -203,7 +203,7 @@ static ssize_t field##_show(struct device *dev, \
> } \
> static DEVICE_ATTR_RO(field)
>
> -#define gb_dev_loopback_rw_attr(field, type) \
> +#define declare_gb_dev_loopback_rw_attr(field, type) \
> static ssize_t field##_show(struct device *dev, \
> struct device_attribute *attr, \
> char *buf) \
> @@ -223,7 +223,7 @@ static ssize_t field##_store(struct device *dev, \
> if (ret != 1) \
> len = -EINVAL; \
> else \
> - gb_loopback_check_attr(gb); \
> + gb_loopback_check_attr(gb); \
> mutex_unlock(&gb->mutex); \
> return len; \
> } \
> @@ -268,26 +268,26 @@ static void gb_loopback_check_attr(struct gb_loopback *gb)
> }
>
> /* Time to send and receive one message */
> -gb_loopback_stats_attrs(latency);
> +declare_gb_loopback_stats_attrs(latency);
> /* Number of requests sent per second on this cport */
> -gb_loopback_stats_attrs(requests_per_second);
> +declare_gb_loopback_stats_attrs(requests_per_second);
> /* Quantity of data sent and received on this cport */
> -gb_loopback_stats_attrs(throughput);
> +declare_gb_loopback_stats_attrs(throughput);
> /* Latency across the UniPro link from APBridge's perspective */
> -gb_loopback_stats_attrs(apbridge_unipro_latency);
> +declare_gb_loopback_stats_attrs(apbridge_unipro_latency);
> /* Firmware induced overhead in the GPBridge */
> -gb_loopback_stats_attrs(gbphy_firmware_latency);
> +declare_gb_loopback_stats_attrs(gbphy_firmware_latency);
>
> /* Number of errors encountered during loop */
> -gb_loopback_ro_attr(error);
> +declare_gb_loopback_ro_attr(error);
> /* Number of requests successfully completed async */
> -gb_loopback_ro_attr(requests_completed);
> +declare_gb_loopback_ro_attr(requests_completed);
> /* Number of requests timed out async */
> -gb_loopback_ro_attr(requests_timedout);
> +declare_gb_loopback_ro_attr(requests_timedout);
> /* Timeout minimum in useconds */
> -gb_loopback_ro_attr(timeout_min);
> +declare_gb_loopback_ro_attr(timeout_min);
> /* Timeout minimum in useconds */
> -gb_loopback_ro_attr(timeout_max);
> +declare_gb_loopback_ro_attr(timeout_max);
>
> /*
> * Type of loopback message to send based on protocol type definitions
> @@ -297,21 +297,21 @@ gb_loopback_ro_attr(timeout_max);
> * payload returned in response)
> * 4 => Send a sink message (message with payload, no payload in response)
> */
> -gb_dev_loopback_rw_attr(type, d);
> +declare_gb_dev_loopback_rw_attr(type, d);
> /* Size of transfer message payload: 0-4096 bytes */
> -gb_dev_loopback_rw_attr(size, u);
> +declare_gb_dev_loopback_rw_attr(size, u);
> /* Time to wait between two messages: 0-1000 ms */
> -gb_dev_loopback_rw_attr(us_wait, d);
> +declare_gb_dev_loopback_rw_attr(us_wait, d);
> /* Maximum iterations for a given operation: 1-(2^32-1), 0 implies infinite */
> -gb_dev_loopback_rw_attr(iteration_max, u);
> +declare_gb_dev_loopback_rw_attr(iteration_max, u);
> /* The current index of the for (i = 0; i < iteration_max; i++) loop */
> -gb_dev_loopback_ro_attr(iteration_count, false);
> +declare_gb_dev_loopback_ro_attr(iteration_count, false);
> /* A flag to indicate synchronous or asynchronous operations */
> -gb_dev_loopback_rw_attr(async, u);
> +declare_gb_dev_loopback_rw_attr(async, u);
> /* Timeout of an individual asynchronous request */
> -gb_dev_loopback_rw_attr(timeout, u);
> +declare_gb_dev_loopback_rw_attr(timeout, u);
> /* Maximum number of in-flight operations before back-off */
> -gb_dev_loopback_rw_attr(outstanding_operations_max, u);
> +declare_gb_dev_loopback_rw_attr(outstanding_operations_max, u);
>
> static struct attribute *loopback_attrs[] = {
> &dev_attr_latency_min.attr,
>
On Fri, 2021-05-14 at 13:53 -0500, Alex Elder wrote:
> On 5/14/21 10:56 AM, Joe Perches wrote:
> > On Fri, 2021-05-14 at 17:30 +0200, Greg KH wrote:
> > > On Fri, May 14, 2021 at 08:42:16PM +0530, Shreyansh Chouhan wrote:
> > []
> > > > I didn't look at how/where was the macro called and missed a very
> > > > obvious error. Now that I have looked at it, the only way I can think of
> > > > fixing this is changing the macro to a (inline?) function. Will
> > > > that be a desirable change?
> > >
> > > No, it can't be a function, the code is fine as-is, checkpatch is just a
> > > perl script and does not always know what needs to be done.
> >
> > true.
> >
> > perhaps better though to rename these declaring macros to start with declare_
>
> I don't disagree with your suggestion, but it's not clear it
> would have prevented submission of the erroneous initial patch
> (nor future ones from people who blindly follow checkpatch.pl
> suggestions).
With my checkpatch maintainer hat on:
Yeah Alex, I know. checkpatch can't teach people c either.
There's not much to do other than try to make the code clearer.
Adding exceptions to checkpatch only leads to other exceptions
and false negatives...
> PS Lots of negatives in that sentence.
Only positives...
cheers, Joe
PS: Thought I should clarify why I think it might help. The macro
definition for gb_loopback_attr, calls other macros, but since these
macros are not in all caps, (I apologise if they follow some other macro
convention that I am not aware about,) it makes them look like function
calls. And I know it were lazy of me to not look at the individual macros
themselves, I think it would be a lot clearer if there was a way of knowing
that these calls are infact to other macros and not functions.
Adding declare_ as a prefix to all of them certainly helps, if only a
little, in that regards. Functions are not used for declaring things,
and since the gb_loopback_attr macro itself would have declare_
prepended to it, it would certainly give a hint to the fact that the
calls used in the definition might similarly be macros.
Once again, I do accept it was my fault that I sent the patch without
looking at those definitions more closely, and not considering that the
developer who wrote it must have had a reason for doing so. I just
wanted to clarify that renaming them might actually help.
Regards,
Shreyansh Chouhan