Re: [PATCH] drm/i915/bios: Fix the sequence size calculations for MIPI seq v3

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 14 Jan 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Jan 14, 2016 at 05:12:07PM +0200, Jani Nikula wrote:
>> Two errors in a single line. The size was read from the wrong offset,
>> and the end index didn't take the five bytes for sequence byte and size
>> of sequence into account. Fix it all, and break up the calculations a
>> bit to make it clearer.
>> 
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Reported-by: Mika Kahola <mika.kahola@intel.com>
>> Fixes: 2a33d93486f2 ("drm/i915/bios: add support for MIPI sequence block v3")
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++---
>>  1 file changed, 14 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index 12e2f8b8bf9c..bf62a19c8f69 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -842,6 +842,7 @@ static int goto_next_sequence_v3(const u8 *data, int index, int total)
>>  {
>>  	int seq_end;
>>  	u16 len;
>> +	u32 size_of_sequence;
>>  
>>  	/*
>>  	 * Could skip sequence based on Size of Sequence alone, but also do some
>> @@ -852,14 +853,24 @@ static int goto_next_sequence_v3(const u8 *data, int index, int total)
>>  		return 0;
>>  	}
>>  
>> -	seq_end = index + *((const u32 *)(data + 1));
>> +	/* Skip Sequence Byte. */
>> +	index++;
>> +
>> +	/*
>> +	 * Size of Sequence. Excludes the Sequence Byte and the size itself,
>> +	 * includes MIPI_SEQ_ELEM_END byte, excludes the final MIPI_SEQ_END
>> +	 * byte.
>> +	 */
>> +	size_of_sequence = *((const uint32_t *)(data + index));
>
> Hmm. So it was reading from 'data+1' and now it's basically 'data+index+1'.
> So it was correct for the first sequence, and busted for later ones I
> suppose.
>
>> +	index += 4;
>> +
>> +	seq_end = index + size_of_sequence;
>
> And now we count the size of the sequence starting from the operation
> byte, before we counted it from the sequence byte. "Fortunately" the spec
> doesn't even tell us which is correct. If it works, it works.
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pushed to drm-intel-next-queued, thanks for the review and testing.

BR,
Jani.

>
> BTW I was thinking that we could maybe add some kind of
> "read the thing at index, and and increment the index past it" helpers.
>
> Eg.
> int get_u8(const void *data, int index, int size, u8 *ret);
> int get_u32(const void *data, int index, int size, u32 *ret);
>
> they could also do the index vs. size check and just return an error if
> we try to go too far.
>
>>  	if (seq_end > total) {
>>  		DRM_ERROR("Invalid sequence size\n");
>>  		return 0;
>>  	}
>>  
>> -	/* Skip Sequence Byte and Size of Sequence. */
>> -	for (index = index + 5; index < total; index += len) {
>> +	for (; index < total; index += len) {
>>  		u8 operation_byte = *(data + index);
>>  		index++;
>>  
>> -- 
>> 2.1.4

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx