drm/vc4: Fixes for interrupt races #4433
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Yup, that fixes it for me. Extra logging confirmed that we were still going into the HPD ISR in the early probes that get deferred, it triggered during at least one of them, but the driver didn't blow up. Sometimes devm_ isn't your friend. |
pelwell
reviewed
Jul 5, 2021
drivers/gpu/drm/vc4/vc4_hdmi.c
Outdated
| return 0; | ||
| } | ||
|
|
||
| static int vc4_hdmi_hotplug_exit(struct vc4_hdmi *vc4_hdmi) |
Contributor
There was a problem hiding this comment.
Is there a convention that _exit functions can fail? If not, I think it would be better to lose the return value.
mripard
force-pushed
the
rpi/5.10-irq-breakages
branch
from
be43b34 to
7d9e83e
Compare
The CEC interrupt handlers are registered through the devm_request_threaded_irq function. However, while free_irq is indeed called properly when the device is unbound or bind fails, it's called after unbind or bind is done. In our particular case, it means that on failure it creates a window where our interrupt handler can be called, but we're freeing every resource (CEC adapter, DRM objects, etc.) it might need. In order to address this, let's switch to the non-devm variant to control better when the handler will be unregistered and allow us to make it safe. Fixes: 15b4511 ("drm/vc4: add HDMI CEC support") Signed-off-by: Maxime Ripard <maxime@cerno.tech>
The hotplugs interrupt handlers are registered through the devm_request_threaded_irq function. However, while free_irq is indeed called properly when the device is unbound or bind fails, it's called after unbind or bind is done. In our particular case, it means that on failure it creates a window where our interrupt handler can be called, but we're freeing every resource (CEC adapter, DRM objects, etc.) it might need. In order to address this, let's switch to the non-devm variant to control better when the handler will be unregistered and allow us to make it safe. Fixes: f479008 ("drm/vc4: hdmi: Rely on interrupts to handle hotplug") Signed-off-by: Maxime Ripard <maxime@cerno.tech>
Our hotplug handler will currently call the drm_kms_helper_hotplug_event every time a hotplug interrupt is called. However, since the device is registered after all the drivers have finished their bind callback, we have a window between when we install our interrupt handler and when drm_dev_register() is eventually called where our handler can run and call drm_kms_helper_hotplug_event but the device hasn't been registered yet, causing a null pointer dereference. Fix this by making sure we only call drm_kms_helper_hotplug_event if our device has been properly registered. Fixes: f479008 ("drm/vc4: hdmi: Rely on interrupts to handle hotplug") Signed-off-by: Maxime Ripard <maxime@cerno.tech>
mripard
force-pushed
the
rpi/5.10-irq-breakages
branch
from
7d9e83e to
eccc70a
Compare
Contributor
Author
Contributor
|
Thanks, Maxime. |
popcornmix
added a commit
to raspberrypi/firmware
that referenced
this pull request
Jul 9, 2021
See: raspberrypi/linux#4433 kernel: media: bcm2835-unicam: Forward input status from subdevice See: raspberrypi/linux#4437 kernel: bcm2711_thermal: Don't clamp temperature at zero See: raspberrypi/linux#4438 kernel: vc4/drm: Pick up more upstream patches See: raspberrypi/linux#4441 kernel: Add overlay for Chipdip I2S master DAC See: raspberrypi/linux#4444
popcornmix
added a commit
to Hexxeh/rpi-firmware
that referenced
this pull request
Jul 9, 2021
See: raspberrypi/linux#4433 kernel: media: bcm2835-unicam: Forward input status from subdevice See: raspberrypi/linux#4437 kernel: bcm2711_thermal: Don't clamp temperature at zero See: raspberrypi/linux#4438 kernel: vc4/drm: Pick up more upstream patches See: raspberrypi/linux#4441 kernel: Add overlay for Chipdip I2S master DAC See: raspberrypi/linux#4444
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi,
These are fixes to address issues with various races in the CEC and hotplug interrupt handlers in the HDMI driver.
The most visible fix is the last one, where we could have a null pointer dereference right when the driver was loaded. @6by9 could trigger it 100% of the time using hdmi_ignore_hotplug:0=1. I couldn't but forced interrupts to be generated by the interrupt controller at various point of the driver's life and could see that crash as well.