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

[Noralf Trønnes]

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.

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