Re: [PATCH v3 4/5] drm/bridge: nwl-dsi: Add error handling of nwl_dsi_mode_set()

<[email protected]> writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Unbalanced resource teardown and PM runtime underflow in mode_set error path.
> --
>
> --- Patch [4]: [PATCH v3 4/5] drm/bridge: nwl-dsi: Add error handling of nwl_dsi_mode_set() ---
> commit 0db3c1cc2f73a9bc66aea2633f754029dd76b743
> Author: Esben Haabendal <[email protected]>
>
> drm/bridge: nwl-dsi: Add error handling of nwl_dsi_mode_set()
>
> This commit adds a return value check for nwl_dsi_mode_set() in
> nwl_dsi_bridge_mode_set(), jumping to an error path on failure.
>
>> diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
>> index 3fd41212933c8..e90588299c5ea 100644
>> --- a/drivers/gpu/drm/bridge/nwl-dsi.c
>> +++ b/drivers/gpu/drm/bridge/nwl-dsi.c
>> @@ -884,7 +884,9 @@ nwl_dsi_bridge_mode_set(struct drm_bridge *bridge,
>>  	}
>>
>>  	/* Step 2 from DSI reset-out instructions */
>> -	nwl_dsi_mode_set(dsi);
>> +	ret = nwl_dsi_mode_set(dsi);
>> +	if (ret < 0)
>> +		goto runtime_put;
>
> [Severity: High]
> This is a pre-existing issue, but does taking the runtime_put error path here
> lead to unbalanced resource teardown and PM runtime underflows?

Yes, it looks like it does. Similar to the existing error flows unfortunately.

> Because the drm_bridge_funcs.mode_set callback returns void, failures cannot be
> propagated to the DRM core to abort the commit. If nwl_dsi_mode_set() fails and
> jumps to runtime_put, it calls pm_runtime_put_sync() and returns early.
>
> However, the DRM core still considers the bridge pipeline state to be 'enabled',
> and will unconditionally call nwl_dsi_bridge_atomic_disable() during the next
> pipeline teardown.
>
> When atomic_disable() runs, it unconditionally calls pm_runtime_put(), leading
> to a reference count underflow:
>
> drivers/gpu/drm/bridge/nwl-dsi.c:nwl_dsi_bridge_atomic_disable() {
> 	...
> 	pm_runtime_put(dsi->dev);
> }

Maybe we should use pm_runtime_get_sync() instead of
pm_runtime_resume_and_get() in nwl_dsi_bridge_mode_set(), and then call
pm_request_idle() on error instead of pm_runtime_put_sync(). This way we
can keep the pm_runtime_put() in nwl_dsi_bridge_atomic_disable() without
risk of counter undeflow.

We would need to do something about the initial early return on
nwl_dsi_get_dphy_params() error, to avoid that causing counter
underflow. I think it should be safe to call pm_runtime_get_synmc()
first thing in nwl_dsi_bridge_mode_set().

Is it possible to do this in a follow-up patch series, so that we can
proceed with merging this without causing more complications by dealing
with this tricky pre-existing issue?

> Furthermore, atomic_disable() unconditionally calls nwl_dsi_disable(), which
> blindly attempts to tear down the PHY:
>
> drivers/gpu/drm/bridge/nwl-dsi.c:nwl_dsi_bridge_atomic_disable() {
> 	...
> 	nwl_dsi_disable(dsi);
> 	...
> }
>
> Since nwl_dsi_mode_set() already cleans up its own PHY resources on internal
> failure,
>
> this double-disable pushes the PHY framework's power_count and
> init_count to -1. The next attempt to initialize the display will increment
> these counters to 0, skipping the actual hardware ops->power_on() and
> ops->init() calls, permanently leaving the display broken until the module is
> reloaded.

True. That is a pre-existing issue, but I don't see how it is affected
by this change. In case nwl_dsi_mode_set() fails, it did internal cleanup/disable
of PHY resources before this change, and does so after this change in
the same way.

> While this broken error path already existed for clock initialization failures
> in this function, wiring up nwl_dsi_mode_set's return value to it exposes the
> PHY breakage on a new failure path.

I don't think that is true. The changed behavior in case
nwl_dsi_mode_set() fails is that we are now skipping deassert of rst_esc
and rst_byte and doing pm_runtime_put_sync(), where as before we were
executin g the deassert of rst_esc and rst_byte and not doing
pm_runtime_put_sync(). The PHY double-disable issue is unchanged.

>>
>>  	/* Step 3 from DSI reset-out instructions */
>>  	ret = reset_control_deassert(dsi->rst_esc);
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4