linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2][next] RDMA/hns: fix comparison of unsigned long variable 'end' with less than zero
@ 2019-05-31  9:21 Colin King
  2019-05-31  9:21 ` [PATCH 2/2][next] RDMA/hns: fix inverted logic of readl read and shift Colin King
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Colin King @ 2019-05-31  9:21 UTC (permalink / raw)
  To: Lijun Ou, Wei Hu, Doug Ledford, Jason Gunthorpe, linux-rdma
  Cc: kernel-janitors, linux-kernel

From: Colin Ian King <colin.king@canonical.com>

Currently the comparison of end with less than zero is always false
because end is an unsigned long.  Also, replace checks of end with
non-zero with end > 0 as it is possible that the #defined decrement
may be changed in the future causing end to step over zero and go
negative.

The initialization of end with 0 is also redundant as this value is
never read and is later set to HW_SYNC_TIMEOUT_MSECS, so fix this by
initializing it with this value to begin with.

Addresses-Coverity: ("Unsigned compared against 0")
Fixes: 669cefb654cb ("RDMA/hns: Remove jiffies operation in disable interrupt context")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/infiniband/hw/hns/hns_roce_hem.c   |  4 ++--
 drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_hem.c b/drivers/infiniband/hw/hns/hns_roce_hem.c
index 157c84a1f55f..b3641aeff27a 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hem.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hem.c
@@ -326,7 +326,7 @@ static int hns_roce_set_hem(struct hns_roce_dev *hr_dev,
 {
 	spinlock_t *lock = &hr_dev->bt_cmd_lock;
 	struct device *dev = hr_dev->dev;
-	unsigned long end = 0;
+	long end;
 	unsigned long flags;
 	struct hns_roce_hem_iter iter;
 	void __iomem *bt_cmd;
@@ -377,7 +377,7 @@ static int hns_roce_set_hem(struct hns_roce_dev *hr_dev,
 		bt_cmd = hr_dev->reg_base + ROCEE_BT_CMD_H_REG;
 
 		end = HW_SYNC_TIMEOUT_MSECS;
-		while (end) {
+		while (end > 0) {
 			if (!readl(bt_cmd) >> BT_CMD_SYNC_SHIFT)
 				break;
 
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
index 1fc77b1f2c6d..e13fea71bcb8 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
@@ -966,7 +966,7 @@ static int hns_roce_v1_recreate_lp_qp(struct hns_roce_dev *hr_dev)
 	struct hns_roce_free_mr *free_mr;
 	struct hns_roce_v1_priv *priv;
 	struct completion comp;
-	unsigned long end = HNS_ROCE_V1_RECREATE_LP_QP_TIMEOUT_MSECS;
+	long end = HNS_ROCE_V1_RECREATE_LP_QP_TIMEOUT_MSECS;
 
 	priv = (struct hns_roce_v1_priv *)hr_dev->priv;
 	free_mr = &priv->free_mr;
@@ -986,7 +986,7 @@ static int hns_roce_v1_recreate_lp_qp(struct hns_roce_dev *hr_dev)
 
 	queue_work(free_mr->free_mr_wq, &(lp_qp_work->work));
 
-	while (end) {
+	while (end > 0) {
 		if (try_wait_for_completion(&comp))
 			return 0;
 		msleep(HNS_ROCE_V1_RECREATE_LP_QP_WAIT_VALUE);
@@ -1104,7 +1104,7 @@ static int hns_roce_v1_dereg_mr(struct hns_roce_dev *hr_dev,
 	struct hns_roce_free_mr *free_mr;
 	struct hns_roce_v1_priv *priv;
 	struct completion comp;
-	unsigned long end = HNS_ROCE_V1_FREE_MR_TIMEOUT_MSECS;
+	long end = HNS_ROCE_V1_FREE_MR_TIMEOUT_MSECS;
 	unsigned long start = jiffies;
 	int npages;
 	int ret = 0;
@@ -1134,7 +1134,7 @@ static int hns_roce_v1_dereg_mr(struct hns_roce_dev *hr_dev,
 
 	queue_work(free_mr->free_mr_wq, &(mr_work->work));
 
-	while (end) {
+	while (end > 0) {
 		if (try_wait_for_completion(&comp))
 			goto free_mr;
 		msleep(HNS_ROCE_V1_FREE_MR_WAIT_VALUE);
@@ -2425,7 +2425,8 @@ static int hns_roce_v1_clear_hem(struct hns_roce_dev *hr_dev,
 {
 	struct device *dev = &hr_dev->pdev->dev;
 	struct hns_roce_v1_priv *priv;
-	unsigned long end = 0, flags = 0;
+	unsigned long flags = 0;
+	long end = HW_SYNC_TIMEOUT_MSECS;
 	__le32 bt_cmd_val[2] = {0};
 	void __iomem *bt_cmd;
 	u64 bt_ba = 0;
@@ -2463,7 +2464,6 @@ static int hns_roce_v1_clear_hem(struct hns_roce_dev *hr_dev,
 
 	bt_cmd = hr_dev->reg_base + ROCEE_BT_CMD_H_REG;
 
-	end = HW_SYNC_TIMEOUT_MSECS;
 	while (1) {
 		if (readl(bt_cmd) >> BT_CMD_SYNC_SHIFT) {
 			if (end < 0) {
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/2][next] RDMA/hns: fix inverted logic of readl read and shift
  2019-05-31  9:21 [PATCH 1/2][next] RDMA/hns: fix comparison of unsigned long variable 'end' with less than zero Colin King
@ 2019-05-31  9:21 ` Colin King
  2019-06-07 17:57   ` Jason Gunthorpe
  2019-06-07 17:57 ` [PATCH 1/2][next] RDMA/hns: fix comparison of unsigned long variable 'end' with less than zero Jason Gunthorpe
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 6+ messages in thread
From: Colin King @ 2019-05-31  9:21 UTC (permalink / raw)
  To: Lijun Ou, Wei Hu, Doug Ledford, Jason Gunthorpe, linux-rdma
  Cc: kernel-janitors, linux-kernel

From: Colin Ian King <colin.king@canonical.com>

A previous change incorrectly changed the inverted logic and logically
negated the readl rather than the shifted readl result. Fix this by
adding in missing parentheses around the expression that needs to be
logically negated.

Addresses-Coverity: ("Logically dead code")
Fixes: 669cefb654cb ("RDMA/hns: Remove jiffies operation in disable interrupt context")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/infiniband/hw/hns/hns_roce_hem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_hem.c b/drivers/infiniband/hw/hns/hns_roce_hem.c
index b3641aeff27a..a8e9329cbf4e 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hem.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hem.c
@@ -378,7 +378,7 @@ static int hns_roce_set_hem(struct hns_roce_dev *hr_dev,
 
 		end = HW_SYNC_TIMEOUT_MSECS;
 		while (end > 0) {
-			if (!readl(bt_cmd) >> BT_CMD_SYNC_SHIFT)
+			if (!(readl(bt_cmd) >> BT_CMD_SYNC_SHIFT))
 				break;
 
 			mdelay(HW_SYNC_SLEEP_TIME_INTERVAL);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2][next] RDMA/hns: fix inverted logic of readl read and shift
  2019-05-31  9:21 ` [PATCH 2/2][next] RDMA/hns: fix inverted logic of readl read and shift Colin King
@ 2019-06-07 17:57   ` Jason Gunthorpe
  0 siblings, 0 replies; 6+ messages in thread
From: Jason Gunthorpe @ 2019-06-07 17:57 UTC (permalink / raw)
  To: Colin King
  Cc: Lijun Ou, Wei Hu, Doug Ledford, linux-rdma, kernel-janitors,
	linux-kernel

On Fri, May 31, 2019 at 10:21:01AM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> A previous change incorrectly changed the inverted logic and logically
> negated the readl rather than the shifted readl result. Fix this by
> adding in missing parentheses around the expression that needs to be
> logically negated.
> 
> Addresses-Coverity: ("Logically dead code")
> Fixes: 669cefb654cb ("RDMA/hns: Remove jiffies operation in disable interrupt context")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/infiniband/hw/hns/hns_roce_hem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to for-next, thanks

Jason

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2][next] RDMA/hns: fix comparison of unsigned long variable 'end' with less than zero
  2019-05-31  9:21 [PATCH 1/2][next] RDMA/hns: fix comparison of unsigned long variable 'end' with less than zero Colin King
  2019-05-31  9:21 ` [PATCH 2/2][next] RDMA/hns: fix inverted logic of readl read and shift Colin King
@ 2019-06-07 17:57 ` Jason Gunthorpe
  2019-07-09 10:58 ` oulijun
  2019-07-24 19:31 ` Jason Gunthorpe
  3 siblings, 0 replies; 6+ messages in thread
From: Jason Gunthorpe @ 2019-06-07 17:57 UTC (permalink / raw)
  To: Colin King
  Cc: Lijun Ou, Wei Hu, Doug Ledford, linux-rdma, kernel-janitors,
	linux-kernel

