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@ffwll.ch

...

Fixes: c76f0f7cb546 ("drm: Begin an API for in-kernel clients") Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Icenowy Zheng icenowy@aosc.io Cc: Chen-Yu Tsai wens@csie.org Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Noralf Trønnes noralf@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

Den 11.07.2018 20.53, skrev Daniel Vetter:

On Wed, Jul 11, 2018 at 8:21 PM, Noralf Trønnes noralf@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@ffwll.ch

...

Fixes: c76f0f7cb546 ("drm: Begin an API for in-kernel clients") Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Icenowy Zheng icenowy@aosc.io Cc: Chen-Yu Tsai wens@csie.org Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Noralf Trønnes noralf@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