Re: [PATCH v8 4/8] drm/komeda: use drmm_writeback_connector_init()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Manual call to `drm_mode_config_cleanup()` during driver teardown causes NULL pointer dereferences on drmm-managed encoder and connector.
--

--- Patch [4]: [PATCH v8 4/8] drm/komeda: use drmm_writeback_connector_init() ---
commit 252267d60505e182c9ac4aa8fed48741764eeb23
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

    drm/komeda: use drmm_writeback_connector_init()
    
    The driver uses drm_writeback_connector_init() instead of its drmm
    counterpart, but it doesn't perform the job queue cleanup (neither
    manually nor by calling drm_writeback_connector_cleanup()). On the
    contrary, the drmm_writeback_connector_init() function ensures the
    proper cleanup of the job queue.
    
    Use drmm_plain_encoder_alloc() to allocate simple encoder and
    drmm_writeback_connector_init() in order to initialize writeback
    connector instance.

> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> index 41cc3e080dc9c..65f072cfe42f9 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c

[ ... ]

> @@ -121,17 +122,10 @@ komeda_wb_connector_fill_modes(struct drm_connector *connector,
>  	return 0;
>  }
>  
> -static void komeda_wb_connector_destroy(struct drm_connector *connector)
> -{
> -	drm_connector_cleanup(connector);
> -	kfree(to_kconn(to_wb_conn(connector)));
> -}
> -
>  static const struct drm_connector_funcs komeda_wb_connector_funcs = {
>  	.reset			= drm_atomic_helper_connector_reset,
>  	.detect			= komeda_wb_connector_detect,
>  	.fill_modes		= komeda_wb_connector_fill_modes,
> -	.destroy		= komeda_wb_connector_destroy,
>  	.atomic_duplicate_state	= drm_atomic_helper_connector_duplicate_state,
>  	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
>  };

[ ... ]

> @@ -161,20 +157,29 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms,
>  					       kwb_conn->wb_layer->layer_type,
>  					       &n_formats);
>  	if (!formats) {
> -		kfree(kwb_conn);
> +		drmm_kfree(&kms->base, kwb_conn);
>  		return -ENOMEM;
>  	}
>  
> -	err = drm_writeback_connector_init(&kms->base, wb_conn,
> -					   &komeda_wb_connector_funcs,
> -					   &komeda_wb_encoder_helper_funcs,
> -					   formats, n_formats,
> -					   BIT(drm_crtc_index(&kcrtc->base)));
> +	encoder = drmm_plain_encoder_alloc(&kms->base, NULL,
> +					   DRM_MODE_ENCODER_VIRTUAL, NULL);

[Severity: High]
Will this crash during driver teardown or initialization failure?

The komeda driver still explicitly calls drm_mode_config_cleanup() during its
teardown in komeda_kms_detach() and in the error path of komeda_kms_attach():

komeda_kms_attach() {
    ...
cleanup_mode_config:
	drm_mode_config_cleanup(drm);
    ...
}

Because drm_mode_config_cleanup() unconditionally invokes the .destroy hook
for all encoders and connectors, and drmm_plain_encoder_alloc() is called
here with a NULL funcs pointer, will the cleanup attempt to dereference a
NULL funcs pointer (encoder->funcs->destroy)? 

Similarly, the .destroy hook has been removed from komeda_wb_connector_funcs
above, so will drm_mode_config_cleanup() also crash when attempting to call
connector->funcs->destroy()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723003205.330362-1-John.Harrison@Igalia.com?part=4