On Fri, May 31, 2019 at 10:21:00AM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Currently the comparison of end with less than zero is always false
> because end is an unsigned long.  Also, replace checks of end with
> non-zero with end > 0 as it is possible that the #defined decrement
> may be changed in the future causing end to step over zero and go
> negative.
> 
> The initialization of end with 0 is also redundant as this value is
> never read and is later set to HW_SYNC_TIMEOUT_MSECS, so fix this by
> initializing it with this value to begin with.
> 
> Addresses-Coverity: ("Unsigned compared against 0")
> Fixes: 669cefb654cb ("RDMA/hns: Remove jiffies operation in disable interrupt context")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/infiniband/hw/hns/hns_roce_hem.c   |  4 ++--
>  drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 12 ++++++------
>  2 files changed, 8 insertions(+), 8 deletions(-)

hns team: Can you please comment on this patch

Jason

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2][next] RDMA/hns: fix comparison of unsigned long variable 'end' with less than zero
  2019-05-31  9:21 [PATCH 1/2][next] RDMA/hns: fix comparison of unsigned long variable 'end' with less than zero Colin King
  2019-05-31  9:21 ` [PATCH 2/2][next] RDMA/hns: fix inverted logic of readl read and shift Colin King
  2019-06-07 17:57 ` [PATCH 1/2][next] RDMA/hns: fix comparison of unsigned long variable 'end' with less than zero Jason Gunthorpe
@ 2019-07-09 10:58 ` oulijun
  2019-07-24 19:31 ` Jason Gunthorpe
  3 siblings, 0 replies; 6+ messages in thread
From: oulijun @ 2019-07-09 10:58 UTC (permalink / raw)
  To: Colin King, Wei Hu, Doug Ledford, Jason Gunthorpe, linux-rdma
  Cc: kernel-janitors, linux-kernel

