| Message ID | 1459717154-27607-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On su, 2016-04-03 at 21:59 +0100, Chris Wilson wrote: > Silences > > src/drivers/gpu/drm/i915/intel_ddi.c: warning: 'port' may be used uninitialized in this function [-Wuninitialized] > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/intel_ddi.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 766156f88ef4..921edf183d22 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -315,6 +315,9 @@ static void ddi_get_encoder_port(struct intel_encoder *intel_encoder, > *dig_port = enc_to_mst(encoder)->primary; > *port = (*dig_port)->port; > break; > + default: > + WARN(1, "Invalid DDI encoder type %d\n", intel_encoder->type); > + /* fallthrough and treat as unknown */ While touching it, might as well fix into MISSING_CASE. With that; Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > case INTEL_OUTPUT_DISPLAYPORT: > case INTEL_OUTPUT_EDP: > case INTEL_OUTPUT_HDMI: > @@ -326,9 +329,6 @@ static void ddi_get_encoder_port(struct intel_encoder *intel_encoder, > *dig_port = NULL; > *port = PORT_E; > break; > - default: > - WARN(1, "Invalid DDI encoder type %d\n", intel_encoder->type); > - break; > } > } >
On Mon, Apr 04, 2016 at 09:20:27AM +0300, Joonas Lahtinen wrote: > On su, 2016-04-03 at 21:59 +0100, Chris Wilson wrote: > > Silences > > > > src/drivers/gpu/drm/i915/intel_ddi.c: warning: 'port' may be used uninitialized in this function [-Wuninitialized] > > > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index 766156f88ef4..921edf183d22 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -315,6 +315,9 @@ static void ddi_get_encoder_port(struct intel_encoder *intel_encoder, > > *dig_port = enc_to_mst(encoder)->primary; > > *port = (*dig_port)->port; > > break; > > + default: > > + WARN(1, "Invalid DDI encoder type %d\n", intel_encoder->type); > > + /* fallthrough and treat as unknown */ > > While touching it, might as well fix into MISSING_CASE. With that; I was tempted, I wasn't sure if not using MISSING_CASE was deliberate (trying not to confuse semantics between adding support for a new generation versus an unknown DDI)? So I left it for whoever has to make that fix (and kept the change as small as possible for a minor buglet). > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Thanks, -Chris
On Mon, 04 Apr 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Mon, Apr 04, 2016 at 09:20:27AM +0300, Joonas Lahtinen wrote: >> On su, 2016-04-03 at 21:59 +0100, Chris Wilson wrote: >> > Silences >> > >> > src/drivers/gpu/drm/i915/intel_ddi.c: warning: 'port' may be used uninitialized in this function [-Wuninitialized] >> > >> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> > --- >> > drivers/gpu/drm/i915/intel_ddi.c | 6 +++--- >> > 1 file changed, 3 insertions(+), 3 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> > index 766156f88ef4..921edf183d22 100644 >> > --- a/drivers/gpu/drm/i915/intel_ddi.c >> > +++ b/drivers/gpu/drm/i915/intel_ddi.c >> > @@ -315,6 +315,9 @@ static void ddi_get_encoder_port(struct intel_encoder *intel_encoder, >> > *dig_port = enc_to_mst(encoder)->primary; >> > *port = (*dig_port)->port; >> > break; >> > + default: >> > + WARN(1, "Invalid DDI encoder type %d\n", intel_encoder->type); >> > + /* fallthrough and treat as unknown */ >> >> While touching it, might as well fix into MISSING_CASE. With that; > > I was tempted, I wasn't sure if not using MISSING_CASE was deliberate > (trying not to confuse semantics between adding support for a new > generation versus an unknown DDI)? So I left it for whoever has to make > that fix (and kept the change as small as possible for a minor > buglet). Hitting MISSING_CASE here would, IMO, imply that the fix is to add the missing case, but we have plenty of intel_output_type enums that simply should not be handled here at all, and we should ensure we don't end up here with the wrong output types. Thus I think keeping the WARN is the way to go. I'm just slightly concerned about the fix here though. So we're screwed anyway if the output type is not handled, and we might use an uninitialized variable... but the assumption that using *dig_port = enc_to_dig_port(encoder) will result in any more meaningful result is wrong. BR, Jani. > >> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Thanks, > -Chris
On Mon, Apr 04, 2016 at 11:45:26AM +0300, Jani Nikula wrote: > On Mon, 04 Apr 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > On Mon, Apr 04, 2016 at 09:20:27AM +0300, Joonas Lahtinen wrote: > >> On su, 2016-04-03 at 21:59 +0100, Chris Wilson wrote: > >> > Silences > >> > > >> > src/drivers/gpu/drm/i915/intel_ddi.c: warning: 'port' may be used uninitialized in this function [-Wuninitialized] > >> > > >> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >> > --- > >> > drivers/gpu/drm/i915/intel_ddi.c | 6 +++--- > >> > 1 file changed, 3 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > >> > index 766156f88ef4..921edf183d22 100644 > >> > --- a/drivers/gpu/drm/i915/intel_ddi.c > >> > +++ b/drivers/gpu/drm/i915/intel_ddi.c > >> > @@ -315,6 +315,9 @@ static void ddi_get_encoder_port(struct intel_encoder *intel_encoder, > >> > *dig_port = enc_to_mst(encoder)->primary; > >> > *port = (*dig_port)->port; > >> > break; > >> > + default: > >> > + WARN(1, "Invalid DDI encoder type %d\n", intel_encoder->type); > >> > + /* fallthrough and treat as unknown */ > >> > >> While touching it, might as well fix into MISSING_CASE. With that; > > > > I was tempted, I wasn't sure if not using MISSING_CASE was deliberate > > (trying not to confuse semantics between adding support for a new > > generation versus an unknown DDI)? So I left it for whoever has to make > > that fix (and kept the change as small as possible for a minor > > buglet). > > Hitting MISSING_CASE here would, IMO, imply that the fix is to add the > missing case, but we have plenty of intel_output_type enums that simply > should not be handled here at all, and we should ensure we don't end up > here with the wrong output types. Thus I think keeping the WARN is the > way to go. > > I'm just slightly concerned about the fix here though. So we're screwed > anyway if the output type is not handled, and we might use an > uninitialized variable... but the assumption that using *dig_port = > enc_to_dig_port(encoder) will result in any more meaningful result is > wrong. Hobson's choice, we're fubar anyway. The only question is whether or not we can make a choice that prevents us from locking the machine up during boot. I was tempted by using INTEL_OUTPUT_VGA instead, would that be better? -Chris
On ma, 2016-04-04 at 10:00 +0100, Chris Wilson wrote: > On Mon, Apr 04, 2016 at 11:45:26AM +0300, Jani Nikula wrote: > > > > On Mon, 04 Apr 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > > > On Mon, Apr 04, 2016 at 09:20:27AM +0300, Joonas Lahtinen wrote: > > > > > > > > On su, 2016-04-03 at 21:59 +0100, Chris Wilson wrote: > > > > > > > > > > Silences > > > > > > > > > > src/drivers/gpu/drm/i915/intel_ddi.c: warning: 'port' may be used uninitialized in this function [-Wuninitialized] > > > > > > > > > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_ddi.c | 6 +++--- > > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > > > > index 766156f88ef4..921edf183d22 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > > > @@ -315,6 +315,9 @@ static void ddi_get_encoder_port(struct intel_encoder *intel_encoder, > > > > > *dig_port = enc_to_mst(encoder)->primary; > > > > > *port = (*dig_port)->port; > > > > > break; > > > > > + default: > > > > > + WARN(1, "Invalid DDI encoder type %d\n", intel_encoder->type); > > > > > + /* fallthrough and treat as unknown */ > > > > While touching it, might as well fix into MISSING_CASE. With that; > > > I was tempted, I wasn't sure if not using MISSING_CASE was deliberate > > > (trying not to confuse semantics between adding support for a new > > > generation versus an unknown DDI)? So I left it for whoever has to make > > > that fix (and kept the change as small as possible for a minor > > > buglet). > > Hitting MISSING_CASE here would, IMO, imply that the fix is to add the > > missing case, but we have plenty of intel_output_type enums that simply > > should not be handled here at all, and we should ensure we don't end up > > here with the wrong output types. Thus I think keeping the WARN is the > > way to go. > > > > I'm just slightly concerned about the fix here though. So we're screwed > > anyway if the output type is not handled, and we might use an > > uninitialized variable... but the assumption that using *dig_port = > > enc_to_dig_port(encoder) will result in any more meaningful result is > > wrong. Well, enumeration values *known* not to be acceptable should cause a ERR_PTR(-EINVAL) or along that route, and there should be a MISSING_CASE for rest totally unexpected values. How to actually handle that situation outside of the function to make system bootable, is an entirely different question :P Regards, Joonas > Hobson's choice, we're fubar anyway. The only question is whether or not > we can make a choice that prevents us from locking the machine up during > boot. I was tempted by using INTEL_OUTPUT_VGA instead, would that be > better? > -Chris >
On Mon, 04 Apr 2016, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: > Well, enumeration values *known* not to be acceptable should cause a > ERR_PTR(-EINVAL) or along that route, and there should be a > MISSING_CASE for rest totally unexpected values. In this case, they're all the same. No use returning errors when this shouldn't happen to begin with. J.
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 766156f88ef4..921edf183d22 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -315,6 +315,9 @@ static void ddi_get_encoder_port(struct intel_encoder *intel_encoder, *dig_port = enc_to_mst(encoder)->primary; *port = (*dig_port)->port; break; + default: + WARN(1, "Invalid DDI encoder type %d\n", intel_encoder->type); + /* fallthrough and treat as unknown */ case INTEL_OUTPUT_DISPLAYPORT: case INTEL_OUTPUT_EDP: case INTEL_OUTPUT_HDMI: @@ -326,9 +329,6 @@ static void ddi_get_encoder_port(struct intel_encoder *intel_encoder, *dig_port = NULL; *port = PORT_E; break; - default: - WARN(1, "Invalid DDI encoder type %d\n", intel_encoder->type); - break; } }
Silences src/drivers/gpu/drm/i915/intel_ddi.c: warning: 'port' may be used uninitialized in this function [-Wuninitialized] Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/intel_ddi.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)