* [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@vger.kernel.org Cc: kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org, 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@vger.kernel.org Cc: kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org, 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@vger.kernel.org Cc: kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org, 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