linux-kernel - Re: [PATCH v2 2/6] drm/vkms: Support multiple DRM objects (crtcs, etc.) per VKMS device

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20230818033605.2910699-1-brpol@chromium.org>
Date:   Fri, 18 Aug 2023 03:36:05 +0000
From:   Brandon Pollack <brpol@...omium.org>
To:     marius.vlad@...labora.com
Cc:     corbet@....net, dri-devel@...ts.freedesktop.org,
        hamohammed.sa@...il.com, jshargo@...omium.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        mairacanal@...eup.net, melissa.srw@...il.com, mripard@...nel.org,
        rodrigosiqueiramelo@...il.com, tzimmermann@...e.de
Subject: Re: [PATCH v2 2/6] drm/vkms: Support multiple DRM objects (crtcs, etc.) per VKMS device

Thanks for taking the time, everyone! Sorry it took so long, we had some
internal shuffling etc going on and I was building out what we needed these
chagnes for in the first place, this will be the first of a few replies
followed by a new version of the series to be sent out.

First up is a respons to Maria, Marius to follow.

---

Maria,

> -	if (vkms->output.composer_workq)
> -		destroy_workqueue(vkms->output.composer_workq);
> +	for (int i = 0; i < vkms->output.num_crtcs; i++)
> +		destroy_workqueue(vkms->output.crtcs[i].composer_workq);

I don't believe there is any need for a null check.  If you look in the
crtc_init, it is zero'd before returning any errors and that is the only place
it is set.  I don't believe that release can be called by an interrupt/async
(and if it did it would need a mutex/lock anyway).

>
>   static const struct drm_plane_funcs vkms_plane_funcs = {
> -	.update_plane		= drm_atomic_helper_update_plane,
> -	.disable_plane		= drm_atomic_helper_disable_plane,
> -	.reset			= vkms_plane_reset,

Yeah these do seem weirdly formatted on devices that don't treat tabs well.
The default formatter on my editor has a few suggestions for this file, but
they are all optional.  I'll send an extra patch that formats stuff and see
what people think, but ill make it seperate after all this is done. 
For now I reverted this.

>> -	if (IS_ERR(plane))
>> -		return plane;
>> +	if (output->num_planes >= VKMS_MAX_PLANES)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	plane = &output->planes[output->num_planes++];
>> +	ret = drm_universal_plane_init(dev, &plane->base, 0, &vkms_plane_funcs,
>> +				       vkms_formats, ARRAY_SIZE(vkms_formats),
>> +				       NULL, type, NULL);
>
>Wouldn't be possible to use drmm_universal_plane_alloc?

Maybe, but the *_init pattern allows these things to be inline in the struct as
they are now, and consistent with the other drm kernel objects in the
vkms_output struct.  There are also a few other places we could use drmm,
surely, but to limit the scope/risk why don't we do that as a followup?

---

Marius,

Yeah those values could safely be completely removed.  Good catch :)