| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <YJ7Kbb5U71l8ukBD@fedora>
Date: Sat, 15 May 2021 00:37:25 +0530
From: Shreyansh Chouhan <chouhan.shreyansh630@...il.com>
To: Alex Elder <elder@...e.org>
Cc: Joe Perches <joe@...ches.com>,
Greg KH <gregkh@...uxfoundation.org>,
pure.logic@...us-software.ie, johan@...nel.org, elder@...nel.org,
greybus-dev@...ts.linaro.org, linux-staging@...ts.linux.dev,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: greybus: fix gb_loopback_stats_attrs definition
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,
> >
>
Powered by blists - more mailing lists