Re: [PATCH] drm/i915/ddi: Silence compiler warning for unknown output type

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx