| Message ID | 20240916230103.611490-1-lyude@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] drm/panic: Fix uninitialized spinlock acquisition with CONFIG_DRM_PANIC=n | expand |
On 17/09/2024 01:00, Lyude Paul wrote: > It turns out that if you happen to have a kernel config where > CONFIG_DRM_PANIC is disabled and spinlock debugging is enabled, along with > KMS being enabled - we'll end up trying to acquire an uninitialized > spin_lock with drm_panic_lock() when we try to do a commit: The raw spinlock should be initialized in drm_dev_init() [1] regardless of DRM_PANIC being enabled or not. From the call trace, it looks like you are calling drm_client_register() before calling drm_dev_register(), and that's probably the root cause. I didn't find a doc saying drm_dev_register() should be done before drm_client_register(), but all drivers are doing it this way. Can you try to do that in rvkms, and see if it fixes this error ? Best regards,
Eek - sorry, I had already pushed this since it had been reviewed a while ago and I just forgot to push it afterwards. This being said though - I'm a little confused here myself. This is correct - drm_client_register was getting called too early, I wonder if I ran into this before I had moved around the order of stuff in the KMS init stuff for rust. I will check today and if it fixes the issue, I'll look at just sending out a revert for review. On Tue, 2024-09-17 at 09:32 +0200, Jocelyn Falempe wrote: > On 17/09/2024 01:00, Lyude Paul wrote: > > It turns out that if you happen to have a kernel config where > > CONFIG_DRM_PANIC is disabled and spinlock debugging is enabled, along with > > KMS being enabled - we'll end up trying to acquire an uninitialized > > spin_lock with drm_panic_lock() when we try to do a commit: > > The raw spinlock should be initialized in drm_dev_init() [1] regardless > of DRM_PANIC being enabled or not. > > From the call trace, it looks like you are calling > drm_client_register() before calling drm_dev_register(), and that's > probably the root cause. > > I didn't find a doc saying drm_dev_register() should be done before > drm_client_register(), but all drivers are doing it this way. > > Can you try to do that in rvkms, and see if it fixes this error ? > > Best regards, >
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 43cdf39019a44..5186d2114a503 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3015,7 +3015,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, bool stall) { int i, ret; - unsigned long flags; + unsigned long flags = 0; struct drm_connector *connector; struct drm_connector_state *old_conn_state, *new_conn_state; struct drm_crtc *crtc; diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h index 54085d5d05c34..f4e1fa9ae607a 100644 --- a/include/drm/drm_panic.h +++ b/include/drm/drm_panic.h @@ -64,6 +64,8 @@ struct drm_scanout_buffer { }; +#ifdef CONFIG_DRM_PANIC + /** * drm_panic_trylock - try to enter the panic printing critical section * @dev: struct drm_device @@ -149,4 +151,16 @@ struct drm_scanout_buffer { #define drm_panic_unlock(dev, flags) \ raw_spin_unlock_irqrestore(&(dev)->mode_config.panic_lock, flags) +#else + +static inline bool drm_panic_trylock(struct drm_device *dev, unsigned long flags) +{ + return true; +} + +static inline void drm_panic_lock(struct drm_device *dev, unsigned long flags) {} +static inline void drm_panic_unlock(struct drm_device *dev, unsigned long flags) {} + +#endif + #endif /* __DRM_PANIC_H__ */