<[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
