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

[Noralf Trønnes]

Den 11.07.2018 20.53, skrev 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

Ok, I've fixed the commit message and pushed. Thanks for reviewing.
Icenowy, thanks for testing.

Noralf.

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