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

[Michal Wajdeczko]

On Mon, 26 Mar 2018 17:35:00 +0200, Jani Nikula  
<jani.nikula at linux.intel.com> wrote:

> On Mon, 26 Mar 2018, Michał Winiarski <michal.winiarski at intel.com> 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 at intel.com>
>>> Cc: Oscar Mateo <oscar.mateo at intel.com>
>>> Cc: Michel Thierry <michel.thierry at intel.com>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>>> ---
>>>  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