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

[Daniel Vetter]

On Fri, Jan 15, 2016 at 11:51:31AM +0200, Jani Nikula wrote:
> On Thu, 14 Jan 2016, Ville Syrjälä <ville.syrjala at 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 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>
> 
> Pushed to drm-intel-next-queued, thanks for the review and testing.

You failed bat CI. Please make that the failure really is pre-existing and
if so dig out the bugzilla for it. If that's not the case please revert.
I'll paste you the link to the internal wiki in private.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch