drm/msm/dsi: Fix an error code in msm_dsi_modeset_init()

PATCH In review Dan Carpenter 2 participants 8 messages Oct 1, 2021 – Oct 4, 2021

Subsystems (4)

DRM drivers Arm Qualcomm mailing list Kernel janitors DRM driver for Qualcomm display hardware

lore

Share & tools

lore

Dan Carpenter

author

2021-10-01 12:33 UTC #0 lore raw

PATCH 1/3 drm/msm/dsi: Fix an error code in msm_dsi_modeset_init()

Return an error code if msm_dsi_manager_validate_current_config(). Don't return success.

Fixes 8b03ad30e314 ("drm/msm/dsi: Use one connector for dual DSI mode")

Signed-off-by Dan Carpenter <[email protected]>

---
 drivers/gpu/drm/msm/dsi/dsi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/gpu/drm/msm/dsi/dsi.c +3 -1

index 614dc7f26f2c..75ae3008b68f 100644
@@ -215,8 +215,10 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
 		goto fail;
 	}
 
-	if (!msm_dsi_manager_validate_current_config(msm_dsi->id))
+	if (!msm_dsi_manager_validate_current_config(msm_dsi->id)) {
+		ret = -EINVAL;
 		goto fail;
+	}
 
 	msm_dsi->encoder = encoder;

Signature · 1 line

2.20.1

Dan Carpenter

author

2021-10-01 12:34 UTC #1 lore raw

PATCH 2/3 drm/msm/dsi: fix off by one in dsi_bus_clk_enable error handling

This disables a lock which wasn't enabled and it does not disable the first lock in the array.

Fixes 6e0eb52eba9e ("drm/msm/dsi: Parse bus clocks from a list")

Signed-off-by Dan Carpenter <[email protected]>

---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/msm/dsi/dsi_host.c +1 -1

index e269df285136..c86b5090fae6 100644
@@ -451,7 +451,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host)
 
 	return 0;
 err:
-	for (; i > 0; i--)
+	while (--i >= 0)
 		clk_disable_unprepare(msm_host->bus_clks[i]);
 
 	return ret;

Signature · 1 line

2.20.1

Dmitry Baryshkov

Reviewed-by on v1 2/3

2021-10-01 22:47 UTC #2 lore raw

Re: [PATCH 2/3] drm/msm/dsi: fix off by one in dsi_bus_clk_enable error handling

Quoted 5 lines · Dan Carpenter

This disables a lock which wasn't enabled and it does not disable the first lock in the array.
Fixes 6e0eb52eba9e ("drm/msm/dsi: Parse bus clocks from a list")
Signed-off-by Dan Carpenter <[email protected]>

Reviewed-by Dmitry Baryshkov <[email protected]>

We should probably switch this to bulk clk api.

Quoted 18 lines

---
  drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/msm/dsi/dsi_host.c +1 -1

index e269df285136..c86b5090fae6 100644
@@ -451,7 +451,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host)
 
  	return 0;
  err:
-	for (; i > 0; i--)
+	while (--i >= 0)
  		clk_disable_unprepare(msm_host->bus_clks[i]);
 
  	return ret;

Signature · 2 lines

With best wishes
Dmitry

Dan Carpenter

author

2021-10-01 12:36 UTC #3 lore raw

PATCH 3/3 drm/msm/dsi: fix signedness bug in msm_dsi_host_cmd_rx()

In reply to Dan Carpenter

The "msg->tx_len" variable is type size_t so if dsi_cmds2buf_tx() returns a negative error code that it type promoted to a high positive value and treat as a success. The second problem with this code is that it can return meaningless positive values on error.

Fixes a689554ba6ed ("drm/msm: Initial add DSI connector support")

Signed-off-by Dan Carpenter <[email protected]>

---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/gpu/drm/msm/dsi/dsi_host.c +3 -1

index c86b5090fae6..42073a562072 100644
@@ -2133,8 +2133,10 @@ int msm_dsi_host_cmd_rx(struct mipi_dsi_host *host,
 		}
 
 		ret = dsi_cmds2buf_tx(msm_host, msg);
