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

From: Naresh Kamboju <naresh.kamboju@linaro.org>
To: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Arnd Bergmann <arnd@arndb.de>,
	Archit Taneja <architt@codeaurora.org>,
	Bhumika Goyal <bhumirks@gmail.com>,
	David Airlie <airlied@linux.ie>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	dri-devel@lists.freedesktop.org,
	Hans Verkuil <hans.verkuil@cisco.com>,
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
	Dan Carpenter <dan.carpenter@oracle.com>
Subject: Re: [PATCH] [RFT] drm: adv7511/33: fix adv7511_cec_init() failure handling
Date: Thu, 16 Nov 2017 22:12:43 +0530	[thread overview]
Message-ID: <CA+G9fYuRbmmGt7b9Us=JKqzJjP+3AZr4ow8bkErgrUzd0+xAig@mail.gmail.com> (raw)
In-Reply-To: <06e748b6-738e-fbee-4251-0584a2030223@xs4all.nl>

On 15 November 2017 at 18:28, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 15/11/17 13:37, Arnd Bergmann 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.
>>
>> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
>> Cc: Xinliang Liu <xinliang.liu@linaro.org>
>> Cc: Dan Carpenter <dan.carpenter@oracle.com>
>> Cc: Sean Paul <seanpaul@chromium.org>
>> Cc: Hans Verkuil <hans.verkuil@cisco.com>
>> Cc: Archit Taneja <architt@codeaurora.org>
>> Link: https://bugs.linaro.org/show_bug.cgi?id=3345
>> Link: https://lkft.validation.linaro.org/scheduler/job/48017#L3551
>> Fixes: 7af35b0addbc ("drm/kirin: Checking for IS_ERR() instead of NULL")
>> Fixes: 3b1b975003e4 ("drm: adv7511/33: add HDMI CEC support")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>> Untested so far, this is what I came up with after reading the
>> WARN_ON log from a modified kernel.
>>
>> Naresh, can you give this one a go?

Tested with this patch.
I did not notice kernel crash/WARNING in dmesg log on HiKey (arm64) board.

Ref test log:
Link: https://pastebin.com/t8iLEFwF

Tested-by: Naresh Kamboju <naresh.kamboju@linaro.org>

>>
>> Hans and others, can you review in the meantime?
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> index 0e14f1572d05..93d1ecafe8fa 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -1204,7 +1204,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>>  #ifdef CONFIG_DRM_I2C_ADV7511_CEC
>>       ret = adv7511_cec_init(dev, adv7511, offset);
>>       if (ret)
>> -             goto err_unregister_cec;
>> +             goto err_unregister_bridge;
>
> Rather than adding the err_unregister_bridge label, I think it is better to move
> this code up to just before the call to drm_bridge_add().
>
> I think I just didn't realize that doing it after would require additional cleanup.
> But it should be perfectly fine to move it up so we can avoid doing that.
>
> I can't test it until Monday as I don't have access to the hardware at the moment.
>
> Regards,
>
>         Hans
>
>>  #else
>>       regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
>>                    ADV7511_CEC_CTRL_POWER_DOWN);
>> @@ -1212,6 +1212,11 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>>
>>       return 0;
>>
>> +#ifdef CONFIG_DRM_I2C_ADV7511_CEC
>> +err_unregister_bridge:
>> +     adv7511_audio_exit(adv7511);
>> +     drm_bridge_remove(&adv7511->bridge);
>> +#endif
>>  err_unregister_cec:
>>       i2c_unregister_device(adv7511->i2c_cec);
>>       if (adv7511->cec_clk)
>>
>