linux-kernel - Re: [PATCH] [RFT] drm: adv7511/33: fix adv7511_cec

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: <CALAqxLVihS-twfXMgnWjGwhTBK9BAXkRS_4m6=uMfU8mKb7JSg@mail.gmail.com>
Date:   Thu, 16 Nov 2017 14:23:24 -0800
From:   John Stultz <john.stultz@...aro.org>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Archit Taneja <architt@...eaurora.org>,
        Xinliang Liu <xinliang.liu@...aro.org>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Sean Paul <seanpaul@...omium.org>,
        Hans Verkuil <hans.verkuil@...co.com>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        David Airlie <airlied@...ux.ie>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Bhumika Goyal <bhumirks@...il.com>,
        Inki Dae <inki.dae@...sung.com>,
        dri-devel@...ts.freedesktop.org,
        lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] [RFT] drm: adv7511/33: fix adv7511_cec_init() failure handling

On Thu, Nov 16, 2017 at 2:20 PM, John Stultz <john.stultz@...aro.org> wrote:
> On Thu, Nov 16, 2017 at 1:50 PM, John Stultz <john.stultz@...aro.org> wrote:
>> On Wed, Nov 15, 2017 at 4:37 AM, Arnd Bergmann <arnd@...db.de> wrote:
>>> An otherwise correct cleanup patch from Dan Carpenter turned a broken
>>> failure handling from a feature patch by Hans Verkuil into a kernel
>>> Oops, so bisection points to commit 7af35b0addbc ("drm/kirin: Checking
>>> for IS_ERR() instead of NULL") rather than 3b1b975003e4 ("drm:
>>> adv7511/33: add HDMI CEC support").
>>>
>>> I've managed to piece together several partial problems, though
>>> I'm still struggling with the bigger picture:
>>>
>>> adv7511_probe() registers a drm_bridge structure that was allocated
>>> with devm_kzalloc(). It calls adv7511_cec_init(), which fails for an
>>> unknown reason, which in turn triggers the registered structure to be
>>> removed.
>>>
>>> Elsewhere, kirin_drm_platform_probe() gets called, which calls
>>> of_graph_get_remote_node(), and that returns NULL. Before Dan's
>>> patch we would go on with a NULL pointer here and register that,
>>> now kirin_drm_platform_probe() fails with -ENODEV.
>>>
>>> In a third driver, dsi_parse_dt() calls drm_of_find_panel_or_bridge(),
>>> which after not finding a panel goes on to call of_drm_find_bridge(),
>>> and that crashes due to the earlier list corruption.
>>>
>>> This addresses the first issue by making sure that adv7511_probe()
>>> does not leave behind any corrupted list entries. This should
>>> get the system back to boot but needs testing. From my understanding,
>>> there is at least one more bug that needs to be resolved to actually
>>> get everything to work again.
>>
>> So I've started hitting the issue this patch tries to address (now
>> that the related code landed in Linus' tree). The only issue is that
>> with this fix, I don't see graphics initializing properly, so I
>> suspect something is still wrong with the error handling (though what
>> exactly I'm not sure).
>
> So this seems to only happen when CONFIG_DRM_I2C_ADV7511_CEC is
> enabled. If it is on, I don't get any graphics, but if its disabled
> graphics works.
>
> Tying this with Arnd's patch, I'm guessing adv7511_cec_init() is
> failing, but it seems like instead of just disabling the CEC feature,
> we're failing to load the driver entirely.
>
> Maybe should the logic be something like:
> #ifdef CONFIG_DRM_I2C_ADV7511_CEC
>         ret = adv7511_cec_init(dev, adv7511, offset);
>         if (ret)
> #endif
>             regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
>                         ADV7511_CEC_CTRL_POWER_DOWN);
>
> ?

I just tested with this, and this approach seems to work for me.

thanks
-john