linux-kernel - Re: [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

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]

Date:   Fri, 5 Nov 2021 11:15:53 +0100
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Javier Martinez Canillas <javierm@...hat.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        linux-kernel@...r.kernel.org
Cc:     "Pan, Xinhui" <Xinhui.Pan@....com>,
        Pekka Paalanen <pekka.paalanen@...labora.com>,
        Hans de Goede <hdegoede@...hat.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        dri-devel@...ts.freedesktop.org,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        amd-gfx@...ts.freedesktop.org,
        Gurchetan Singh <gurchetansingh@...omium.org>,
        Ben Skeggs <bskeggs@...hat.com>,
        VMware Graphics <linux-graphics-maintainer@...are.com>,
        Gerd Hoffmann <kraxel@...hat.com>,
        spice-devel@...ts.freedesktop.org, nouveau@...ts.freedesktop.org,
        Alex Deucher <alexander.deucher@....com>,
        Dave Airlie <airlied@...hat.com>,
        Christian König <christian.koenig@....com>,
        virtualization@...ts.linux-foundation.org,
        intel-gfx@...ts.freedesktop.org,
        Michel Dänzer <michel@...nzer.net>,
        Peter Robinson <pbrobinson@...il.com>
Subject: Re: [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers
 should be enabled

Hi

Am 05.11.21 um 10:48 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> On 11/5/21 09:43, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 04.11.21 um 21:09 schrieb Javier Martinez Canillas:
>>> Hello Jani,
>>>
>>> On 11/4/21 20:57, Jani Nikula wrote:
>>>> On Thu, 04 Nov 2021, Javier Martinez Canillas <javierm@...hat.com> wrote:
>>>>> +/**
>>>>> + * drm_drv_enabled - Checks if a DRM driver can be enabled
>>>>> + * @driver: DRM driver to check
>>>>> + *
>>>>> + * Checks whether a DRM driver can be enabled or not. This may be the case
>>>>> + * if the "nomodeset" kernel command line parameter is used.
>>>>> + *
>>>>> + * Return: 0 on success or a negative error code on failure.
>>>>> + */
>>>>> +int drm_drv_enabled(const struct drm_driver *driver)
>>
>> Jani mentioned that i915 absolutely wants this to run from the
>> module_init function. Best is to drop the parameter.
>>
> 
> Ok. I now wonder though how much value would add this function since
> it will just be a wrapper around the nomodeset check.
> 
> We talked about adding a new DRIVER_GENERIC feature flag and check for
> this, but as danvet mentioned that is not really needed. We just need
> to avoid testing for nomodeset in the simpledrm driver.
> 
> Do you envision other condition that could be added later to disable a
> DRM driver ? Or do you think that just from a code readability point of
> view makes worth it ?

DRIVER_GENERIC would have been nice, but it's not happening now.

I suggest to move over the nomodeset parameter (i.e., this patchset), 
then make the config option system agnostic, and finally add the test to 
all drivers expect simpledrm. That should solve the imminent problem.

Best regards
Thomas

> 
>>>>> +{
>>>>> +	if (vgacon_text_force()) {
>>>>> +		DRM_INFO("%s driver is disabled\n", driver->name);
>>>>> +		return -ENODEV;
>>>>> +	}
>>
>> If we run this from within a module_init function, we'd get plenty of
>> these warnings if drivers are compiled into the kernel. Maybe simply
>> remove the message. There's already a warning printed by the nomodeset
>> handler.
>>
> 
> Indeed. I'll just drop it.
> 
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL(drm_drv_enabled);
>>>>
>>>> The name implies a bool return, but it's not.
>>>>
>>>> 	if (drm_drv_enabled(...)) {
>>>> 		/* surprise, it's disabled! */
>>>> 	}
>>>>
>>>
>>> It used to return a bool in v2 but Thomas suggested an int instead to
>>> have consistency on the errno code that was returned by the callers.
>>>
>>> I should probably name that function differently to avoid confusion.
>>
>> Yes, please.
>>
> 
> drm_driver_check() maybe ?
>   
> Best regards,
> 

-- 
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

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)