-		if (ret < msg->tx_len) {
+		if (ret < 0 || ret < msg->tx_len) {
 			pr_err("%s: Read cmd Tx failed, %d\n", __func__, ret);
+			if (ret >= 0)
+				ret = -EIO;
 			return ret;
 		}

Signature · 1 line

2.20.1

Dmitry Baryshkov

2021-10-01 23:00 UTC #4 lore raw

Re: [PATCH 3/3] drm/msm/dsi: fix signedness bug in msm_dsi_host_cmd_rx()

Quoted 4 lines · Dan Carpenter

The "msg->tx_len" variable is type size_t so if dsi_cmds2buf_tx() returns a negative error code that it type promoted to a high positive value and treat as a success. The second problem with this code is that it can return meaningless positive values on error.

It looks to me that this piece of code is not fully correct at all. dsi_cmds2bus_tx would return the size of DSI packet, not the size of the DSI buffer.

Could you please be more specific, which 'meaningless positive values' were you receiving?

Quoted 24 lines

Fixes a689554ba6ed ("drm/msm: Initial add DSI connector support")

Signed-off-by Dan Carpenter <[email protected]>

---
  drivers/gpu/drm/msm/dsi/dsi_host.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

drivers/gpu/drm/msm/dsi/dsi_host.c +3 -1

index c86b5090fae6..42073a562072 100644
@@ -2133,8 +2133,10 @@ int msm_dsi_host_cmd_rx(struct mipi_dsi_host *host,
  		}
 
  		ret = dsi_cmds2buf_tx(msm_host, msg);
-		if (ret < msg->tx_len) {
+		if (ret < 0 || ret < msg->tx_len) {
  			pr_err("%s: Read cmd Tx failed, %d\n", __func__, ret);
+			if (ret >= 0)
+				ret = -EIO;
  			return ret;
  		}

Signature · 2 lines

With best wishes
Dmitry

Dan Carpenter

author

2021-10-04 06:19 UTC #5 lore raw

Re: [PATCH 3/3] drm/msm/dsi: fix signedness bug in msm_dsi_host_cmd_rx()

Quoted 6 lines · Dmitry Baryshkov

Quoted 4 lines · Dan Carpenter
The "msg->tx_len" variable is type size_t so if dsi_cmds2buf_tx() returns a negative error code that it type promoted to a high positive value and treat as a success. The second problem with this code is that it can return meaningless positive values on error.

Quoted 7 lines

It looks to me that this piece of code is not fully correct at all. dsi_cmds2bus_tx would return the size of DSI packet, not the size of the DSI buffer.
Could you please be more specific, which 'meaningless positive values' were you receiving?

Sorry, I misread the code. I thought it returned negatives or the number of bytes copied. (This is from static analysis btw). Anyway, returning only negatives is a much better way.

I will fix this patch and resend.

regards,
dan carpenter

Dan Carpenter

author

2021-10-04 06:46 UTC #6 lore raw

Re: [PATCH 3/3] drm/msm/dsi: fix signedness bug in msm_dsi_host_cmd_rx()

In reply to Dmitry Baryshkov

Quoted 9 lines · Dmitry Baryshkov

Quoted 4 lines · Dan Carpenter
The "msg->tx_len" variable is type size_t so if dsi_cmds2buf_tx() returns a negative error code that it type promoted to a high positive value and treat as a success. The second problem with this code is that it can return meaningless positive values on error.
It looks to me that this piece of code is not fully correct at all. dsi_cmds2bus_tx would return the size of DSI packet, not the size of the DSI buffer.

Ugh... I misread what you were saying. I was thinking I could just check for negatives. This sounds like struct_size() thing?

Quoted 4 lines

Could you please be more specific, which 'meaningless positive values' were you receiving?

Returning any positive values at this point is a bug. It's supposed to return the number of bytes that were recieved.

And there is another bug as well:

drivers/gpu/drm/msm/dsi/dsi_host.c
1370 static int dsi_cmds2buf_tx(struct msm_dsi_host *msm_host,
1371 const struct mipi_dsi_msg *msg)
1372 {
1373 int len, ret;
1374 int bllp_len = msm_host->mode->hdisplay *
1375 dsi_get_bpp(msm_host->format) / 8;
1376
1377 len = dsi_cmd_dma_add(msm_host, msg);
1378 if (!len) {

The dsi_cmd_dma_add() returns negative error codes so this check should be "if (len <= 0) {".

1379 pr_err("%s: failed to add cmd type = 0x%x\n",
1380 __func__, msg->type);
1381 return -EINVAL;
1382 }
1383

I'm not sure about the size of "the DSI packet" Could you handle this one and give me a Reported-by tag? That's probably simpler than another back and forth on email.

regards,
dan carpenter

Dmitry Baryshkov

Reviewed-by on v1 1/3

2021-10-01 22:45 UTC #7 lore raw

In reply to Dan Carpenter

Quoted 5 lines · Dan Carpenter

Return an error code if msm_dsi_manager_validate_current_config(). Don't return success.
Fixes 8b03ad30e314 ("drm/msm/dsi: Use one connector for dual DSI mode")
Signed-off-by Dan Carpenter <[email protected]>

Reviewed-by Dmitry Baryshkov <[email protected]>

Quoted 21 lines

---
  drivers/gpu/drm/msm/dsi/dsi.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

drivers/gpu/drm/msm/dsi/dsi.c +3 -1

index 614dc7f26f2c..75ae3008b68f 100644
@@ -215,8 +215,10 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
  		goto fail;
  	}
 
-	if (!msm_dsi_manager_validate_current_config(msm_dsi->id))
+	if (!msm_dsi_manager_validate_current_config(msm_dsi->id)) {
+		ret = -EINVAL;
  		goto fail;
+	}
 
  	msm_dsi->encoder = encoder;

Signature · 2 lines

With best wishes
Dmitry