| Message ID | 20230718013216.495830-1-suhui@nfschina.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915/tv: avoid possible division by zero | expand |
On Tue, Jul 18, 2023 at 09:32:17AM +0800, Su Hui wrote: > Clang warning: drivers/gpu/drm/i915/display/intel_tv.c: > line 991, column 22 Division by zero. > Assuming tv_mode->oversample=1 and (!tv_mode->progressive)=1, > then division by zero will happen. > > Fixes: 1bba5543e4fe ("drm/i915: Fix TV encoder clock computation") > Signed-off-by: Su Hui <suhui@nfschina.com> > --- > drivers/gpu/drm/i915/display/intel_tv.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_tv.c b/drivers/gpu/drm/i915/display/intel_tv.c > index 36b479b46b60..f59553f7c132 100644 > --- a/drivers/gpu/drm/i915/display/intel_tv.c > +++ b/drivers/gpu/drm/i915/display/intel_tv.c > @@ -988,7 +988,7 @@ intel_tv_mode_to_mode(struct drm_display_mode *mode, > const struct tv_mode *tv_mode, > int clock) > { > - mode->clock = clock / (tv_mode->oversample >> !tv_mode->progressive); > + mode->clock = clock / tv_mode->oversample << !tv_mode->progressive; but this does not provide the same value. Try with: 8 / (2 >> 1) and 8 / 2 << 1 They are definitely different. The real check could be: if (!(tv_mode->oversample >> 1)) return ... But first I would check if that's actually possible. Andi
On 2023/7/25 01:35, Andi Shyti wrote: > On Tue, Jul 18, 2023 at 09:32:17AM +0800, Su Hui wrote: >> Clang warning: drivers/gpu/drm/i915/display/intel_tv.c: >> line 991, column 22 Division by zero. >> Assuming tv_mode->oversample=1 and (!tv_mode->progressive)=1, >> then division by zero will happen. >> >> Fixes: 1bba5543e4fe ("drm/i915: Fix TV encoder clock computation") >> Signed-off-by: Su Hui <suhui@nfschina.com> >> --- >> drivers/gpu/drm/i915/display/intel_tv.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_tv.c b/drivers/gpu/drm/i915/display/intel_tv.c >> index 36b479b46b60..f59553f7c132 100644 >> --- a/drivers/gpu/drm/i915/display/intel_tv.c >> +++ b/drivers/gpu/drm/i915/display/intel_tv.c >> @@ -988,7 +988,7 @@ intel_tv_mode_to_mode(struct drm_display_mode *mode, >> const struct tv_mode *tv_mode, >> int clock) >> { >> - mode->clock = clock / (tv_mode->oversample >> !tv_mode->progressive); >> + mode->clock = clock / tv_mode->oversample << !tv_mode->progressive; > but this does not provide the same value. Try with: > > 8 / (2 >> 1) > > and > > 8 / 2 << 1 > > They are definitely different. > > The real check could be: > > if (!(tv_mode->oversample >> 1)) > return ... > > But first I would check if that's actually possible. Oh, I have a v3 patch, like this: - mode->clock = clock / (tv_mode->oversample >> !tv_mode->progressive); + mode->clock = clock; + if (tv_mode->oversample >> !tv_mode->progressive) + mode->clock /= tv_mode->oversample >> 1; But I'm not sure does it need to print some error messages or do some things when "tv_mode->oversample << !tv_mode->progressive" is zero? If all right , I will send this v3 patch. Su Hui > Andi
The reason why the first five attempts had bugs is because we are
trying to write it in the most complicated way possible, shifting by
logical not what?
regards,
dan carpenter
diff --git a/drivers/gpu/drm/i915/display/intel_tv.c b/drivers/gpu/drm/i915/display/intel_tv.c
index 36b479b46b60..6997b6cb1df2 100644
--- a/drivers/gpu/drm/i915/display/intel_tv.c
+++ b/drivers/gpu/drm/i915/display/intel_tv.c
@@ -988,7 +988,13 @@ intel_tv_mode_to_mode(struct drm_display_mode *mode,
const struct tv_mode *tv_mode,
int clock)
{
- mode->clock = clock / (tv_mode->oversample >> !tv_mode->progressive);
+ int div = tv_mode->oversample;
+
+ if (!tv_mode->progressive)
+ div >>= 1;
+ if (div == 0)
+ div = 1;
+ mode->clock = clock / div;
/*
* tv_mode horizontal timings:
@@ -1135,6 +1141,8 @@ intel_tv_get_config(struct intel_encoder *encoder,
break;
default:
tv_mode.oversample = 1;
+ WARN_ON_ONCE(!tv_mode.progressive);
+ tv_mode.progressive = true;
break;
}
On 2023/7/25 13:51, Dan Carpenter wrote: > The reason why the first five attempts had bugs is because we are > trying to write it in the most complicated way possible, shifting by > logical not what? Wonderful! Should I add your name as signed-of-by? I will send a v3 patch later. Really thanks for your help! Su Hui > regards, > dan carpenter > > diff --git a/drivers/gpu/drm/i915/display/intel_tv.c b/drivers/gpu/drm/i915/display/intel_tv.c > index 36b479b46b60..6997b6cb1df2 100644 > --- a/drivers/gpu/drm/i915/display/intel_tv.c > +++ b/drivers/gpu/drm/i915/display/intel_tv.c > @@ -988,7 +988,13 @@ intel_tv_mode_to_mode(struct drm_display_mode *mode, > const struct tv_mode *tv_mode, > int clock) > { > - mode->clock = clock / (tv_mode->oversample >> !tv_mode->progressive); > + int div = tv_mode->oversample; > + > + if (!tv_mode->progressive) > + div >>= 1; > + if (div == 0) > + div = 1; > + mode->clock = clock / div; > > /* > * tv_mode horizontal timings: > @@ -1135,6 +1141,8 @@ intel_tv_get_config(struct intel_encoder *encoder, > break; > default: > tv_mode.oversample = 1; > + WARN_ON_ONCE(!tv_mode.progressive); > + tv_mode.progressive = true; > break; > } > >
On Wed, Jul 26, 2023 at 09:21:50AM +0800, Su Hui wrote: > On 2023/7/25 13:51, Dan Carpenter wrote: > > The reason why the first five attempts had bugs is because we are > > trying to write it in the most complicated way possible, shifting by > > logical not what? > Wonderful! Should I add your name as signed-of-by? Sure. Or you can put it as a Suggested-by. regards, dan carpenter
diff --git a/drivers/gpu/drm/i915/display/intel_tv.c b/drivers/gpu/drm/i915/display/intel_tv.c index 36b479b46b60..f59553f7c132 100644 --- a/drivers/gpu/drm/i915/display/intel_tv.c +++ b/drivers/gpu/drm/i915/display/intel_tv.c @@ -988,7 +988,7 @@ intel_tv_mode_to_mode(struct drm_display_mode *mode, const struct tv_mode *tv_mode, int clock) { - mode->clock = clock / (tv_mode->oversample >> !tv_mode->progressive); + mode->clock = clock / tv_mode->oversample << !tv_mode->progressive; /* * tv_mode horizontal timings:
Clang warning: drivers/gpu/drm/i915/display/intel_tv.c: line 991, column 22 Division by zero. Assuming tv_mode->oversample=1 and (!tv_mode->progressive)=1, then division by zero will happen. Fixes: 1bba5543e4fe ("drm/i915: Fix TV encoder clock computation") Signed-off-by: Su Hui <suhui@nfschina.com> --- drivers/gpu/drm/i915/display/intel_tv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)