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

From: John Stultz <john.stultz@linaro.org>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Archit Taneja <architt@codeaurora.org>,
	Xinliang Liu <xinliang.liu@linaro.org>,
	Dan Carpenter <dan.carpenter@oracle.com>,
	Sean Paul <seanpaul@chromium.org>,
	Hans Verkuil <hans.verkuil@cisco.com>,
	Andrzej Hajda <a.hajda@samsung.com>,
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
	David Airlie <airlied@linux.ie>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Bhumika Goyal <bhumirks@gmail.com>,
	Inki Dae <inki.dae@samsung.com>,
	dri-devel@lists.freedesktop.org,
	lkml <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] [RFT] drm: adv7511/33: fix adv7511_cec_init() failure handling
Date: Thu, 16 Nov 2017 14:20:30 -0800	[thread overview]
Message-ID: <CALAqxLUXWk=aPCJff+1tYLgPrQA87voROJu2Rw6Rya11=TczNQ@mail.gmail.com> (raw)
In-Reply-To: <CALAqxLWstbhPJVbR1hT1AGKMdpZ-GdMyjmDmqE78B0jr0D+-Sw@mail.gmail.com>

On Thu, Nov 16, 2017 at 1:50 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Wed, Nov 15, 2017 at 4:37 AM, Arnd Bergmann <arnd@arndb.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);

?
thanks
-john