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

[Ville Syrjälä]

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 at linux.intel.com>
> Reported-by: Mika Kahola <mika.kahola at intel.com>
> Fixes: 2a33d93486f2 ("drm/i915/bios: add support for MIPI sequence block v3")
> Signed-off-by: Jani Nikula <jani.nikula at 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 at linux.intel.com>

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

-- 
Ville Syrjälä
Intel OTC