linux-kernel - Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

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]

Date:   Tue, 22 Nov 2016 09:25:22 -0800
From:   John Stultz <john.stultz@...aro.org>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
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" <dri-devel@...ts.freedesktop.org>
Subject: Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off()
 so they can be reused internally

On Tue, Nov 22, 2016 at 12:14 AM, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
> On Monday 21 Nov 2016 16:37:30 John Stultz wrote:
 @@ -545,24 +554,13 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
>>       unsigned int count;
>>
>>       /* Reading the EDID only works if the device is powered */
>> -     if (!adv7511->powered) {
>> -             regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>> -                                ADV7511_POWER_POWER_DOWN, 0);
>> -             if (adv7511->i2c_main->irq) {
>> -                     regmap_write(adv7511->regmap,
> ADV7511_REG_INT_ENABLE(0),
>> -                                  ADV7511_INT0_EDID_READY);
>> -                     regmap_write(adv7511->regmap,
> ADV7511_REG_INT_ENABLE(1),
>> -                                  ADV7511_INT1_DDC_ERROR);
>> -             }
>> -             adv7511->current_edid_segment = -1;
>> -     }
>> +     if (!adv7511->powered)
>> +             __adv7511_power_on(adv7511);
>
> The __adv7511_power_on() function does more than the above, in particular it
> performs an expensive regcache_sync() and calls adv7533_dsi_power_on() for the
> ADV7533. Don't those operations have side effects that are either not wanted
> or not needed here ? In any case this patch modifies the behaviour of the
> driver, which needs to be documented in the kernel message.

So yes, while the adv7533 bits aren't needed in the internal function,
I'm finding the logic to pulse the HPD and the regcache_sync call seem
to be needed side effects, as without that logic, I get i2c_transfer()
errors in adv7511_get_edid_block().

I'll try to rework this patch to split the two changes of reworking
the power_on/off function to be re-used (with no logic chage), and the
patch to reuse it in get_modes() which resolves a bug.

Thanks so much for the review!
-john