linux-kernel - Re: [PATCH v4 12/23] drm/tests: helpers: Add a (re)try helper variant to enable CRTC connector

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <050e7504-67c9-4384-826a-c655cef51b09@collabora.com>
Date: Mon, 19 May 2025 13:20:36 +0300
From: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>,
 Dave Stevenson <dave.stevenson@...pberrypi.com>,
 Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
 Dmitry Baryshkov <lumag@...nel.org>, kernel@...labora.com,
 dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 12/23] drm/tests: helpers: Add a (re)try helper variant
 to enable CRTC connector

Hi Maxime,

On 5/16/25 4:15 PM, Maxime Ripard wrote:
> Hi,
> 
> On Fri, Apr 25, 2025 at 01:27:03PM +0300, Cristian Ciocaltea wrote:
>> Provide a wrapper over drm_kunit_helper_enable_crtc_connector() to
>> automatically handle EDEADLK.
>>
>> This is going to help improve the error handling in a bunch of test
>> cases without open coding the restart of the atomic sequence.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
>> ---
>>  drivers/gpu/drm/tests/drm_kunit_helpers.c | 39 +++++++++++++++++++++++++++++++
>>  include/drm/drm_kunit_helpers.h           |  7 ++++++
>>  2 files changed, 46 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
>> index 5f7257840d8ef0aeabe5f00802f5037ed652ae66..4e1174c50b1f2b6358eb740cd73c6d86e53d0df9 100644
>> --- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
>> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
>> @@ -332,6 +332,45 @@ int drm_kunit_helper_enable_crtc_connector(struct kunit *test,
>>  }
>>  EXPORT_SYMBOL_GPL(drm_kunit_helper_enable_crtc_connector);
>>  
>> +/**
>> + * drm_kunit_helper_try_enable_crtc_connector - (Re)tries to enable a CRTC -> Connector output
>> + * @test: The test context object
>> + * @drm: The device to alloc the plane for
>> + * @crtc: The CRTC to enable
>> + * @connector: The Connector to enable
>> + * @mode: The display mode to configure the CRTC with
>> + * @ctx: Locking context
>> + *
>> + * This function is a wrapper over @drm_kunit_helper_enable_crtc_connector
>> + * to automatically handle EDEADLK and (re)try to enable the route from
>> + * @crtc to @connector, with the given @mode.
>> + *
>> + * Returns:
>> + *
>> + * A pointer to the new CRTC, or an ERR_PTR() otherwise.
>> + */
>> +int drm_kunit_helper_try_enable_crtc_connector(struct kunit *test,
>> +					       struct drm_device *drm,
>> +					       struct drm_crtc *crtc,
>> +					       struct drm_connector *connector,
>> +					       const struct drm_display_mode *mode,
>> +					       struct drm_modeset_acquire_ctx *ctx)
>> +{
>> +	int ret;
>> +
>> +retry_enable:
>> +	ret = drm_kunit_helper_enable_crtc_connector(test, drm, crtc, connector,
>> +						     mode, ctx);
>> +	if (ret == -EDEADLK) {
>> +		ret = drm_modeset_backoff(ctx);
>> +		if (!ret)
>> +			goto retry_enable;
>> +	}
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(drm_kunit_helper_try_enable_crtc_connector);
> 
> I'm not sure it's a good idea. This function might affect the locking
> context of the caller without even reporting it.
> 
> Generally speaking, I'd really prefer to have explicit locking, even if
> it means slightly more boilerplate.

Ack.

Will drop this patch and the next one for now, and sort this out in a
separate series.

Thanks,
Cristian