linux-kernel - Re: [PATCH v2] drm/nouveau: Fix nouveau_connector_ddc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <1070321d947fe290294ab3e838233227590a672c.camel@redhat.com>
Date:   Thu, 30 Aug 2018 13:37:41 -0400
From:   Lyude Paul <lyude@...hat.com>
To:     Ville Syrjälä <ville.syrjala@...ux.intel.com>
Cc:     nouveau@...ts.freedesktop.org, Ben Skeggs <bskeggs@...hat.com>,
        David Airlie <airlied@...ux.ie>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] drm/nouveau: Fix nouveau_connector_ddc_detect()

On Thu, 2018-08-30 at 20:36 +0300, Ville Syrjälä wrote:
> On Thu, Aug 30, 2018 at 01:16:28PM -0400, Lyude Paul wrote:
> > It looks like that when we moved over to using
> > drm_connector_for_each_possible_encoder() in nouveau, that one rather
> > important part of this function got dropped by accident:
> > 
> > 	/*          Right   v   here */
> > 	for (i = 0; nv_encoder = NULL, i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> > 		int id = connector->encoder_ids[i];
> > 		if (id == 0)
> > 			break;
> > 
> > Since it's rather difficult to notice: the conditional in this loop is
> > actually:
> > 
> > 	nv_encoder = NULL, i < DRM_CONNECTOR_MAX_ENCODER
> > 
> > Meaning that all early breaks result in nv_encoder keeping it's value,
> > otherwise nv_encoder = NULL. Ugh.
> > 
> > Since this got dropped, nouveau_connector_ddc_detect() now returns an
> > encoder for every single connector, regardless of whether or not it's
> > detected:
> > 
> >     [ 1780.056185] nouveau 0000:01:00.0: DRM: DDC responded, but no EDID for
> > DP-2
> > 
> > So: fix this to ensure we only return an encoder if we actually found
> > one, and clean up the rest of the function while we're at it since it's
> > nearly impossible to read properly.
> > 
> > Changes since v1:
> > - Don't skip ddc probing for LVDS if we can't switch DDC through
> >   vga-switcheroo, just do the DDC probing without calling
> >   vga_switcheroo_lock_ddc() - skeggsb
> > 
> > Signed-off-by: Lyude Paul <lyude@...hat.com>
> > Cc: Ville Syrjälä <ville.syrjala@...ux.intel.com>
> > Fixes: ddba766dd07e ("drm/nouveau: Use
> > drm_connector_for_each_possible_encoder()")
> > ---
> >  drivers/gpu/drm/nouveau/nouveau_connector.c | 49 ++++++++++++---------
> >  1 file changed, 28 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
> > b/drivers/gpu/drm/nouveau/nouveau_connector.c
> > index 71c3bd8ff9f8..461111aaedc4 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> > @@ -414,9 +414,10 @@ nouveau_connector_ddc_detect(struct drm_connector
> > *connector)
> >  	struct nouveau_connector *nv_connector = nouveau_connector(connector);
> >  	struct nouveau_drm *drm = nouveau_drm(dev);
> >  	struct nvkm_gpio *gpio = nvxx_gpio(&drm->client.device);
> > -	struct nouveau_encoder *nv_encoder = NULL;
> > +	struct nouveau_encoder *nv_encoder = NULL, *found = NULL;
> >  	struct drm_encoder *encoder;
> > -	int i, panel = -ENODEV;
> > +	int i, ret, panel = -ENODEV;
> > +	bool switcheroo_ddc = false;
> >  
> >  	/* eDP panels need powering on by us (if the VBIOS doesn't default it
> >  	 * to on) before doing any AUX channel transactions.  LVDS panel power
> > @@ -433,37 +434,43 @@ nouveau_connector_ddc_detect(struct drm_connector
> > *connector)
> >  	drm_connector_for_each_possible_encoder(connector, encoder, i) {
> >  		nv_encoder = nouveau_encoder(encoder);
> >  
> > -		if (nv_encoder->dcb->type == DCB_OUTPUT_DP) {
> > -			int ret = nouveau_dp_detect(nv_encoder);
> > +		switch (nv_encoder->dcb->type) {
> > +		case DCB_OUTPUT_DP:
> > +			ret = nouveau_dp_detect(nv_encoder);
> >  			if (ret == NOUVEAU_DP_MST)
> >  				return NULL;
> > -			if (ret == NOUVEAU_DP_SST)
> > -				break;
> > -		} else
> > -		if ((vga_switcheroo_handler_flags() &
> > -		     VGA_SWITCHEROO_CAN_SWITCH_DDC) &&
> > -		    nv_encoder->dcb->type == DCB_OUTPUT_LVDS &&
> > -		    nv_encoder->i2c) {
> > -			int ret;
> > -			vga_switcheroo_lock_ddc(dev->pdev);
> > -			ret = nvkm_probe_i2c(nv_encoder->i2c, 0x50);
> > -			vga_switcheroo_unlock_ddc(dev->pdev);
> > -			if (ret)
> > +			else if (ret == NOUVEAU_DP_SST)
> > +				found = nv_encoder;
> > +
> > +			break;
> > +		case DCB_OUTPUT_LVDS:
> > +			switcheroo_ddc = !!(vga_switcheroo_handler_flags() &
> > +					    VGA_SWITCHEROO_CAN_SWITCH_DDC);
> > +		/* fall-through */
> > +		default:
> > +			if (!nv_encoder->i2c)
> >  				break;
> > -		} else
> > -		if (nv_encoder->i2c) {
> > +
> > +			if (switcheroo_ddc)
> > +				vga_switcheroo_lock_ddc(dev->pdev);
> >  			if (nvkm_probe_i2c(nv_encoder->i2c, 0x50))
> > -				break;
> > +				found = nv_encoder;
> > +			if (switcheroo_ddc)
> > +				vga_switcheroo_unlock_ddc(dev->pdev);
> > +
> > +			break;
> >  		}
> > +		if (found)
> > +			break;
> 
> The diff is rather messy due to the change to a switch, but after staring
> at it for a while it does look correct to me.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@...ux.intel.com>
> and sorry for the mess.
No problem! Honestly, if I was the one who made those changes I expect I would
have broken this same bit of code as well :P

> 
> >  	}
> >  
> >  	/* eDP panel not detected, restore panel power GPIO to previous
> >  	 * state to avoid confusing the SOR for other output types.
> >  	 */
> > -	if (!nv_encoder && panel == 0)
> > +	if (!found && panel == 0)
> >  		nvkm_gpio_set(gpio, 0, DCB_GPIO_PANEL_POWER, 0xff, panel);
> >  
> > -	return nv_encoder;
> > +	return found;
> >  }
> >  
> >  static struct nouveau_encoder *
> > -- 
> > 2.17.1
> 
>