From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Date: Wed, 21 Oct 2015 15:16:08 +0000 Subject: Re: [PATCH] drm: rcar-du: Perform initialization/cleanup at probe/remove time Message-Id: <2416796.7aKg1mPfX6@avalon> List-Id: References: <1445295114-20921-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <20151020073213.GP13786@phenom.ffwll.local> In-Reply-To: <20151020073213.GP13786@phenom.ffwll.local> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Daniel Vetter Cc: Laurent Pinchart , dri-devel@lists.freedesktop.org, linux-sh@vger.kernel.org Hi Daniel, On Tuesday 20 October 2015 09:32:13 Daniel Vetter wrote: > On Tue, Oct 20, 2015 at 01:51:54AM +0300, Laurent Pinchart wrote: > > The drm driver .load() operation is prone to race conditions as it > > initializes the driver after registering the device nodes. Its usage is > > deprecated, inline it in the probe function and call drm_dev_alloc() and > > drm_dev_register() explicitly. > > > > For consistency inline the .unload() handler in the remove function as > > well. > > > > Signed-off-by: Laurent Pinchart > > > > --- > > > > drivers/gpu/drm/rcar-du/rcar_du_drv.c | 184 +++++++++++++----------- > > drivers/gpu/drm/rcar-du/rcar_du_hdmicon.c | 11 +- > > drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c | 11 +- > > drivers/gpu/drm/rcar-du/rcar_du_vgacon.c | 11 +- > > 4 files changed, 104 insertions(+), 113 deletions(-) > > > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c > > b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index bebcc97db5e5..46d628752371 > > 100644 > > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c [snip] > > -static int rcar_du_remove(struct platform_device *pdev) > > +static int rcar_du_probe(struct platform_device *pdev) > > { > > - struct rcar_du_device *rcdu = platform_get_drvdata(pdev); > > + struct device_node *np = pdev->dev.of_node; > > + struct rcar_du_device *rcdu; > > + struct drm_connector *connector; > > + struct drm_device *ddev; > > + struct resource *mem; > > + int ret; > > + > > + if (np = NULL) { > > + dev_err(&pdev->dev, "no device tree node\n"); > > + return -ENODEV; > > + } > > + > > + /* Allocate and initialize the DRM and R-Car device structures. */ > > + rcdu = devm_kzalloc(&pdev->dev, sizeof(*rcdu), GFP_KERNEL); > > + if (rcdu = NULL) > > + return -ENOMEM; > > + > > + init_waitqueue_head(&rcdu->commit.wait); > > > > - drm_put_dev(rcdu->ddev); > > + rcdu->dev = &pdev->dev; > > + rcdu->info = of_match_device(rcar_du_of_table, rcdu->dev)->data; > > + > > + ddev = drm_dev_alloc(&rcar_du_driver, &pdev->dev); > > + if (!ddev) > > + return -ENOMEM; > > + > > + rcdu->ddev = ddev; > > + ddev->dev_private = rcdu; > > + > > + platform_set_drvdata(pdev, rcdu); > > + > > + /* I/O resources */ > > + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + rcdu->mmio = devm_ioremap_resource(&pdev->dev, mem); > > + if (IS_ERR(rcdu->mmio)) { > > + ret = PTR_ERR(rcdu->mmio); > > + goto error; > > + } > > + > > + /* Initialize vertical blanking interrupts handling. Start with vblank > > + * disabled for all CRTCs. > > + */ > > + ret = drm_vblank_init(ddev, (1 << rcdu->info->num_crtcs) - 1); > > + if (ret < 0) { > > + dev_err(&pdev->dev, "failed to initialize vblank\n"); > > + goto error; > > + } > > + > > + /* DRM/KMS objects */ > > + ret = rcar_du_modeset_init(rcdu); > > + if (ret < 0) { > > + dev_err(&pdev->dev, "failed to initialize DRM/KMS (%d)\n", ret); > > + goto error; > > + } > > + > > + ddev->irq_enabled = 1; > > + > > + /* Register the DRM device with the core and the connectors with > > + * sysfs. > > + */ > > + ret = drm_dev_register(ddev, 0); > > + if (ret) > > + goto error; > > + > > + mutex_lock(&ddev->mode_config.mutex); > > + drm_for_each_connector(connector, ddev) { > > + ret = drm_connector_register(connector); > > + if (ret < 0) > > + break; > > + } > > + mutex_unlock(&ddev->mode_config.mutex); > > I'm wondereding whether we shouldn't just wrap this up in a helper somehow, > like drm_dev_modeset_register. How about drm_connector_plug_all() to match the existing drm_connector_unplug_all() ? > Only trouble is that we might be racing with adding MST connectors already, > and that's where I stopped thinking about it ;-) You'll have to brief me on the MST issue if you want my opinion on the matter :-) > Anyway that aside aside: > > Acked-by: Daniel Vetter Thanks. > > + > > + if (ret < 0) > > + goto error; > > + > > + DRM_INFO("Device %s probed\n", dev_name(&pdev->dev)); > > > > return 0; > > + > > +error: > > + rcar_du_remove(pdev); > > + > > + return ret; > > > > } -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH] drm: rcar-du: Perform initialization/cleanup at probe/remove time Date: Wed, 21 Oct 2015 18:16:08 +0300 Message-ID: <2416796.7aKg1mPfX6@avalon> References: <1445295114-20921-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <20151020073213.GP13786@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from galahad.ideasonboard.com (galahad.ideasonboard.com [185.26.127.97]) by gabe.freedesktop.org (Postfix) with ESMTPS id 912F3720C3 for ; Wed, 21 Oct 2015 08:16:09 -0700 (PDT) In-Reply-To: <20151020073213.GP13786@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: Laurent Pinchart , dri-devel@lists.freedesktop.org, linux-sh@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org SGkgRGFuaWVsLAoKT24gVHVlc2RheSAyMCBPY3RvYmVyIDIwMTUgMDk6MzI6MTMgRGFuaWVsIFZl dHRlciB3cm90ZToKPiBPbiBUdWUsIE9jdCAyMCwgMjAxNSBhdCAwMTo1MTo1NEFNICswMzAwLCBM YXVyZW50IFBpbmNoYXJ0IHdyb3RlOgo+ID4gVGhlIGRybSBkcml2ZXIgLmxvYWQoKSBvcGVyYXRp b24gaXMgcHJvbmUgdG8gcmFjZSBjb25kaXRpb25zIGFzIGl0Cj4gPiBpbml0aWFsaXplcyB0aGUg ZHJpdmVyIGFmdGVyIHJlZ2lzdGVyaW5nIHRoZSBkZXZpY2Ugbm9kZXMuIEl0cyB1c2FnZSBpcwo+ ID4gZGVwcmVjYXRlZCwgaW5saW5lIGl0IGluIHRoZSBwcm9iZSBmdW5jdGlvbiBhbmQgY2FsbCBk cm1fZGV2X2FsbG9jKCkgYW5kCj4gPiBkcm1fZGV2X3JlZ2lzdGVyKCkgZXhwbGljaXRseS4KPiA+ IAo+ID4gRm9yIGNvbnNpc3RlbmN5IGlubGluZSB0aGUgLnVubG9hZCgpIGhhbmRsZXIgaW4gdGhl IHJlbW92ZSBmdW5jdGlvbiBhcwo+ID4gd2VsbC4KPiA+IAo+ID4gU2lnbmVkLW9mZi1ieTogTGF1 cmVudCBQaW5jaGFydAo+ID4gPGxhdXJlbnQucGluY2hhcnQrcmVuZXNhc0BpZGVhc29uYm9hcmQu Y29tPgo+ID4gLS0tCj4gPiAKPiA+ICBkcml2ZXJzL2dwdS9kcm0vcmNhci1kdS9yY2FyX2R1X2Ry di5jICAgICB8IDE4NCAgKysrKysrKysrKysrKy0tLS0tLS0tLS0tCj4gPiAgZHJpdmVycy9ncHUv ZHJtL3JjYXItZHUvcmNhcl9kdV9oZG1pY29uLmMgfCAgMTEgKy0KPiA+ICBkcml2ZXJzL2dwdS9k cm0vcmNhci1kdS9yY2FyX2R1X2x2ZHNjb24uYyB8ICAxMSArLQo+ID4gIGRyaXZlcnMvZ3B1L2Ry bS9yY2FyLWR1L3JjYXJfZHVfdmdhY29uLmMgIHwgIDExICstCj4gPiAgNCBmaWxlcyBjaGFuZ2Vk LCAxMDQgaW5zZXJ0aW9ucygrKSwgMTEzIGRlbGV0aW9ucygtKQo+ID4gCj4gPiBkaWZmIC0tZ2l0 IGEvZHJpdmVycy9ncHUvZHJtL3JjYXItZHUvcmNhcl9kdV9kcnYuYwo+ID4gYi9kcml2ZXJzL2dw dS9kcm0vcmNhci1kdS9yY2FyX2R1X2Rydi5jIGluZGV4IGJlYmNjOTdkYjVlNS4uNDZkNjI4NzUy MzcxCj4gPiAxMDA2NDQKPiA+IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9yY2FyLWR1L3JjYXJfZHVf ZHJ2LmMKPiA+ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9yY2FyLWR1L3JjYXJfZHVfZHJ2LmMKCltz bmlwXQoKPiA+IC1zdGF0aWMgaW50IHJjYXJfZHVfcmVtb3ZlKHN0cnVjdCBwbGF0Zm9ybV9kZXZp Y2UgKnBkZXYpCj4gPiArc3RhdGljIGludCByY2FyX2R1X3Byb2JlKHN0cnVjdCBwbGF0Zm9ybV9k ZXZpY2UgKnBkZXYpCj4gPiAgewo+ID4gLQlzdHJ1Y3QgcmNhcl9kdV9kZXZpY2UgKnJjZHUgPSBw bGF0Zm9ybV9nZXRfZHJ2ZGF0YShwZGV2KTsKPiA+ICsJc3RydWN0IGRldmljZV9ub2RlICpucCA9 IHBkZXYtPmRldi5vZl9ub2RlOwo+ID4gKwlzdHJ1Y3QgcmNhcl9kdV9kZXZpY2UgKnJjZHU7Cj4g PiArCXN0cnVjdCBkcm1fY29ubmVjdG9yICpjb25uZWN0b3I7Cj4gPiArCXN0cnVjdCBkcm1fZGV2 aWNlICpkZGV2Owo+ID4gKwlzdHJ1Y3QgcmVzb3VyY2UgKm1lbTsKPiA+ICsJaW50IHJldDsKPiA+ ICsKPiA+ICsJaWYgKG5wID09IE5VTEwpIHsKPiA+ICsJCWRldl9lcnIoJnBkZXYtPmRldiwgIm5v IGRldmljZSB0cmVlIG5vZGVcbiIpOwo+ID4gKwkJcmV0dXJuIC1FTk9ERVY7Cj4gPiArCX0KPiA+ ICsKPiA+ICsJLyogQWxsb2NhdGUgYW5kIGluaXRpYWxpemUgdGhlIERSTSBhbmQgUi1DYXIgZGV2 aWNlIHN0cnVjdHVyZXMuICovCj4gPiArCXJjZHUgPSBkZXZtX2t6YWxsb2MoJnBkZXYtPmRldiwg c2l6ZW9mKCpyY2R1KSwgR0ZQX0tFUk5FTCk7Cj4gPiArCWlmIChyY2R1ID09IE5VTEwpCj4gPiAr CQlyZXR1cm4gLUVOT01FTTsKPiA+ICsKPiA+ICsJaW5pdF93YWl0cXVldWVfaGVhZCgmcmNkdS0+ Y29tbWl0LndhaXQpOwo+ID4gCj4gPiAtCWRybV9wdXRfZGV2KHJjZHUtPmRkZXYpOwo+ID4gKwly Y2R1LT5kZXYgPSAmcGRldi0+ZGV2Owo+ID4gKwlyY2R1LT5pbmZvID0gb2ZfbWF0Y2hfZGV2aWNl KHJjYXJfZHVfb2ZfdGFibGUsIHJjZHUtPmRldiktPmRhdGE7Cj4gPiArCj4gPiArCWRkZXYgPSBk cm1fZGV2X2FsbG9jKCZyY2FyX2R1X2RyaXZlciwgJnBkZXYtPmRldik7Cj4gPiArCWlmICghZGRl dikKPiA+ICsJCXJldHVybiAtRU5PTUVNOwo+ID4gKwo+ID4gKwlyY2R1LT5kZGV2ID0gZGRldjsK PiA+ICsJZGRldi0+ZGV2X3ByaXZhdGUgPSByY2R1Owo+ID4gKwo+ID4gKwlwbGF0Zm9ybV9zZXRf ZHJ2ZGF0YShwZGV2LCByY2R1KTsKPiA+ICsKPiA+ICsJLyogSS9PIHJlc291cmNlcyAqLwo+ID4g KwltZW0gPSBwbGF0Zm9ybV9nZXRfcmVzb3VyY2UocGRldiwgSU9SRVNPVVJDRV9NRU0sIDApOwo+ ID4gKwlyY2R1LT5tbWlvID0gZGV2bV9pb3JlbWFwX3Jlc291cmNlKCZwZGV2LT5kZXYsIG1lbSk7 Cj4gPiArCWlmIChJU19FUlIocmNkdS0+bW1pbykpIHsKPiA+ICsJCXJldCA9IFBUUl9FUlIocmNk dS0+bW1pbyk7Cj4gPiArCQlnb3RvIGVycm9yOwo+ID4gKwl9Cj4gPiArCj4gPiArCS8qIEluaXRp YWxpemUgdmVydGljYWwgYmxhbmtpbmcgaW50ZXJydXB0cyBoYW5kbGluZy4gU3RhcnQgd2l0aCAK dmJsYW5rCj4gPiArCSAqIGRpc2FibGVkIGZvciBhbGwgQ1JUQ3MuCj4gPiArCSAqLwo+ID4gKwly ZXQgPSBkcm1fdmJsYW5rX2luaXQoZGRldiwgKDEgPDwgcmNkdS0+aW5mby0+bnVtX2NydGNzKSAt IDEpOwo+ID4gKwlpZiAocmV0IDwgMCkgewo+ID4gKwkJZGV2X2VycigmcGRldi0+ZGV2LCAiZmFp bGVkIHRvIGluaXRpYWxpemUgdmJsYW5rXG4iKTsKPiA+ICsJCWdvdG8gZXJyb3I7Cj4gPiArCX0K PiA+ICsKPiA+ICsJLyogRFJNL0tNUyBvYmplY3RzICovCj4gPiArCXJldCA9IHJjYXJfZHVfbW9k ZXNldF9pbml0KHJjZHUpOwo+ID4gKwlpZiAocmV0IDwgMCkgewo+ID4gKwkJZGV2X2VycigmcGRl di0+ZGV2LCAiZmFpbGVkIHRvIGluaXRpYWxpemUgRFJNL0tNUyAoJWQpXG4iLCByZXQpOwo+ID4g KwkJZ290byBlcnJvcjsKPiA+ICsJfQo+ID4gKwo+ID4gKwlkZGV2LT5pcnFfZW5hYmxlZCA9IDE7 Cj4gPiArCj4gPiArCS8qIFJlZ2lzdGVyIHRoZSBEUk0gZGV2aWNlIHdpdGggdGhlIGNvcmUgYW5k IHRoZSBjb25uZWN0b3JzIHdpdGgKPiA+ICsJICogc3lzZnMuCj4gPiArCSAqLwo+ID4gKwlyZXQg PSBkcm1fZGV2X3JlZ2lzdGVyKGRkZXYsIDApOwo+ID4gKwlpZiAocmV0KQo+ID4gKwkJZ290byBl cnJvcjsKPiA+ICsKPiA+ICsJbXV0ZXhfbG9jaygmZGRldi0+bW9kZV9jb25maWcubXV0ZXgpOwo+ ID4gKwlkcm1fZm9yX2VhY2hfY29ubmVjdG9yKGNvbm5lY3RvciwgZGRldikgewo+ID4gKwkJcmV0 ID0gZHJtX2Nvbm5lY3Rvcl9yZWdpc3Rlcihjb25uZWN0b3IpOwo+ID4gKwkJaWYgKHJldCA8IDAp Cj4gPiArCQkJYnJlYWs7Cj4gPiArCX0KPiA+ICsJbXV0ZXhfdW5sb2NrKCZkZGV2LT5tb2RlX2Nv bmZpZy5tdXRleCk7Cj4gCj4gSSdtIHdvbmRlcmVkaW5nIHdoZXRoZXIgd2Ugc2hvdWxkbid0IGp1 c3Qgd3JhcCB0aGlzIHVwIGluIGEgaGVscGVyIHNvbWVob3csCj4gbGlrZSBkcm1fZGV2X21vZGVz ZXRfcmVnaXN0ZXIuCgpIb3cgYWJvdXQgZHJtX2Nvbm5lY3Rvcl9wbHVnX2FsbCgpIHRvIG1hdGNo IHRoZSBleGlzdGluZyAKZHJtX2Nvbm5lY3Rvcl91bnBsdWdfYWxsKCkgPwoKPiBPbmx5IHRyb3Vi bGUgaXMgdGhhdCB3ZSBtaWdodCBiZSByYWNpbmcgd2l0aCBhZGRpbmcgTVNUIGNvbm5lY3RvcnMg YWxyZWFkeSwKPiBhbmQgdGhhdCdzIHdoZXJlIEkgc3RvcHBlZCB0aGlua2luZyBhYm91dCBpdCA7 LSkKCllvdSdsbCBoYXZlIHRvIGJyaWVmIG1lIG9uIHRoZSBNU1QgaXNzdWUgaWYgeW91IHdhbnQg bXkgb3BpbmlvbiBvbiB0aGUgbWF0dGVyIAo6LSkKCj4gQW55d2F5IHRoYXQgYXNpZGUgYXNpZGU6 Cj4gCj4gQWNrZWQtYnk6IERhbmllbCBWZXR0ZXIgPGRhbmllbC52ZXR0ZXJAZmZ3bGwuY2g+CgpU aGFua3MuCgo+ID4gKwo+ID4gKwlpZiAocmV0IDwgMCkKPiA+ICsJCWdvdG8gZXJyb3I7Cj4gPiAr Cj4gPiArCURSTV9JTkZPKCJEZXZpY2UgJXMgcHJvYmVkXG4iLCBkZXZfbmFtZSgmcGRldi0+ZGV2 KSk7Cj4gPiAKPiA+ICAJcmV0dXJuIDA7Cj4gPiArCj4gPiArZXJyb3I6Cj4gPiArCXJjYXJfZHVf cmVtb3ZlKHBkZXYpOwo+ID4gKwo+ID4gKwlyZXR1cm4gcmV0Owo+ID4gCj4gPiAgfQoKLS0gClJl Z2FyZHMsCgpMYXVyZW50IFBpbmNoYXJ0CgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5m cmVlZGVza3RvcC5vcmcKaHR0cDovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3Rp bmZvL2RyaS1kZXZlbAo=