Re: [PATCH v4 08/13] drm/i915/guc: Implement response handling in send_ct()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 26 Mar 2018 17:35:00 +0200, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:

On Mon, 26 Mar 2018, Michał Winiarski <michal.winiarski@xxxxxxxxx> wrote:
On Fri, Mar 23, 2018 at 02:47:23PM +0000, Michal Wajdeczko wrote:
Instead of returning small data in response status dword,
GuC may append longer data as response message payload.
If caller provides response buffer, we will copy received
data and use number of received data dwords as new success
return value. We will WARN if response from GuC does not
match caller expectation.

v2: fix timeout and checkpatch warnings (Michal)

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>
Cc: Michel Thierry <michel.thierry@xxxxxxxxx>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
---
drivers/gpu/drm/i915/intel_guc_ct.c | 137 ++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_guc_ct.h |   5 ++
 2 files changed, 128 insertions(+), 14 deletions(-)


[SNIP]

@@ -418,13 +499,15 @@ static int ctch_send(struct intel_guc *guc,
static int intel_guc_send_ct(struct intel_guc *guc, const u32 *action, u32 len,
 			     u32 *response_buf, u32 response_buf_size)
 {
-	struct intel_guc_ct_channel *ctch = &guc->ct.host_channel;
+	struct intel_guc_ct *ct = &guc->ct;
+	struct intel_guc_ct_channel *ctch = &ct->host_channel;
 	u32 status = ~0; /* undefined */
 	int ret;

 	mutex_lock(&guc->send_mutex);

-	ret = ctch_send(guc, ctch, action, len, &status);
+ ret = ctch_send(ct, ctch, action, len, response_buf, response_buf_size,
+			&status);
 	if (unlikely(ret < 0)) {
 		DRM_ERROR("CT: send action %#X failed; err=%d status=%#X\n",
 			  action[0], ret, status);
@@ -503,8 +586,13 @@ static int ctb_read(struct intel_guc_ct_buffer *ctb, u32 *data)
 static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
 {
 	u32 header = msg[0];
+	u32 fence = msg[1];
 	u32 status = msg[2];
 	u32 len = ct_header_get_len(header) + 1; /* total len with header */
+	u32 payload_len = len - 3; /* len<3 is treated as protocol error */

Magic numbers, please ether define 3 as min payload length or hide this behind
macro.

And check len >= 3 before substracting 3.


This was speculative, same as reading 'fence' and 'status', hence comment
about protocol error, but also note that all of them were used after proper
check for unlikely len<3 condition later in the function.

I will reorder existing code to avoid such tricks

/m
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux