linux-kernel - Re: [RFC][PATCH] drm/bridge: adv7511: Re-write the i2c address as it may have been lost

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <5310723.io2gjIHX6R@avalon>
Date:   Tue, 17 Jan 2017 01:36:37 +0200
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     John Stultz <john.stultz@...aro.org>
Cc:     lkml <linux-kernel@...r.kernel.org>,
        David Airlie <airlied@...ux.ie>,
        Archit Taneja <architt@...eaurora.org>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        dri-devel@...ts.freedesktop.org
Subject: Re: [RFC][PATCH] drm/bridge: adv7511: Re-write the i2c address as it may have been lost

Hi John,

Thank you for the patch.

On Monday 16 Jan 2017 15:16:51 John Stultz wrote:
> Laurent: Would something like the following be preferred? Seems
> to work as well for me..

That looks good to me. Feel free to still de-duplicate the power on/off code 
if you want (but of course without adding the regcache_sync to the common 
power on function this time).

Please see below for an additional comment.

> I've found that by just turning the chip on and off via the
> POWER_DOWN register, I end up getting i2c_transfer errors on
> HiKey.
> 
> Investigating further, it seems some of the register state in
> the regmap cache is somehow getting lost, likely as the device
> registers were reset during power off.
> 
> Thus this patch simply re-writes the i2c address to the
> ADV7511_REG_EDID_I2C_ADDR register to ensure its properly set
> before we try to read the EDID data.
> 
> Cc: David Airlie <airlied@...ux.ie>
> Cc: Archit Taneja <architt@...eaurora.org>
> Cc: Wolfram Sang <wsa+renesas@...g-engineering.com>
> Cc: Lars-Peter Clausen <lars@...afoo.de>
> Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> Cc: dri-devel@...ts.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@...aro.org>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 405e460..32c59cb
> 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -567,6 +567,8 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
> 
>  	/* Reading the EDID only works if the device is powered */
>  	if (!adv7511->powered) {
> +		unsigned int edid_i2c_addr = (adv7511->i2c_main->addr << 1) + 
4;
> +
>  		regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>  				   ADV7511_POWER_POWER_DOWN, 0);
>  		if (adv7511->i2c_main->irq) {
> @@ -576,6 +578,9 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
>  				     ADV7511_INT1_DDC_ERROR);
>  		}
>  		adv7511->current_edid_segment = -1;
> +
> +		/* Reset the EDID_I2C_ADDR register as it may have been 
cleared */
> +		regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, 
edid_i2c_addr);

As powering the device off called regcache_mark_dirty(), this will perform an 
I2C write and cache the value. If we then try to read the EDID a second time 
without going through a full power on/off sequence, I believe that regmap will 
skip the write the second time, as the cache will contain the same value and 
won't be marked as dirty. Should we call regcache_mark_dirty() when powering 
the device down further down this function ?

>  	}
> 
>  	edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);

-- 
Regards,

Laurent Pinchart