Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fbdev: mxc_hdmi: hotplug: lock fb_info before modification #13

Open
wants to merge 1 commit into
base: lf-6.1.y
Choose a base branch
from

Conversation

malsyned
Copy link

When an HDMI hotplug interrupt occurs, it schedules hotplug_worker(), which indirectly makes one or more calls to fb_videomode_to_var() that modify the fb_info struct associated with the HDMI connection. These can occur concurrently with the handling of an FBIOPUT_VSCREENINFO ioctl which also reads, modifies, and writes the same fields of fb_info, leading to race conditions.

This patch holds the fb_info lock in mxc_hdmi_cable_connected() while calling mxc_hdmi_set_mode(), eliminating those race conditions.

All of the hogplug handler's calls to fb_videomode_to_var() happen either directly or indirectly from mxc_hdmi_set_mode(), which is called from only one place, in mxc_hdmi_cable_connected(). That seems like the most convenient, least invasive, easiest to get right place to take and release the fb_info lock.

@malsyned malsyned changed the title V3-378 fbdev: mxc_hdmi: hotplug: lock fb_info before modification fbdev: mxc_hdmi: hotplug: lock fb_info before modification Oct 25, 2023
When an HDMI hotplug interrupt occurs, it schedules hotplug_worker(),
which indirectly makes one or more calls to fb_videomode_to_var() that
modify the fb_info struct associated with the HDMI connection. These can
occur concurrently with the handling of an FBIOPUT_VSCREENINFO ioctl
which also reads, modifies, and writes the same fields of fb_info,
leading to race conditions.

This patch holds the fb_info lock in mxc_hdmi_cable_connected() while
calling mxc_hdmi_set_mode(), eliminating those race conditions.

All of the hogplug handler's calls to fb_videomode_to_var() happen
either directly or indirectly from mxc_hdmi_set_mode(), which is called
from only one place, in mxc_hdmi_cable_connected(). That seems like the
most convenient, least invasive, easiest to get right place to take and
release the fb_info lock.
@Oquirella
Copy link

Which problems do the race conditions give?

@malsyned
Copy link
Author

malsyned commented Nov 1, 2023

Sometimes after an fbset I'm seeing this hotplug interrupt fire even though the display hasn't been disconnected and reconnected. It looks like a simulated disconnect/reconnect, since the kernel debug messages are almost exactly 1s apart:

2023-10-30T22:22:51,475334+00:00 mxc_hdmi 20e0000.hdmi_video: Hotplug interrupt received
2023-10-30T22:22:51,504619+00:00 mxc_hdmi 20e0000.hdmi_video: EVENT=plugout
2023-10-30T22:22:51,504974+00:00 mxc_hdmi 20e0000.hdmi_video: Hotplug interrupt received
2023-10-30T22:22:51,544717+00:00 mxc_hdmi 20e0000.hdmi_video: EVENT=plugin

So if the resolution is changed twice back-to-back, sometimes this race condition can happen. In my case, it's happening because I have an ExecStartPre=fbset ... for my weston.service, and then Weston's fbdev driver issues a second resolution change right afterward to triple the size of the virtual display as off-screen buffer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants