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
| ||
|
Message-ID: <015855d9-62f3-be81-a4c1-b8439534ec06@redhat.com> Date: Sun, 24 Oct 2021 22:40:25 +0200 From: Javier Martinez Canillas <javierm@...hat.com> To: Thomas Zimmermann <tzimmermann@...e.de>, linux-kernel@...r.kernel.org Cc: Peter Robinson <pbrobinson@...il.com>, Neal Gompa <ngompa13@...il.com>, Daniel Vetter <daniel@...ll.ch>, David Airlie <airlied@...ux.ie>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, dri-devel@...ts.freedesktop.org Subject: Re: [RFC PATCH] drm/aperture: Add param to disable conflicting framebuffers removal Hello Thomas, Thanks a lot for your feedback. On 10/22/21 21:05, Thomas Zimmermann wrote: [snip] >> >> +static bool drm_aperture_remove_fb = true; > > Global variables should default to zero if somehow possible. This way, > they can all be stored in the BSS segment and backed by a single shared > zero-filled page. Otherwise they require actual memory. In the worst > case, you'd allocate a full page to hold a single boolean. > >> +module_param_named(remove_fb, drm_aperture_remove_fb, bool, 0600); >> +MODULE_PARM_DESC(remove_fb, >> + "Allow conflicting framebuffers removal [default=true]"); >> + > > And with variables set to zero, a command-line parameter enables > non-default behavior (i.e., "drm-param=1"). That more logical than the > other way around IMHO. > Agreed. I'll change that. >> /** >> * DOC: overview >> * >> @@ -283,6 +288,9 @@ static void drm_aperture_detach_drivers(resource_size_t base, resource_size_t si >> * This function removes graphics device drivers which use memory range described by >> * @base and @size. >> * >> + * The conflicting framebuffers removal can be disabled by setting the drm.remove_fb=0 kernel >> + * command line option. When this is disabled, the function will return an -EINVAL errno code. > > Please use -EBUSY for the error. That's what the acquire function > returns in case of a conflict. > Sure, makes sense. I was pondering between -EINVAL, -EBUSY and -EPERM. >> + * >> * Returns: >> * 0 on success, or a negative errno code otherwise >> */ >> @@ -292,7 +300,12 @@ int drm_aperture_remove_conflicting_framebuffers(resource_size_t base, resource_ >> #if IS_REACHABLE(CONFIG_FB) > > Rather not split up this block. It's better style to put the > fbdev-related code into a helper and call it unconditionally. > > static drm_aperture_remove_conflicting_fbdev_framebuffers() > { > #if (FB) > ... > #endif > return 0; > } > Ok. >> struct apertures_struct *a; >> int ret; >> +#endif >> + >> + if (!drm_aperture_remove_fb) >> + return -EINVAL; > > There's still the question of the semantics of this parameter. It's a > bit fuzzy. > > If you use 'disable_handover' (as you mentioned in another mail), it > would mean that only the handover itself is disabled. So if simpledrm is > not bound to the device, then a native driver should load. That would be > hard to implement with the current code base, where we have to take old > fbdev drivers into account. > > (And to be pedantic, we don't really do a handover of the device. We > hot-unplug the generic platform device, so that the driver for the > native device can operate the HW without interference.) > > Simpledrm only acquires an aperture, but never removes a driver. If > there is a driver already, simpledrm would fail. Only native drivers try > to remove drivers and would trigger the test. So your patch is more > something like 'disable_native_drivers'. > > I'd go with 'disable_native_drivers', or maybe 'disable_device_handover' That works for me and "drm.disable_native_drivers" is also what Neal meant with his "drm.noplatformdrv", but yours is much easier to remember / type. Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Powered by blists - more mailing lists