Re: [PATCH] [RFT] drm: adv7511/33: fix adv7511_cec_init() failure handling

[John Stultz]

On Thu, Nov 16, 2017 at 2:20 PM, John Stultz <john.stultz@xxxxxxxxxx> wrote:
> On Thu, Nov 16, 2017 at 1:50 PM, John Stultz <john.stultz@xxxxxxxxxx> wrote:
>> On Wed, Nov 15, 2017 at 4:37 AM, Arnd Bergmann <arnd@xxxxxxxx> 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