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 dan.carpenter@oracle.com --- drivers/gpu/drm/msm/dsi/dsi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c index 614dc7f26f2c..75ae3008b68f 100644 --- a/drivers/gpu/drm/msm/dsi/dsi.c +++ b/drivers/gpu/drm/msm/dsi/dsi.c @@ -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;
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 dan.carpenter@oracle.com --- drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index e269df285136..c86b5090fae6 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -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;
On 01/10/2021 15:34, Dan Carpenter wrote:
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 dan.carpenter@oracle.com
Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
We should probably switch this to bulk clk api.
drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index e269df285136..c86b5090fae6 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -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;
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 dan.carpenter@oracle.com --- drivers/gpu/drm/msm/dsi/dsi_host.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index c86b5090fae6..42073a562072 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -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; }
On 01/10/2021 15:36, Dan Carpenter wrote:
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?
Fixes: a689554ba6ed ("drm/msm: Initial add DSI connector support") Signed-off-by: Dan Carpenter dan.carpenter@oracle.com
drivers/gpu/drm/msm/dsi/dsi_host.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index c86b5090fae6..42073a562072 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -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;
On Sat, Oct 02, 2021 at 01:59:56AM +0300, Dmitry Baryshkov wrote:
On 01/10/2021 15:36, Dan Carpenter wrote:
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?
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
On Sat, Oct 02, 2021 at 01:59:56AM +0300, Dmitry Baryshkov wrote:
On 01/10/2021 15:36, Dan Carpenter wrote:
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?
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
On 01/10/2021 15:33, Dan Carpenter wrote:
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 dan.carpenter@oracle.com
Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
drivers/gpu/drm/msm/dsi/dsi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c index 614dc7f26f2c..75ae3008b68f 100644 --- a/drivers/gpu/drm/msm/dsi/dsi.c +++ b/drivers/gpu/drm/msm/dsi/dsi.c @@ -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;
dri-devel@lists.freedesktop.org
-
Dan Carpenter -
Dmitry Baryshkov