* [PATCH] IB/qib: remove redundant setting of any in for-loop
@ 2017-10-20 7:21 Colin King
2017-10-20 7:35 ` Joe Perches
0 siblings, 1 reply; 6+ messages in thread
From: Colin King @ 2017-10-20 7:21 UTC (permalink / raw)
To: Mike Marciniszyn, Doug Ledford, Sean Hefty, Hal Rosenstock, linux-rdma
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The variable all is being set but is never read after this
hence it can be removed from the for loop initialization.
Cleans up clang warning:
drivers/infiniband/hw/qib/qib_file_ops.c:640:7: warning: Value
stored to 'any' is never read
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/infiniband/hw/qib/qib_file_ops.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/infiniband/hw/qib/qib_file_ops.c b/drivers/infiniband/hw/qib/qib_file_ops.c
index 2d6a191afec0..b5c2e4223ee7 100644
--- a/drivers/infiniband/hw/qib/qib_file_ops.c
+++ b/drivers/infiniband/hw/qib/qib_file_ops.c
@@ -637,7 +637,7 @@ static int qib_set_part_key(struct qib_ctxtdata *rcd, u16 key)
ret = -EBUSY;
goto bail;
}
- for (any = i = 0; i < ARRAY_SIZE(ppd->pkeys); i++) {
+ for (i = 0; i < ARRAY_SIZE(ppd->pkeys); i++) {
if (!ppd->pkeys[i] &&
atomic_inc_return(&ppd->pkeyrefs[i]) == 1) {
rcd->pkeys[pidx] = key;
--
2.14.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] IB/qib: remove redundant setting of any in for-loop
2017-10-20 7:21 [PATCH] IB/qib: remove redundant setting of any in for-loop Colin King
@ 2017-10-20 7:35 ` Joe Perches
2017-11-10 18:00 ` Doug Ledford
0 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2017-10-20 7:35 UTC (permalink / raw)
To: Colin King, Mike Marciniszyn, Doug Ledford, Sean Hefty,
Hal Rosenstock, linux-rdma
Cc: kernel-janitors, linux-kernel
On Fri, 2017-10-20 at 09:21 +0200, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The variable all is being set but is never read after this
> hence it can be removed from the for loop initialization.
> Cleans up clang warning:
any is really used as bool and is initialized at function
entry. The earlier loop also reinitializes any unnecessarily.
> drivers/infiniband/hw/qib/qib_file_ops.c:640:7: warning: Value
> stored to 'any' is never read
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/infiniband/hw/qib/qib_file_ops.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/hw/qib/qib_file_ops.c b/drivers/infiniband/hw/qib/qib_file_ops.c
> index 2d6a191afec0..b5c2e4223ee7 100644
> --- a/drivers/infiniband/hw/qib/qib_file_ops.c
> +++ b/drivers/infiniband/hw/qib/qib_file_ops.c
> @@ -637,7 +637,7 @@ static int qib_set_part_key(struct qib_ctxtdata *rcd, u16 key)
> ret = -EBUSY;
> goto bail;
> }
> - for (any = i = 0; i < ARRAY_SIZE(ppd->pkeys); i++) {
> + for (i = 0; i < ARRAY_SIZE(ppd->pkeys); i++) {
> if (!ppd->pkeys[i] &&
> atomic_inc_return(&ppd->pkeyrefs[i]) == 1) {
> rcd->pkeys[pidx] = key;
Perhaps the function is better written without
the empty bail: label and without setting ret
and just using return.
Combining the int/bool conversion of any and the
direct returns seems clearer to me.
Something like:
---
drivers/infiniband/hw/qib/qib_file_ops.c | 70 ++++++++++++--------------------
1 file changed, 27 insertions(+), 43 deletions(-)
diff --git a/drivers/infiniband/hw/qib/qib_file_ops.c b/drivers/infiniband/hw/qib/qib_file_ops.c
index 2d6a191afec0..8078854e1cd6 100644
--- a/drivers/infiniband/hw/qib/qib_file_ops.c
+++ b/drivers/infiniband/hw/qib/qib_file_ops.c
@@ -568,20 +568,17 @@ static int qib_tid_free(struct qib_ctxtdata *rcd, unsigned subctxt,
static int qib_set_part_key(struct qib_ctxtdata *rcd, u16 key)
{
struct qib_pportdata *ppd = rcd->ppd;
- int i, any = 0, pidx = -1;
+ int i;
+ bool any = false;
+ int pidx = -1;
u16 lkey = key & 0x7FFF;
- int ret;
- if (lkey == (QIB_DEFAULT_P_KEY & 0x7FFF)) {
- /* nothing to do; this key always valid */
- ret = 0;
- goto bail;
- }
+ /* nothing to do; this key always valid */
+ if (lkey == (QIB_DEFAULT_P_KEY & 0x7FFF))
+ return 0;
- if (!lkey) {
- ret = -EINVAL;
- goto bail;
- }
+ if (!lkey)
+ return -EINVAL;
/*
* Set the full membership bit, because it has to be
@@ -594,18 +591,15 @@ static int qib_set_part_key(struct qib_ctxtdata *rcd, u16 key)
for (i = 0; i < ARRAY_SIZE(rcd->pkeys); i++) {
if (!rcd->pkeys[i] && pidx == -1)
pidx = i;
- if (rcd->pkeys[i] == key) {
- ret = -EEXIST;
- goto bail;
- }
- }
- if (pidx == -1) {
- ret = -EBUSY;
- goto bail;
+ if (rcd->pkeys[i] == key)
+ return -EEXIST;
}
- for (any = i = 0; i < ARRAY_SIZE(ppd->pkeys); i++) {
+ if (pidx == -1)
+ return -EBUSY;
+
+ for (i = 0; i < ARRAY_SIZE(ppd->pkeys); i++) {
if (!ppd->pkeys[i]) {
- any++;
+ any = true;
continue;
}
if (ppd->pkeys[i] == key) {
@@ -613,44 +607,34 @@ static int qib_set_part_key(struct qib_ctxtdata *rcd, u16 key)
if (atomic_inc_return(pkrefs) > 1) {
rcd->pkeys[pidx] = key;
- ret = 0;
- goto bail;
- } else {
- /*
- * lost race, decrement count, catch below
- */
- atomic_dec(pkrefs);
- any++;
+ return 0;
}
+ /* lost race, decrement count, catch below */
+ atomic_dec(pkrefs);
+ any = true;
}
- if ((ppd->pkeys[i] & 0x7FFF) == lkey) {
+ if ((ppd->pkeys[i] & 0x7FFF) == lkey)
/*
* It makes no sense to have both the limited and
* full membership PKEY set at the same time since
* the unlimited one will disable the limited one.
*/
- ret = -EEXIST;
- goto bail;
- }
- }
- if (!any) {
- ret = -EBUSY;
- goto bail;
+ return -EEXIST;
}
- for (any = i = 0; i < ARRAY_SIZE(ppd->pkeys); i++) {
+ if (!any)
+ return -EBUSY;
+
+ for (i = 0; i < ARRAY_SIZE(ppd->pkeys); i++) {
if (!ppd->pkeys[i] &&
atomic_inc_return(&ppd->pkeyrefs[i]) == 1) {
rcd->pkeys[pidx] = key;
ppd->pkeys[i] = key;
(void) ppd->dd->f_set_ib_cfg(ppd, QIB_IB_CFG_PKEYS, 0);
- ret = 0;
- goto bail;
+ return 0;
}
}
- ret = -EBUSY;
-bail:
- return ret;
+ return -EBUSY;
}
/**
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] IB/qib: remove redundant setting of any in for-loop
2017-10-20 7:35 ` Joe Perches
@ 2017-11-10 18:00 ` Doug Ledford
2017-11-10 19:07 ` Marciniszyn, Mike
0 siblings, 1 reply; 6+ messages in thread
From: Doug Ledford @ 2017-11-10 18:00 UTC (permalink / raw)
To: Joe Perches, Colin King, Mike Marciniszyn, Sean Hefty,
Hal Rosenstock, linux-rdma
Cc: kernel-janitors, linux-kernel, Dalessandro, Dennis
[-- Attachment #1: Type: text/plain, Size: 5092 bytes --]
On Fri, 2017-10-20 at 00:35 -0700, Joe Perches wrote:
> On Fri, 2017-10-20 at 09:21 +0200, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > The variable all is being set but is never read after this
> > hence it can be removed from the for loop initialization.
> > Cleans up clang warning:
>
> any is really used as bool and is initialized at function
> entry. The earlier loop also reinitializes any unnecessarily.
Denny, can you weigh in on what you want in this thread? Thanks.
> > drivers/infiniband/hw/qib/qib_file_ops.c:640:7: warning: Value
> > stored to 'any' is never read
> >
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> > drivers/infiniband/hw/qib/qib_file_ops.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/infiniband/hw/qib/qib_file_ops.c b/drivers/infiniband/hw/qib/qib_file_ops.c
> > index 2d6a191afec0..b5c2e4223ee7 100644
> > --- a/drivers/infiniband/hw/qib/qib_file_ops.c
> > +++ b/drivers/infiniband/hw/qib/qib_file_ops.c
> > @@ -637,7 +637,7 @@ static int qib_set_part_key(struct qib_ctxtdata *rcd, u16 key)
> > ret = -EBUSY;
> > goto bail;
> > }
> > - for (any = i = 0; i < ARRAY_SIZE(ppd->pkeys); i++) {
> > + for (i = 0; i < ARRAY_SIZE(ppd->pkeys); i++) {
> > if (!ppd->pkeys[i] &&
> > atomic_inc_return(&ppd->pkeyrefs[i]) == 1) {
> > rcd->pkeys[pidx] = key;
>
> Perhaps the function is better written without
> the empty bail: label and without setting ret
> and just using return.
>
> Combining the int/bool conversion of any and the
> direct returns seems clearer to me.
>
> Something like:
> ---
> drivers/infiniband/hw/qib/qib_file_ops.c | 70 ++++++++++++--------------------
> 1 file changed, 27 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/infiniband/hw/qib/qib_file_ops.c b/drivers/infiniband/hw/qib/qib_file_ops.c
> index 2d6a191afec0..8078854e1cd6 100644
> --- a/drivers/infiniband/hw/qib/qib_file_ops.c
> +++ b/drivers/infiniband/hw/qib/qib_file_ops.c
> @@ -568,20 +568,17 @@ static int qib_tid_free(struct qib_ctxtdata *rcd, unsigned subctxt,
> static int qib_set_part_key(struct qib_ctxtdata *rcd, u16 key)
> {
> struct qib_pportdata *ppd = rcd->ppd;
> - int i, any = 0, pidx = -1;
> + int i;
> + bool any = false;
> + int pidx = -1;
> u16 lkey = key & 0x7FFF;
> - int ret;
>
> - if (lkey == (QIB_DEFAULT_P_KEY & 0x7FFF)) {
> - /* nothing to do; this key always valid */
> - ret = 0;
> - goto bail;
> - }
> + /* nothing to do; this key always valid */
> + if (lkey == (QIB_DEFAULT_P_KEY & 0x7FFF))
> + return 0;
>
> - if (!lkey) {
> - ret = -EINVAL;
> - goto bail;
> - }
> + if (!lkey)
> + return -EINVAL;
>
> /*
> * Set the full membership bit, because it has to be
> @@ -594,18 +591,15 @@ static int qib_set_part_key(struct qib_ctxtdata *rcd, u16 key)
> for (i = 0; i < ARRAY_SIZE(rcd->pkeys); i++) {
> if (!rcd->pkeys[i] && pidx == -1)
> pidx = i;
> - if (rcd->pkeys[i] == key) {
> - ret = -EEXIST;
> - goto bail;
> - }
> - }
> - if (pidx == -1) {
> - ret = -EBUSY;
> - goto bail;
> + if (rcd->pkeys[i] == key)
> + return -EEXIST;
> }
> - for (any = i = 0; i < ARRAY_SIZE(ppd->pkeys); i++) {
> + if (pidx == -1)
> + return -EBUSY;
> +
> + for (i = 0; i < ARRAY_SIZE(ppd->pkeys); i++) {
> if (!ppd->pkeys[i]) {
> - any++;
> + any = true;
> continue;
> }
> if (ppd->pkeys[i] == key) {
> @@ -613,44 +607,34 @@ static int qib_set_part_key(struct qib_ctxtdata *rcd, u16 key)
>
> if (atomic_inc_return(pkrefs) > 1) {
> rcd->pkeys[pidx] = key;
> - ret = 0;
> - goto bail;
> - } else {
> - /*
> - * lost race, decrement count, catch below
> - */
> - atomic_dec(pkrefs);
> - any++;
> + return 0;
> }
> + /* lost race, decrement count, catch below */
> + atomic_dec(pkrefs);
> + any = true;
> }
> - if ((ppd->pkeys[i] & 0x7FFF) == lkey) {
> + if ((ppd->pkeys[i] & 0x7FFF) == lkey)
> /*
> * It makes no sense to have both the limited and
> * full membership PKEY set at the same time since
> * the unlimited one will disable the limited one.
> */
> - ret = -EEXIST;
> - goto bail;
> - }
> - }
> - if (!any) {
> - ret = -EBUSY;
> - goto bail;
> + return -EEXIST;
> }
> - for (any = i = 0; i < ARRAY_SIZE(ppd->pkeys); i++) {
> + if (!any)
> + return -EBUSY;
> +
> + for (i = 0; i < ARRAY_SIZE(ppd->pkeys); i++) {
> if (!ppd->pkeys[i] &&
> atomic_inc_return(&ppd->pkeyrefs[i]) == 1) {
> rcd->pkeys[pidx] = key;
> ppd->pkeys[i] = key;
> (void) ppd->dd->f_set_ib_cfg(ppd, QIB_IB_CFG_PKEYS, 0);
> - ret = 0;
> - goto bail;
> + return 0;
> }
> }
> - ret = -EBUSY;
>
> -bail:
> - return ret;
> + return -EBUSY;
> }
>
> /**
--
Doug Ledford <dledford@redhat.com>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] IB/qib: remove redundant setting of any in for-loop
2017-11-10 18:00 ` Doug Ledford
@ 2017-11-10 19:07 ` Marciniszyn, Mike
2017-11-13 21:52 ` Doug Ledford
0 siblings, 1 reply; 6+ messages in thread
From: Marciniszyn, Mike @ 2017-11-10 19:07 UTC (permalink / raw)
To: Doug Ledford, Joe Perches, Colin King, Hefty, Sean,
Hal Rosenstock, linux-rdma
Cc: kernel-janitors, linux-kernel, Dalessandro, Dennis
> > On Fri, 2017-10-20 at 09:21 +0200, Colin King wrote:
> > > From: Colin Ian King <colin.king@canonical.com>
> > >
> > > The variable all is being set but is never read after this
> > > hence it can be removed from the for loop initialization.
> > > Cleans up clang warning:
> >
> > any is really used as bool and is initialized at function
> > entry. The earlier loop also reinitializes any unnecessarily.
>
> Denny, can you weigh in on what you want in this thread? Thanks.
>
I am ok with both Colin's and Joe's patch.
Joe's patch would require additional testing vs. the trivial warning fix.
Then there is a "guideline" to keep fixes separate from clean up...
Doug, do you want a tested patch from me?
Mike
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] IB/qib: remove redundant setting of any in for-loop
2017-11-10 19:07 ` Marciniszyn, Mike
@ 2017-11-13 21:52 ` Doug Ledford
2017-11-14 21:40 ` Marciniszyn, Mike
0 siblings, 1 reply; 6+ messages in thread
From: Doug Ledford @ 2017-11-13 21:52 UTC (permalink / raw)
To: Marciniszyn, Mike, Joe Perches, Colin King, Hefty, Sean,
Hal Rosenstock, linux-rdma
Cc: kernel-janitors, linux-kernel, Dalessandro, Dennis
[-- Attachment #1: Type: text/plain, Size: 1011 bytes --]
On Fri, 2017-11-10 at 19:07 +0000, Marciniszyn, Mike wrote:
> > > On Fri, 2017-10-20 at 09:21 +0200, Colin King wrote:
> > > > From: Colin Ian King <colin.king@canonical.com>
> > > >
> > > > The variable all is being set but is never read after this
> > > > hence it can be removed from the for loop initialization.
> > > > Cleans up clang warning:
> > >
> > > any is really used as bool and is initialized at function
> > > entry. The earlier loop also reinitializes any unnecessarily.
> >
> > Denny, can you weigh in on what you want in this thread? Thanks.
> >
>
> I am ok with both Colin's and Joe's patch.
>
> Joe's patch would require additional testing vs. the trivial warning fix.
>
> Then there is a "guideline" to keep fixes separate from clean up...
>
> Doug, do you want a tested patch from me?
Preferably, yes :-)
--
Doug Ledford <dledford@redhat.com>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] IB/qib: remove redundant setting of any in for-loop
2017-11-13 21:52 ` Doug Ledford
@ 2017-11-14 21:40 ` Marciniszyn, Mike
0 siblings, 0 replies; 6+ messages in thread
From: Marciniszyn, Mike @ 2017-11-14 21:40 UTC (permalink / raw)
To: Doug Ledford, Joe Perches, Colin King, Hefty, Sean,
Hal Rosenstock, linux-rdma
Cc: kernel-janitors, linux-kernel, Dalessandro, Dennis
> > Doug, do you want a tested patch from me?
>
> Preferably, yes :-)
>
These patches are in a two patch series:
https://marc.info/?l=linux-rdma&m=151069497506923&w=2
The first is Colin's warning fix and the second is Joe's cleanup.
My value add is the testing. :)
They pass our psm testing.
Thanks for the patches!
Mike
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-11-14 21:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-20 7:21 [PATCH] IB/qib: remove redundant setting of any in for-loop Colin King
2017-10-20 7:35 ` Joe Perches
2017-11-10 18:00 ` Doug Ledford
2017-11-10 19:07 ` Marciniszyn, Mike
2017-11-13 21:52 ` Doug Ledford
2017-11-14 21:40 ` Marciniszyn, Mike
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).