Hi Javier Am 24.07.22 um 20:41 schrieb Javier Martinez Canillas:
Hello Thomas, Thanks for your feedback. On 7/24/22 20:24, Thomas Zimmermann wrote:Hi Javier Am 24.07.22 um 14:37 schrieb Javier Martinez Canillas:DRM drivers initialize the mode configuration with drmm_mode_config_init() and that function (among other things) initializes mutexes that are later used by modeset helpers. But the helpers should only attempt to grab those locks if the mode config was properly initialized. Otherwise it can lead to kernel oops. An example is when a DRM driver using the component framework does not initialize the drm_mode_config, because its .bind callback was not being executed due one of its expected sub-devices' driver failing to probe. Some drivers check the struct drm_driver.registered field as an indication on whether their .shutdown callback should call helpers to tearn down the mode configuration or not, but most drivers just assume that it is always safe to call helpers such as drm_atomic_helper_shutdown() during shutdown. Let make the DRM core more robust and prevent this to happen, by marking a struct drm_mode_config as initialized during drmm_mode_config_init(). that way helpers can check for it and not attempt to grab uninitialized mutexes.I disagree. This patch looks like cargo-cult programming and entirely arbitrary. The solution here is to fix drivers. The actual test to perform is to instrument the mutex implementation to detect uninitialized mutexes.While I do agree that drivers should be fixed, IMO we should try to make it hard for the kernel to crash. We already have checks in other DRM helpers to avoid accessing uninitialized data, so I don't see why we couldn't do the same here.
Code should stand on its own merits, instead of doing something because something else does it. The latter is what is referred to as cargo-cult programming.
Doing sanity checks on values is not a problem, but putting flag variables throughout the code to question other code's state is. That's not 'The Way of the C.' There's also the problem that a good part of struct drm_mode_config's initialization is open-coded in drivers. So the meaning of is_initialized is somewhat fuzzy.
I wrote this patch after fixing a bug in the drm/msm driver [0]. By looking at how other drivers handled this case, I'm pretty sure that they have the same problem. A warning is much better than a kernel crash during shutdown. [0]: https://patchwork.kernel.org/project/dri-devel/patch/20220724111327.1195693-1-javierm@xxxxxxxxxx/
I see. I wasn't aware that missing mode_config_init() is a problem. From the linked URL, I cannot really understand how it's related. msm appears to be calling drm_mode_config_init(), right? The idiomatic solution would be to convert msm to managed code. But that's an entirely different patchset, of course. (I only took a brief look at the link TBH.)
Here's a suggestion on how to construct the mode-config code in order to make it hard to misuse: Driver currently open-code the initialization of many fields in drm_mode_config. Expand the arguments of drm_mode_config_init() to take the pointer to the drm_mode_config_funcs. These functions are essential to do anything, so it's a good candidate for an argument. Drivers are easily converted the the new interface AFAICT. After the conversion, add a test to drm_mode_config_reset() that tests for the funcs to be set. drm_mode_config_reset() is also essential during initialization or the driver will fail immediately on the first modeset operation. That gives a test for an initialized mode_config without adding extra fields.
As a bit of a sidenote: we should consider making drm_mode_config_reset() and the reset callbacks return errors. The reset functions allocate memory for states and if this fails, we have no way of reporting the failure.
Best regards Thomas
Best regards Thomas
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
