[v3,02/14] drm/i915/dp: return errors from rate_to_index()

Submitted by Jani Nikula on March 28, 2017, 2:59 p.m.

Details

Message ID 8a6e83b7bf35da0cbbc703ae157944107ff145be.1490712890.git.jani.nikula@intel.com
State New
Headers show
Series "drm/i915/dp: link rate and lane count refactoring" ( rev: 4 3 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Jani Nikula March 28, 2017, 2:59 p.m.
We shouldn't silently use the first element if we can't find the rate
we're looking for. Make rate_to_index() more generally useful, and
fallback to the first element in the caller, with a big warning.

Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 88c708b07c70..0e200a37b75b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1544,9 +1544,9 @@  static int rate_to_index(const int *rates, int len, int rate)
 
 	for (i = 0; i < len; i++)
 		if (rate == rates[i])
-			break;
+			return i;
 
-	return i;
+	return -1;
 }
 
 int
@@ -1564,8 +1564,13 @@  intel_dp_max_link_rate(struct intel_dp *intel_dp)
 
 int intel_dp_rate_select(struct intel_dp *intel_dp, int rate)
 {
-	return rate_to_index(intel_dp->sink_rates, intel_dp->num_sink_rates,
-			     rate);
+	int i = rate_to_index(intel_dp->sink_rates, intel_dp->num_sink_rates,
+			      rate);
+
+	if (WARN_ON(i < 0))
+		i = 0;
+
+	return i;
 }
 
 void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,

Comments

Won't it make more sense to squash this patch with Patch 01 in this series?
When i was reading Patch 1, I almost was going to comment about
handling the case where we dont find the index..

Regards
Manasi

On Tue, Mar 28, 2017 at 05:59:02PM +0300, Jani Nikula wrote:
> We shouldn't silently use the first element if we can't find the rate
> we're looking for. Make rate_to_index() more generally useful, and
> fallback to the first element in the caller, with a big warning.
> 
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 88c708b07c70..0e200a37b75b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1544,9 +1544,9 @@ static int rate_to_index(const int *rates, int len, int rate)
>  
>  	for (i = 0; i < len; i++)
>  		if (rate == rates[i])
> -			break;
> +			return i;
>  
> -	return i;
> +	return -1;
>  }
>  
>  int
> @@ -1564,8 +1564,13 @@ intel_dp_max_link_rate(struct intel_dp *intel_dp)
>  
>  int intel_dp_rate_select(struct intel_dp *intel_dp, int rate)
>  {
> -	return rate_to_index(intel_dp->sink_rates, intel_dp->num_sink_rates,
> -			     rate);
> +	int i = rate_to_index(intel_dp->sink_rates, intel_dp->num_sink_rates,
> +			      rate);
> +
> +	if (WARN_ON(i < 0))
> +		i = 0;
> +
> +	return i;
>  }
>  
>  void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
> -- 
> 2.1.4
>
On Tue, 28 Mar 2017, Manasi Navare <manasi.d.navare@intel.com> wrote:
> Won't it make more sense to squash this patch with Patch 01 in this series?
> When i was reading Patch 1, I almost was going to comment about
> handling the case where we dont find the index..

Matter of taste. You might then look at patch 3 and figure it should be
squashed too... you have to draw the line somewhere, and I think it's
always easier to review smaller pieces and be sure they are each
correct. If you find an issue with the 2nd patch, the 1st one is still
valid, and the follow-up review is also easier.

BR,
Jani.


>
> Regards
> Manasi
>
> On Tue, Mar 28, 2017 at 05:59:02PM +0300, Jani Nikula wrote:
>> We shouldn't silently use the first element if we can't find the rate
>> we're looking for. Make rate_to_index() more generally useful, and
>> fallback to the first element in the caller, with a big warning.
>> 
>> Cc: Manasi Navare <manasi.d.navare@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 88c708b07c70..0e200a37b75b 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1544,9 +1544,9 @@ static int rate_to_index(const int *rates, int len, int rate)
>>  
>>  	for (i = 0; i < len; i++)
>>  		if (rate == rates[i])
>> -			break;
>> +			return i;
>>  
>> -	return i;
>> +	return -1;
>>  }
>>  
>>  int
>> @@ -1564,8 +1564,13 @@ intel_dp_max_link_rate(struct intel_dp *intel_dp)
>>  
>>  int intel_dp_rate_select(struct intel_dp *intel_dp, int rate)
>>  {
>> -	return rate_to_index(intel_dp->sink_rates, intel_dp->num_sink_rates,
>> -			     rate);
>> +	int i = rate_to_index(intel_dp->sink_rates, intel_dp->num_sink_rates,
>> +			      rate);
>> +
>> +	if (WARN_ON(i < 0))
>> +		i = 0;
>> +
>> +	return i;
>>  }
>>  
>>  void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
>> -- 
>> 2.1.4
>>
On Wed, Mar 29, 2017 at 10:17:41AM +0300, Jani Nikula wrote:
> On Tue, 28 Mar 2017, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > Won't it make more sense to squash this patch with Patch 01 in this series?
> > When i was reading Patch 1, I almost was going to comment about
> > handling the case where we dont find the index..
> 
> Matter of taste. You might then look at patch 3 and figure it should be
> squashed too... you have to draw the line somewhere, and I think it's
> always easier to review smaller pieces and be sure they are each
> correct. If you find an issue with the 2nd patch, the 1st one is still
> valid, and the follow-up review is also easier.
> 
> BR,
> Jani.
>

OK. Agreed.

Regards
Manasi 
> 
> >
> > Regards
> > Manasi
> >
> > On Tue, Mar 28, 2017 at 05:59:02PM +0300, Jani Nikula wrote:
> >> We shouldn't silently use the first element if we can't find the rate
> >> we're looking for. Make rate_to_index() more generally useful, and
> >> fallback to the first element in the caller, with a big warning.
> >> 
> >> Cc: Manasi Navare <manasi.d.navare@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
  Reviewed-by: <manasi.d.navare@intel.com>

> >> ---
> >>  drivers/gpu/drm/i915/intel_dp.c | 13 +++++++++----
> >>  1 file changed, 9 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index 88c708b07c70..0e200a37b75b 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -1544,9 +1544,9 @@ static int rate_to_index(const int *rates, int len, int rate)
> >>  
> >>  	for (i = 0; i < len; i++)
> >>  		if (rate == rates[i])
> >> -			break;
> >> +			return i;
> >>  
> >> -	return i;
> >> +	return -1;
> >>  }
> >>  
> >>  int
> >> @@ -1564,8 +1564,13 @@ intel_dp_max_link_rate(struct intel_dp *intel_dp)
> >>  
> >>  int intel_dp_rate_select(struct intel_dp *intel_dp, int rate)
> >>  {
> >> -	return rate_to_index(intel_dp->sink_rates, intel_dp->num_sink_rates,
> >> -			     rate);
> >> +	int i = rate_to_index(intel_dp->sink_rates, intel_dp->num_sink_rates,
> >> +			      rate);
> >> +
> >> +	if (WARN_ON(i < 0))
> >> +		i = 0;
> >> +
> >> +	return i;
> >>  }
> >>  
> >>  void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
> >> -- 
> >> 2.1.4
> >> 
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center