[PATCH 0/6] slimbus: sparse and CoverityScan fixes

[PATCH 0/6] slimbus: sparse and CoverityScan fixes

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Hi Greg, 

Here are few patches which are submitted in the list few days back,
these also fix some of the sparse errors reported.

Thanks,
Srini

Colin Ian King (3):
  slimbus: avoid null pointer dereference on msg
  slimbus: fix retries comparison to correctly identify failed
    allocation
  slimbus: make functions slim_ack_txn and slim_alloc_txbuf static

Wei Yongjun (3):
  slimbus: Use GFP_ATOMIC under spin lock
  slimbus: Fix missing unlock on error in slim_msg_response()
  slimbus: qcom: Fix return value check in qcom_slim_probe()

 drivers/slimbus/messaging.c |  8 +++++---
 drivers/slimbus/qcom-ctrl.c | 13 +++++++------
 2 files changed, 12 insertions(+), 9 deletions(-)

-- 
2.15.0

[PATCH 1/6] slimbus: avoid null pointer dereference on msg

[srinivas.kandagatla]

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

The pointer msg is checked to see if it is null at the start of
the function and jumps to the error exit label reterr that then
dereferences msg when it prints a dev_err error message. Avoid
this potential null pointer dereference by only printing the
error message if msg is not null.

Detected by CoverityScan, CID#1463141 ("Dereference after null check")

Signed-off-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/slimbus/messaging.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/slimbus/messaging.c b/drivers/slimbus/messaging.c
index 755462a4c75e..8b2c77f516b9 100644
--- a/drivers/slimbus/messaging.c
+++ b/drivers/slimbus/messaging.c
@@ -170,8 +170,9 @@ static int slim_val_inf_sanity(struct slim_controller *ctrl,
 		break;
 	}
 reterr:
-	dev_err(ctrl->dev, "Sanity check failed:msg:offset:0x%x, mc:%d\n",
-		msg->start_offset, mc);
+	if (msg)
+		dev_err(ctrl->dev, "Sanity check failed:msg:offset:0x%x, mc:%d\n",
+			msg->start_offset, mc);
 	return -EINVAL;
 }
 
-- 
2.15.0

[PATCH 1/6] slimbus: avoid null pointer dereference on msg

[srinivas.kandagatla]

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

The pointer msg is checked to see if it is null at the start of
the function and jumps to the error exit label reterr that then
dereferences msg when it prints a dev_err error message. Avoid
this potential null pointer dereference by only printing the
error message if msg is not null.

Detected by CoverityScan, CID#1463141 ("Dereference after null check")

Signed-off-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/slimbus/messaging.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/slimbus/messaging.c b/drivers/slimbus/messaging.c
index 755462a4c75e..8b2c77f516b9 100644
--- a/drivers/slimbus/messaging.c
+++ b/drivers/slimbus/messaging.c
@@ -170,8 +170,9 @@ static int slim_val_inf_sanity(struct slim_controller *ctrl,
 		break;
 	}
 reterr:
-	dev_err(ctrl->dev, "Sanity check failed:msg:offset:0x%x, mc:%d\n",
-		msg->start_offset, mc);
+	if (msg)
+		dev_err(ctrl->dev, "Sanity check failed:msg:offset:0x%x, mc:%d\n",
+			msg->start_offset, mc);
 	return -EINVAL;
 }
 
-- 
2.15.0

[PATCH 2/6] slimbus: fix retries comparison to correctly identify failed allocation

[srinivas.kandagatla]

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

Currently the check for too many retries fails because retries is actually
-1 when the retry loop terminates if no pbuf can be allocated because of
the post decrement on retries.  Fix this by not comparing retries with zero
but instead check if it is negative.

Detected by CoverityScan, CID#1463143 ("Logically dead code") and
CID#1463144 ("Dereference after null check")

Signed-off-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/slimbus/qcom-ctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/slimbus/qcom-ctrl.c b/drivers/slimbus/qcom-ctrl.c
index fb1a5e0eb8dd..2d67419a3c37 100644
--- a/drivers/slimbus/qcom-ctrl.c
+++ b/drivers/slimbus/qcom-ctrl.c
@@ -345,7 +345,7 @@ static int qcom_xfer_msg(struct slim_controller *sctrl,
 		}
 	}
 
-	if (!retries && !pbuf)
+	if (retries < 0 && !pbuf)
 		return -ENOMEM;
 
 	puc = (u8 *)pbuf;
-- 
2.15.0

[PATCH 3/6] slimbus: make functions slim_ack_txn and slim_alloc_txbuf static

[srinivas.kandagatla]

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

The functions slim_ack_txn and slim_alloc_txbuf are local to the
source and do not need to be in global scope, so make them static.

Cleans up sparse warnings:
symbol 'slim_ack_txn' was not declared. Should it be static?
symbol 'slim_alloc_txbuf' was not declared. Should it be static?

Signed-off-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/slimbus/qcom-ctrl.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/slimbus/qcom-ctrl.c b/drivers/slimbus/qcom-ctrl.c
index 2d67419a3c37..8d12baae4119 100644
--- a/drivers/slimbus/qcom-ctrl.c
+++ b/drivers/slimbus/qcom-ctrl.c
@@ -146,7 +146,7 @@ static void *slim_alloc_rxbuf(struct qcom_slim_ctrl *ctrl)
 	return ctrl->rx.base + (idx * ctrl->rx.sl_sz);
 }
 
-void slim_ack_txn(struct qcom_slim_ctrl *ctrl, int err)
+static void slim_ack_txn(struct qcom_slim_ctrl *ctrl, int err)
 {
 	struct completion *comp;
 	unsigned long flags;
@@ -299,8 +299,9 @@ static int qcom_clk_pause_wakeup(struct slim_controller *sctrl)
 	return 0;
 }
 
-void *slim_alloc_txbuf(struct qcom_slim_ctrl *ctrl, struct slim_msg_txn *txn,
-		       struct completion *done)
+static void *slim_alloc_txbuf(struct qcom_slim_ctrl *ctrl,
+			      struct slim_msg_txn *txn,
+			      struct completion *done)
 {
 	unsigned long flags;
 	int idx;
-- 
2.15.0

[PATCH 3/6] slimbus: make functions slim_ack_txn and slim_alloc_txbuf static

[srinivas.kandagatla]

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

The functions slim_ack_txn and slim_alloc_txbuf are local to the
source and do not need to be in global scope, so make them static.

Cleans up sparse warnings:
symbol 'slim_ack_txn' was not declared. Should it be static?
symbol 'slim_alloc_txbuf' was not declared. Should it be static?

Signed-off-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/slimbus/qcom-ctrl.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/slimbus/qcom-ctrl.c b/drivers/slimbus/qcom-ctrl.c
index 2d67419a3c37..8d12baae4119 100644
--- a/drivers/slimbus/qcom-ctrl.c
+++ b/drivers/slimbus/qcom-ctrl.c
@@ -146,7 +146,7 @@ static void *slim_alloc_rxbuf(struct qcom_slim_ctrl *ctrl)
 	return ctrl->rx.base + (idx * ctrl->rx.sl_sz);
 }
 
-void slim_ack_txn(struct qcom_slim_ctrl *ctrl, int err)
+static void slim_ack_txn(struct qcom_slim_ctrl *ctrl, int err)
 {
 	struct completion *comp;
 	unsigned long flags;
@@ -299,8 +299,9 @@ static int qcom_clk_pause_wakeup(struct slim_controller *sctrl)
 	return 0;
 }
 
-void *slim_alloc_txbuf(struct qcom_slim_ctrl *ctrl, struct slim_msg_txn *txn,
-		       struct completion *done)
+static void *slim_alloc_txbuf(struct qcom_slim_ctrl *ctrl,
+			      struct slim_msg_txn *txn,
+			      struct completion *done)
 {
 	unsigned long flags;
 	int idx;
-- 
2.15.0

[PATCH 4/6] slimbus: Use GFP_ATOMIC under spin lock

[srinivas.kandagatla]

From: Wei Yongjun <weiyongjun1@huawei.com>

A spin lock is taken here so we should use GFP_ATOMIC.

Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/slimbus/messaging.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/slimbus/messaging.c b/drivers/slimbus/messaging.c
index 8b2c77f516b9..a9a6dc4af0da 100644
--- a/drivers/slimbus/messaging.c
+++ b/drivers/slimbus/messaging.c
@@ -98,7 +98,7 @@ int slim_do_transfer(struct slim_controller *ctrl, struct slim_msg_txn *txn)
 	if (need_tid) {
 		spin_lock_irqsave(&ctrl->txn_lock, flags);
 		tid = idr_alloc(&ctrl->tid_idr, txn, 0,
-				SLIM_MAX_TIDS, GFP_KERNEL);
+				SLIM_MAX_TIDS, GFP_ATOMIC);
 		txn->tid = tid;
 
 		if (!txn->msg->comp)
-- 
2.15.0

[PATCH 5/6] slimbus: Fix missing unlock on error in slim_msg_response()

[srinivas.kandagatla]

From: Wei Yongjun <weiyongjun1@huawei.com>

Add the missing unlock before return from function slim_msg_response()
in the error handling case.

Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/slimbus/messaging.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/slimbus/messaging.c b/drivers/slimbus/messaging.c
index a9a6dc4af0da..884419c37e84 100644
--- a/drivers/slimbus/messaging.c
+++ b/drivers/slimbus/messaging.c
@@ -38,6 +38,7 @@ void slim_msg_response(struct slim_controller *ctrl, u8 *reply, u8 tid, u8 len)
 	if (msg == NULL || msg->rbuf == NULL) {
 		dev_err(ctrl->dev, "Got response to invalid TID:%d, len:%d\n",
 				tid, len);
+		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
 		return;
 	}
 
-- 
2.15.0

[PATCH 6/6] slimbus: qcom: Fix return value check in qcom_slim_probe()

[srinivas.kandagatla]

From: Wei Yongjun <weiyongjun1@huawei.com>

In case of error, the function devm_ioremap_resource() returns ERR_PTR()
and never returns NULL. The NULL test in the return value check should
be replaced with IS_ERR().

Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/slimbus/qcom-ctrl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/slimbus/qcom-ctrl.c b/drivers/slimbus/qcom-ctrl.c
index 8d12baae4119..ffb46f915334 100644
--- a/drivers/slimbus/qcom-ctrl.c
+++ b/drivers/slimbus/qcom-ctrl.c
@@ -529,9 +529,9 @@ static int qcom_slim_probe(struct platform_device *pdev)
 
 	slim_mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ctrl");
 	ctrl->base = devm_ioremap_resource(ctrl->dev, slim_mem);
-	if (!ctrl->base) {
+	if (IS_ERR(ctrl->base)) {
 		dev_err(&pdev->dev, "IOremap failed\n");
-		return -ENOMEM;
+		return PTR_ERR(ctrl->base);
 	}
 
 	sctrl->set_laddr = qcom_set_laddr;
-- 
2.15.0