在 2019/5/31 17:21, Colin King 写道:
> From: Colin Ian King <colin.king@canonical.com>
>
> Currently the comparison of end with less than zero is always false
> because end is an unsigned long.  Also, replace checks of end with
> non-zero with end > 0 as it is possible that the #defined decrement
> may be changed in the future causing end to step over zero and go
> negative.
>
> The initialization of end with 0 is also redundant as this value is
> never read and is later set to HW_SYNC_TIMEOUT_MSECS, so fix this by
> initializing it with this value to begin with.
>
> Addresses-Coverity: ("Unsigned compared against 0")
> Fixes: 669cefb654cb ("RDMA/hns: Remove jiffies operation in disable interrupt context")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/infiniband/hw/hns/hns_roce_hem.c   |  4 ++--
>  drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 12 ++++++------
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/infiniband/hw/hns/hns_roce_hem.c b/drivers/infiniband/hw/hns/hns_roce_hem.c
> index 157c84a1f55f..b3641aeff27a 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_hem.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_hem.c
> @@ -326,7 +326,7 @@ static int hns_roce_set_hem(struct hns_roce_dev *hr_dev,
>  {
>  	spinlock_t *lock = &hr_dev->bt_cmd_lock;
>  	struct device *dev = hr_dev->dev;
> -	unsigned long end = 0;
> +	long end;
>  	unsigned long flags;
>  	struct hns_roce_hem_iter iter;
>  	void __iomem *bt_cmd;
> @@ -377,7 +377,7 @@ static int hns_roce_set_hem(struct hns_roce_dev *hr_dev,
>  		bt_cmd = hr_dev->reg_base + ROCEE_BT_CMD_H_REG;
>  
>  		end = HW_SYNC_TIMEOUT_MSECS;
> -		while (end) {
> +		while (end > 0) {
>  			if (!readl(bt_cmd) >> BT_CMD_SYNC_SHIFT)
>  				break;
>  
> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
> index 1fc77b1f2c6d..e13fea71bcb8 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
> @@ -966,7 +966,7 @@ static int hns_roce_v1_recreate_lp_qp(struct hns_roce_dev *hr_dev)
>  	struct hns_roce_free_mr *free_mr;
>  	struct hns_roce_v1_priv *priv;
>  	struct completion comp;
> -	unsigned long end = HNS_ROCE_V1_RECREATE_LP_QP_TIMEOUT_MSECS;
> +	long end = HNS_ROCE_V1_RECREATE_LP_QP_TIMEOUT_MSECS;
>  
>  	priv = (struct hns_roce_v1_priv *)hr_dev->priv;
>  	free_mr = &priv->free_mr;
> @@ -986,7 +986,7 @@ static int hns_roce_v1_recreate_lp_qp(struct hns_roce_dev *hr_dev)
>  
>  	queue_work(free_mr->free_mr_wq, &(lp_qp_work->work));
>  
> -	while (end) {
> +	while (end > 0) {
>  		if (try_wait_for_completion(&comp))
>  			return 0;
>  		msleep(HNS_ROCE_V1_RECREATE_LP_QP_WAIT_VALUE);
> @@ -1104,7 +1104,7 @@ static int hns_roce_v1_dereg_mr(struct hns_roce_dev *hr_dev,
>  	struct hns_roce_free_mr *free_mr;
>  	struct hns_roce_v1_priv *priv;
>  	struct completion comp;
> -	unsigned long end = HNS_ROCE_V1_FREE_MR_TIMEOUT_MSECS;
> +	long end = HNS_ROCE_V1_FREE_MR_TIMEOUT_MSECS;
>  	unsigned long start = jiffies;
>  	int npages;
>  	int ret = 0;
> @@ -1134,7 +1134,7 @@ static int hns_roce_v1_dereg_mr(struct hns_roce_dev *hr_dev,
>  
>  	queue_work(free_mr->free_mr_wq, &(mr_work->work));
>  
> -	while (end) {
> +	while (end > 0) {
>  		if (try_wait_for_completion(&comp))
>  			goto free_mr;
>  		msleep(HNS_ROCE_V1_FREE_MR_WAIT_VALUE);
> @@ -2425,7 +2425,8 @@ static int hns_roce_v1_clear_hem(struct hns_roce_dev *hr_dev,
>  {
>  	struct device *dev = &hr_dev->pdev->dev;
>  	struct hns_roce_v1_priv *priv;
> -	unsigned long end = 0, flags = 0;
> +	unsigned long flags = 0;
> +	long end = HW_SYNC_TIMEOUT_MSECS;
>  	__le32 bt_cmd_val[2] = {0};
>  	void __iomem *bt_cmd;
>  	u64 bt_ba = 0;
> @@ -2463,7 +2464,6 @@ static int hns_roce_v1_clear_hem(struct hns_roce_dev *hr_dev,
>  
>  	bt_cmd = hr_dev->reg_base + ROCEE_BT_CMD_H_REG;
>  
> -	end = HW_SYNC_TIMEOUT_MSECS;
>  	while (1) {
>  		if (readl(bt_cmd) >> BT_CMD_SYNC_SHIFT) {
>  			if (end < 0) {

Good, thanks.



^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2][next] RDMA/hns: fix comparison of unsigned long variable 'end' with less than zero
  2019-05-31  9:21 [PATCH 1/2][next] RDMA/hns: fix comparison of unsigned long variable 'end' with less than zero Colin King
                   ` (2 preceding siblings ...)
  2019-07-09 10:58 ` oulijun
@ 2019-07-24 19:31 ` Jason Gunthorpe
  3 siblings, 0 replies; 6+ messages in thread
From: Jason Gunthorpe @ 2019-07-24 19:31 UTC (permalink / raw)
  To: Colin King
  Cc: Lijun Ou, Wei Hu, Doug Ledford, linux-rdma, kernel-janitors,
	linux-kernel

On Fri, May 31, 2019 at 10:21:00AM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Currently the comparison of end with less than zero is always false
> because end is an unsigned long.  Also, replace checks of end with
> non-zero with end > 0 as it is possible that the #defined decrement
> may be changed in the future causing end to step over zero and go
> negative.
> 
> The initialization of end with 0 is also redundant as this value is
> never read and is later set to HW_SYNC_TIMEOUT_MSECS, so fix this by
> initializing it with this value to begin with.
> 
> Addresses-Coverity: ("Unsigned compared against 0")
> Fixes: 669cefb654cb ("RDMA/hns: Remove jiffies operation in disable interrupt context")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/infiniband/hw/hns/hns_roce_hem.c   |  4 ++--
>  drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 12 ++++++------
>  2 files changed, 8 insertions(+), 8 deletions(-)

Applied to for-next, thanks

Jason

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-24 19:32 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-31  9:21 [PATCH 1/2][next] RDMA/hns: fix comparison of unsigned long variable 'end' with less than zero Colin King
2019-05-31  9:21 ` [PATCH 2/2][next] RDMA/hns: fix inverted logic of readl read and shift Colin King
2019-06-07 17:57   ` Jason Gunthorpe
2019-06-07 17:57 ` [PATCH 1/2][next] RDMA/hns: fix comparison of unsigned long variable 'end' with less than zero Jason Gunthorpe
2019-07-09 10:58 ` oulijun
2019-07-24 19:31 ` Jason Gunthorpe

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).