* [PATCH] drm/amdgpu: Move the mutex_lock to protect the return status of securedisplay command buffer
@ 2022-10-25 7:59 Alan Liu
2022-10-25 18:32 ` Alex Deucher
0 siblings, 1 reply; 2+ messages in thread
From: Alan Liu @ 2022-10-25 7:59 UTC (permalink / raw)
To: amd-gfx; +Cc: alexander.deucher, Alan Liu, shane.xiao, wayne.lin
[Why]
Before we call psp_securedisplay_invoke(), we call
psp_prep_securedisplay_cmd_buf() to prepare and initialize the command
buffer.
However, we didn't use the mutex_lock to protect the status of command
buffer. So when multiple threads are using the command buffer, after
thread A return from psp_securedisplay_invoke() and the command buffer
status is set to SUCCESS, another thread B may call
psp_prep_securedisplay_cmd_buf() and initialize the status to FAILURE
again, and cause Thread A to get a failure return status.
[How]
Move the mutex_lock out of psp_securedisplay_invoke() to its caller to
cover psp_prep_securedisplay_cmd_buf() and the code checking the return
status of command buffer.
Signed-off-by: Alan Liu <HaoPing.Liu@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 9 +++++----
drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c | 4 ++++
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 4 ++++
3 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index effa7df3ddbf..7bbf869f4f0d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -1938,10 +1938,15 @@ static int psp_securedisplay_initialize(struct psp_context *psp)
} else
return ret;
+ mutex_lock(&psp->securedisplay_context.mutex);
+
psp_prep_securedisplay_cmd_buf(psp, &securedisplay_cmd,
TA_SECUREDISPLAY_COMMAND__QUERY_TA);
ret = psp_securedisplay_invoke(psp, TA_SECUREDISPLAY_COMMAND__QUERY_TA);
+
+ mutex_unlock(&psp->securedisplay_context.mutex);
+
if (ret) {
psp_securedisplay_terminate(psp);
/* free securedisplay shared memory */
@@ -1990,12 +1995,8 @@ int psp_securedisplay_invoke(struct psp_context *psp, uint32_t ta_cmd_id)
ta_cmd_id != TA_SECUREDISPLAY_COMMAND__SEND_ROI_CRC)
return -EINVAL;
- mutex_lock(&psp->securedisplay_context.mutex);
-
ret = psp_ta_invoke(psp, ta_cmd_id, &psp->securedisplay_context.context);
- mutex_unlock(&psp->securedisplay_context.mutex);
-
return ret;
}
/* SECUREDISPLAY end */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
index cc7597a15fe9..2c1d82fc4c34 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
@@ -121,6 +121,7 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct file *f, const char __u
switch (op) {
case 1:
+ mutex_lock(&psp->securedisplay_context.mutex);
psp_prep_securedisplay_cmd_buf(psp, &securedisplay_cmd,
TA_SECUREDISPLAY_COMMAND__QUERY_TA);
ret = psp_securedisplay_invoke(psp, TA_SECUREDISPLAY_COMMAND__QUERY_TA);
@@ -131,8 +132,10 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct file *f, const char __u
else
psp_securedisplay_parse_resp_status(psp, securedisplay_cmd->status);
}
+ mutex_unlock(&psp->securedisplay_context.mutex);
break;
case 2:
+ mutex_lock(&psp->securedisplay_context.mutex);
psp_prep_securedisplay_cmd_buf(psp, &securedisplay_cmd,
TA_SECUREDISPLAY_COMMAND__SEND_ROI_CRC);
securedisplay_cmd->securedisplay_in_message.send_roi_crc.phy_id = phy_id;
@@ -146,6 +149,7 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct file *f, const char __u
psp_securedisplay_parse_resp_status(psp, securedisplay_cmd->status);
}
}
+ mutex_unlock(&psp->securedisplay_context.mutex);
break;
default:
dev_err(adev->dev, "Invalid input: %s\n", str);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
index 8a441a22c46e..515352ba788e 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
@@ -123,6 +123,8 @@ static void amdgpu_dm_crtc_notify_ta_to_read(struct work_struct *work)
phy_id = crc_rd_wrk->phy_inst;
spin_unlock_irq(&crc_rd_wrk->crc_rd_work_lock);
+ mutex_lock(&psp->securedisplay_context.mutex);
+
psp_prep_securedisplay_cmd_buf(psp, &securedisplay_cmd,
TA_SECUREDISPLAY_COMMAND__SEND_ROI_CRC);
securedisplay_cmd->securedisplay_in_message.send_roi_crc.phy_id =
@@ -133,6 +135,8 @@ static void amdgpu_dm_crtc_notify_ta_to_read(struct work_struct *work)
psp_securedisplay_parse_resp_status(psp, securedisplay_cmd->status);
}
}
+
+ mutex_unlock(&psp->securedisplay_context.mutex);
}
bool amdgpu_dm_crc_window_is_activated(struct drm_crtc *crtc)
--
2.25.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] drm/amdgpu: Move the mutex_lock to protect the return status of securedisplay command buffer
2022-10-25 7:59 [PATCH] drm/amdgpu: Move the mutex_lock to protect the return status of securedisplay command buffer Alan Liu
@ 2022-10-25 18:32 ` Alex Deucher
0 siblings, 0 replies; 2+ messages in thread
From: Alex Deucher @ 2022-10-25 18:32 UTC (permalink / raw)
To: Alan Liu; +Cc: alexander.deucher, shane.xiao, amd-gfx, wayne.lin
On Tue, Oct 25, 2022 at 4:00 AM Alan Liu <HaoPing.Liu@amd.com> wrote:
>
> [Why]
> Before we call psp_securedisplay_invoke(), we call
> psp_prep_securedisplay_cmd_buf() to prepare and initialize the command
> buffer.
>
> However, we didn't use the mutex_lock to protect the status of command
> buffer. So when multiple threads are using the command buffer, after
> thread A return from psp_securedisplay_invoke() and the command buffer
> status is set to SUCCESS, another thread B may call
> psp_prep_securedisplay_cmd_buf() and initialize the status to FAILURE
> again, and cause Thread A to get a failure return status.
>
> [How]
> Move the mutex_lock out of psp_securedisplay_invoke() to its caller to
> cover psp_prep_securedisplay_cmd_buf() and the code checking the return
> status of command buffer.
>
> Signed-off-by: Alan Liu <HaoPing.Liu@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 9 +++++----
> drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c | 4 ++++
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 4 ++++
> 3 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> index effa7df3ddbf..7bbf869f4f0d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> @@ -1938,10 +1938,15 @@ static int psp_securedisplay_initialize(struct psp_context *psp)
> } else
> return ret;
>
> + mutex_lock(&psp->securedisplay_context.mutex);
> +
> psp_prep_securedisplay_cmd_buf(psp, &securedisplay_cmd,
> TA_SECUREDISPLAY_COMMAND__QUERY_TA);
>
> ret = psp_securedisplay_invoke(psp, TA_SECUREDISPLAY_COMMAND__QUERY_TA);
> +
> + mutex_unlock(&psp->securedisplay_context.mutex);
> +
> if (ret) {
> psp_securedisplay_terminate(psp);
> /* free securedisplay shared memory */
> @@ -1990,12 +1995,8 @@ int psp_securedisplay_invoke(struct psp_context *psp, uint32_t ta_cmd_id)
> ta_cmd_id != TA_SECUREDISPLAY_COMMAND__SEND_ROI_CRC)
> return -EINVAL;
>
> - mutex_lock(&psp->securedisplay_context.mutex);
> -
> ret = psp_ta_invoke(psp, ta_cmd_id, &psp->securedisplay_context.context);
>
> - mutex_unlock(&psp->securedisplay_context.mutex);
> -
> return ret;
> }
> /* SECUREDISPLAY end */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
> index cc7597a15fe9..2c1d82fc4c34 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
> @@ -121,6 +121,7 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct file *f, const char __u
>
> switch (op) {
> case 1:
> + mutex_lock(&psp->securedisplay_context.mutex);
> psp_prep_securedisplay_cmd_buf(psp, &securedisplay_cmd,
> TA_SECUREDISPLAY_COMMAND__QUERY_TA);
> ret = psp_securedisplay_invoke(psp, TA_SECUREDISPLAY_COMMAND__QUERY_TA);
> @@ -131,8 +132,10 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct file *f, const char __u
> else
> psp_securedisplay_parse_resp_status(psp, securedisplay_cmd->status);
> }
> + mutex_unlock(&psp->securedisplay_context.mutex);
> break;
> case 2:
> + mutex_lock(&psp->securedisplay_context.mutex);
> psp_prep_securedisplay_cmd_buf(psp, &securedisplay_cmd,
> TA_SECUREDISPLAY_COMMAND__SEND_ROI_CRC);
> securedisplay_cmd->securedisplay_in_message.send_roi_crc.phy_id = phy_id;
> @@ -146,6 +149,7 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct file *f, const char __u
> psp_securedisplay_parse_resp_status(psp, securedisplay_cmd->status);
> }
> }
> + mutex_unlock(&psp->securedisplay_context.mutex);
> break;
> default:
> dev_err(adev->dev, "Invalid input: %s\n", str);
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> index 8a441a22c46e..515352ba788e 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> @@ -123,6 +123,8 @@ static void amdgpu_dm_crtc_notify_ta_to_read(struct work_struct *work)
> phy_id = crc_rd_wrk->phy_inst;
> spin_unlock_irq(&crc_rd_wrk->crc_rd_work_lock);
>
> + mutex_lock(&psp->securedisplay_context.mutex);
> +
> psp_prep_securedisplay_cmd_buf(psp, &securedisplay_cmd,
> TA_SECUREDISPLAY_COMMAND__SEND_ROI_CRC);
> securedisplay_cmd->securedisplay_in_message.send_roi_crc.phy_id =
> @@ -133,6 +135,8 @@ static void amdgpu_dm_crtc_notify_ta_to_read(struct work_struct *work)
> psp_securedisplay_parse_resp_status(psp, securedisplay_cmd->status);
> }
> }
> +
> + mutex_unlock(&psp->securedisplay_context.mutex);
> }
>
> bool amdgpu_dm_crc_window_is_activated(struct drm_crtc *crtc)
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-10-25 18:32 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-25 7:59 [PATCH] drm/amdgpu: Move the mutex_lock to protect the return status of securedisplay command buffer Alan Liu
2022-10-25 18:32 ` Alex Deucher
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.