[PATCH] drm/client: Fix: drm_client_new: Don't require DRM to be registered

[Daniel Vetter]

On Wed, Jul 11, 2018 at 8:21 PM, Noralf Trønnes <noralf at tronnes.org> wrote:
>
> Den 11.07.2018 20.00, skrev Daniel Vetter:
>>
>> On Wed, Jul 11, 2018 at 05:56:32PM +0200, Noralf Trønnes wrote:
>>>
>>> Commit 894a677f4b3e ("drm/cma-helper: Use the generic fbdev emulation")
>>> broke almost all drivers that use the CMA helper.
>>>
>>> The reason is that drm_client_new() requires that the DRM device has
>>> been registered, but the drivers register fbdev before registering DRM.
>>>
>>> Remove the requirement that DRM should be registered when creating a
>>> new client.
>>>
>>> This creates a theoretical race condition where drm_client_new() races
>>> with drm_dev_unregister() resulting in the .unregister hook not being
>>> called.
>>>
>>> Case:
>>> User loads module which calls drm_client_new().
>>> Device is unplugged and drm_dev_unregister() is called.
>>>
>>> If drm_dev_unregister() gets the clientlist_mutex first, the new client
>>> will never get it's unregister hook called.
>>
>> As long as we assume that drm_client_new() is serialized against
>> drm_dev_unregister (which it has to, or all the current fbdev code would
>> be busted) I'm not seeing a race here.
>>
>> We can't allow drm_client_new concurrently with drm_dev_register, but that
>> feature got canned. So looks all good to me, and no races anywhere to be
>> seen. Assuming you agree and update the commit message:
>
>
> There's no problem with an fbdev or bootsplash client, but the problem is
> with say a kmscon client that is loaded as a module. kmscon can be loaded
> any time when there is a DRM device present, but DRM can be unregistered
> while registering this new client.
>
> At least this is how I understand this.

I think we can figure out what to do with kmscon once it's being
(re)submitted for upstream. No point having headaches over an issue
that doesn't yet exist.

My first reaction would be to handle it exactly as the fbdev stuff,
and not allow kmscon to be loaded later on.
-Daniel

>
> Noralf.
>
>
>
>> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>>
>>> Fixes: c76f0f7cb546 ("drm: Begin an API for in-kernel clients")
>>> Cc: Maxime Ripard <maxime.ripard at bootlin.com>
>>> Cc: Icenowy Zheng <icenowy at aosc.io>
>>> Cc: Chen-Yu Tsai <wens at csie.org>
>>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
>>> ---
>>>   drivers/gpu/drm/drm_client.c | 11 +----------
>>>   1 file changed, 1 insertion(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
>>> index 4039a4d103a8..9b142f58d489 100644
>>> --- a/drivers/gpu/drm/drm_client.c
>>> +++ b/drivers/gpu/drm/drm_client.c
>>> @@ -78,7 +78,6 @@ EXPORT_SYMBOL(drm_client_close);
>>>   int drm_client_new(struct drm_device *dev, struct drm_client_dev
>>> *client,
>>>                    const char *name, const struct drm_client_funcs
>>> *funcs)
>>>   {
>>> -       bool registered;
>>>         int ret;
>>>         if (!drm_core_check_feature(dev, DRIVER_MODESET) ||
>>> @@ -97,21 +96,13 @@ int drm_client_new(struct drm_device *dev, struct
>>> drm_client_dev *client,
>>>                 goto err_put_module;
>>>         mutex_lock(&dev->clientlist_mutex);
>>> -       registered = dev->registered;
>>> -       if (registered)
>>> -               list_add(&client->list, &dev->clientlist);
>>> +       list_add(&client->list, &dev->clientlist);
>>>         mutex_unlock(&dev->clientlist_mutex);
>>> -       if (!registered) {
>>> -               ret = -ENODEV;
>>> -               goto err_close;
>>> -       }
>>>         drm_dev_get(dev);
>>>         return 0;
>>>   -err_close:
>>> -       drm_client_close(client);
>>>   err_put_module:
>>>         if (funcs)
>>>                 module_put(funcs->owner);
>>> --
>>> 2.15.1
>>>
>



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch