Messages in this thread | | | Date | Thu, 18 Aug 2022 09:06:26 -0700 | Subject | Re: [PATCH v3] drm/msm/dp: check hpd_state before push idle pattern at dp_bridge_disable() | From | Abhinav Kumar <> |
| |
Hi Stephen
On 8/15/2022 10:08 AM, Stephen Boyd wrote: > Quoting Kuogee Hsieh (2022-08-11 08:20:01) >> >> On 8/10/2022 6:00 PM, Abhinav Kumar wrote: >>> >>> Even then, you do have a valid point. DRM framework should not have >>> caused the disable path to happen without an enable. >>> >>> I went through the stack mentioned in the issue. >>> >>> Lets see this part of the stack: >>> >>> dp_ctrl_push_idle+0x40/0x88 >>> dp_bridge_disable+0x24/0x30 >>> drm_atomic_bridge_chain_disable+0x90/0xbc >>> drm_atomic_helper_commit_modeset_disables+0x198/0x444 >>> msm_atomic_commit_tail+0x1d0/0x374 >>> >>> In drm_atomic_helper_commit_modeset_disables(), we call >>> disable_outputs(). >>> >>> AFAICT, this is the only place which has a protection to not call the >>> disable() flow if it was not enabled here: >>> >>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_atomic_helper.c#L1063 >>> >>> >>> But that function is only checking crtc_state->active. Thats set by >>> the usermode: >>> >>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_atomic_uapi.c#L407 >>> >>> >>> Now, if usermode sets that to true and then crashed it can bypass this >>> check and we will crash in the location kuogee is trying to fix. > > That seems bad, no? We don't want userspace to be able to crash and then > be able to call the disable path when enable never succeeded. > >>> >>> From the issue mentioned in >>> https://gitlab.freedesktop.org/drm/msm/-/issues/17, the reporter did >>> mention the usermode crashed. >>> >>> So this is my tentative analysis of whats happening here. >>> >>> Ideally yes, we should have been protected by the location mentioned >>> above in disable_outputs() but looks to me due to the above hypothesis >>> its getting bypassed. > > Can you fix the problem there? Not fixing it means that every driver out > there has to develop the same "fix", when it could be fixed in the core > one time. >
As per discussion on IRC with Rob, we have pushed another fix for this issue https://lore.kernel.org/all/1660759314-28088-1-git-send-email-quic_khsieh@quicinc.com/.
So, we can drop this one in favor of the other.
Thanks
Abhinav > Ideally drivers are simple. They configure the hardware for what the > function pointer is asking for. State management and things like that > should be pushed into the core framework so that we don't have to > duplicate that multiple times. > >>> >>> Thanks >>> >>> Abhinav >>> >>> >> Ii sound likes that there is a hole either at user space or drm. >> >> But that should not cause dp_bridge_disable() at dp driver to crash. > > Agreed. > >> >> Therefore it is properly to check hdp_state condition at >> dp_bridge_disable() to prevent it from crashing. >> > > Disagree. Userspace shouldn't be able to get drm into a wedged state.
| |