Received: by 10.192.165.148 with SMTP id m20csp4837306imm; Tue, 24 Apr 2018 09:08:56 -0700 (PDT) X-Google-Smtp-Source: AIpwx4+hKbjf2AwsN4FkY/eHVEwXxfksWUG3YLiQN7rDQBhEya8LKzu8W3/pC+/56bUBoO9zD0+r X-Received: by 10.101.90.194 with SMTP id d2mr21037236pgt.352.1524586136278; Tue, 24 Apr 2018 09:08:56 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1524586136; cv=none; d=google.com; s=arc-20160816; b=qVF1CHh/sP9UXNnKQj+z+QJgkW3SY4mbo/U3lsZTz06Sba3zs2sEiXZVw3HtdiTEPq Xez4IrgYpVjzEof+yZUCdajodOvFeK9kPIXzqEjaQExyasFE4qzYKX2krGGTe2DD8EQf H42ZcncRaWtj8l8soOR8DefAHtbzanUTOCs1SiAYrYNldnBUasbJO/+PXQjySipRUh1W dBUSAolSrCFJfAeWHLfvnqYfyO6zlgsU8OeLrsN7UiXthPGRnk11Po0NG7hy2xxSMziH +qrley8R4Q0BWi2sppF8FS9wfnzZGXGBo3/2sp+WlDHhsl8UfxSZWi6k+1mxb+kKLzk2 suQw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:autocrypt:openpgp:from:references:cc:to:subject :dkim-signature:arc-authentication-results; bh=H3bYzHPbqpw4/aQXETKSlCavnOXNRaIXHMKy4X+u/AU=; b=jfpZhHTR1AO9NM8oCSDbsEv7vTCaFtLkeyeB4FXezxRIZ4LQmENnjfAmFQ35jp4bHv Uv4/BfaIyDHAnhKZuLNxn5460nl1KZRLL0YDLFQadAnGCGvJaVaHQc7LE7iXIBqPf7Lv tTBmVpWNN3X9dPVBtpJUWD3mr4LwWPNgxQ2UkO5vmDA2zyJ+Qvg908MAfJ+31IEgjoCi sW+AKGeoK7xIz+S9eby1DiEZqefxIJibTZWQBBm/PC0ksbfYigk3VCr98VUvOMN76rH+ YTM1ftNuSzFRCtPa+pl2cI2jGCEGA9uYxUZ9ptMmbmaUj1R/3ZKxSliJOgl9Ub2LgNDa 3tXg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@ti.com header.s=ti-com-17Q1 header.b=kQ4UAh8h; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=NONE dis=NONE) header.from=ti.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id n1-v6si14120315pld.280.2018.04.24.09.08.41; Tue, 24 Apr 2018 09:08:56 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@ti.com header.s=ti-com-17Q1 header.b=kQ4UAh8h; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=NONE dis=NONE) header.from=ti.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751517AbeDXQFS (ORCPT + 99 others); Tue, 24 Apr 2018 12:05:18 -0400 Received: from lelnx194.ext.ti.com ([198.47.27.80]:13054 "EHLO lelnx194.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750779AbeDXQFO (ORCPT ); Tue, 24 Apr 2018 12:05:14 -0400 Received: from dlelxv90.itg.ti.com ([172.17.2.17]) by lelnx194.ext.ti.com (8.15.1/8.15.1) with ESMTP id w3OG4Ltl014593; Tue, 24 Apr 2018 11:04:21 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ti.com; s=ti-com-17Q1; t=1524585861; bh=CLRSW7q5NoEzc20bQ68fraqIa4/5eq+PRzNpOh+uRRc=; h=Subject:To:CC:References:From:Date:In-Reply-To; b=kQ4UAh8hG4G8KiQ1yY6HSDW/l+Sqs29o5UZtc5Cv8RJ81KTIT55sin+/n/x+R3W/R Hh4nIGR1EwQUUSU+9yFmN7a70R8SxBLIeWVX8Q79Ej9RZK45uXjBLWP4cP9JZ3dQHw wwXyQEq2631vmuaFQSoaG9agq9fv8r4DBNJ29Yfo= Received: from DFLE100.ent.ti.com (dfle100.ent.ti.com [10.64.6.21]) by dlelxv90.itg.ti.com (8.14.3/8.13.8) with ESMTP id w3OG4L46012950; Tue, 24 Apr 2018 11:04:21 -0500 Received: from DFLE111.ent.ti.com (10.64.6.32) by DFLE100.ent.ti.com (10.64.6.21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1466.3; Tue, 24 Apr 2018 11:04:21 -0500 Received: from dlep32.itg.ti.com (157.170.170.100) by DFLE111.ent.ti.com (10.64.6.32) with Microsoft SMTP Server (version=TLS1_0, cipher=TLS_RSA_WITH_AES_256_CBC_SHA) id 15.1.1466.3 via Frontend Transport; Tue, 24 Apr 2018 11:04:20 -0500 Received: from [10.1.3.6] (ileax41-snat.itg.ti.com [10.172.224.153]) by dlep32.itg.ti.com (8.14.3/8.13.8) with ESMTP id w3OG4Htw011204; Tue, 24 Apr 2018 11:04:17 -0500 Subject: Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge To: Peter Rosin , Russell King - ARM Linux CC: , David Airlie , Rob Herring , Mark Rutland , Nicolas Ferre , Alexandre Belloni , Boris Brezillon , Tomi Valkeinen , Laurent Pinchart , Jacopo Mondi , , , References: <20180423072301.11962-1-peda@axentia.se> <20180423072301.11962-8-peda@axentia.se> <20180423160833.GF16141@n2100.armlinux.org.uk> <5d6866d0-75bc-4de8-9b87-4fee5430e9dd@axentia.se> <20180424080833.GJ16141@n2100.armlinux.org.uk> <8448e90a-4562-b564-c160-1b5c67e0f92f@axentia.se> From: Jyri Sarha Openpgp: preference=signencrypt Autocrypt: addr=jsarha@ti.com; prefer-encrypt=mutual; keydata= xsFNBFbdWt8BEADnCIkQrHIvAmuDcDzp1h2pO9s22nacEffl0ZyzIS//ruiwjMfSnuzhhB33 fNEWzMjm7eqoUBi1BUAQIReS6won0cXIEXFg9nDYQ3wNTPyh+VRjBvlb/gRJlf4MQnJDTGDP S5i63HxYtOfjPMSsUSu8NvhbzayNkN5YKspJDu1cK5toRtyUn1bMzUSKDHfwpdmuCDgXZSj2 t+z+c6u7yx99/j4m9t0SVlaMt00p1vJJ3HJ2Pkm3IImWvtIfvCmxnOsK8hmwgNQY6PYK1Idk puSRjMIGLqjZo071Z6dyDe08zv6DWL1fMoOYbAk/H4elYBaqEsdhUlDCJxZURcheQUnOMYXo /kg+7TP6RqjcyXoGgqjfkqlf3hYKmyNMq0FaYmUAfeqCWGOOy3PPxR/IiACezs8mMya1XcIK Hk/5JAGuwsqT80bvDFAB2XfnF+fNIie/n5SUHHejJBxngb9lFE90BsSfdcVwzNJ9gVf/TOJc qJEHuUx0WPi0taO7hw9+jXV8KTHp6CQPmDSikEIlW7/tJmVDBXQx8n4RMUk4VzjE9Y/m9kHE UVJ0bJYzMqECMTAP6KgzgkQCD7n8OzswC18PrK69ByGFpcm664uCAa8YiMuX92MnesKMiYPQ z1rvR5riXZdplziIRjFRX+68fvhPverrvjNVmzz0bAFwfVjBsQARAQABzRpKeXJpIFNhcmhh IDxqc2FyaGFAdGkuY29tPsLBeAQTAQIAIgUCVt1a3wIbAwYLCQgHAwIGFQgCCQoLBBYCAwEC HgECF4AACgkQkDazUNfWGUEVVhAAmFL/21tUhZECrDrP9FWuAUuDvg+1CgrrqBj7ZxKtMaiz qTcZwZdggp8bKlFaNrmsyrBsuPlAk99f7ToxufqbV5l/lAT3DdIkjb4nwN4rJkxqSU3PaUnh mDMKIAp6bo1N9L+h82LE6CjI89W4ydQp5i+cOeD/kbdxbHHvxgNwrv5x4gg1JvEQLVnUSHva R2kx7u2rlnq7OOyh9vU0MUq7U5enNNqdBjjBTeaOwa5xb3S2Cc9dR10mpFiy+jSSkuFOjPpc fLfr/s03NGqbZ4aXvZCGjCw4jclpTJkuWPKO+Gb+a/3oJ4qpGN9pJ+48n2Tx9MdSrR4aaXHi EYMrbYQz9ICJ5V80P5+yCY5PzCvqpkizP6vtKvRSi8itzsglauMZGu6GwGraMJNBgu5u+HIZ nfRtJO1AAiwuupOHxe1nH05c0zBJaEP4xJHyeyDsMDh+ThwbGwQmAkrLJZtOd3rTmqlJXnuj sfgQlFyC68t1YoMHukz9LHzg02xxBCaLb0KjslfwuDUTPrWtcDL1a5hccksrkHx7k9crVFA1 o6XWsOPGKRHOGvYyo3TU3CRygXysO41UnGG40Q3B5R8RMwRHV925LOQIwEGF/6Os8MLgFXCb Lv3iJtan+PBdqO1Bv3u2fXUMbYgQ3v7jHctB8nHphwSwnHuGN7FAmto+SxzotE3OwU0EVt1a 3wEQAMHwOgNaIidGN8UqhSJJWDEfF/SPSCrsd3WsJklanbDlUCB3WFP2EB4k03JroIRvs7/V VMyITLQvPoKgaECbDS5U20r/Po/tmaAOEgC7m1VaWJUUEXhjYQIw7t/tSdWlo5XxZIcO4LwO Kf0S4BPrQux6hDLIFL8RkDH/8lKKc44ZnSLoF1gyjc5PUt6iwgGJRRkOD8gGxCv1RcUsu1xU U9lHBxdWdPmMwyXiyui1Vx7VJJyD55mqc7+qGrpDHG9yh3pUm2IWp7jVt/qw9+OE9dVwwhP9 GV2RmBpDmB3oSFpk7lNvLJ11VPixl+9PpmRlozMBO00wA1W017EpDHgOm8XGkq++3wsFNOmx 6p631T2WuIthdCSlZ2kY32nGITWn4d8L9plgb4HnDX6smrMTy1VHVYX9vsHXzbqffDszQrHS wFo5ygKhbGNXO15Ses1r7Cs/XAZk3PkFsL78eDBHbQd+MveApRB7IyfffIz7pW1R1ZmCrmAg Bn36AkDXJTgUwWqGyJMd+5GHEOg1UPjR5Koxa4zFhj1jp1Fybn1t4N11cmEmWh0aGgI/zsty g/qtGRnFEywBbzyrDEoV4ZJy2Q5pnZohVhpbhsyETeYKQrRnMk/dIPWg6AJx38Cl4P9PK1JX 8VK661BG8GXsXJ3uZbPSu6K0+FiJy09N4IW7CPJNABEBAAHCwV8EGAECAAkFAlbdWt8CGwwA CgkQkDazUNfWGUFOfRAA5K/z9DXVEl2kkuMuIWkgtuuLQ7ZwqgxGP3dMA5z3Iv/N+VNRGbaw oxf+ZkTbJHEE/dWclj1TDtpET/t6BJNLaldLtJ1PborQH+0jTmGbsquemKPgaHeSU8vYLCdc GV/Rz+3FN0/fRdmoq2+bIHght4T6KZJ6jsrnBhm7y6gzjMOiftH6M5GXPjU0/FsU09qsk/af jbwLETaea0mlWMrLd9FC2KfVITA/f/YG2gqtUUF9WlizidyctWJqSTZn08MdzaoPItIkRUTv 6Bv6rmFn0daWkHt23BLd0ZP7e7pON1rqNVljWjWQ/b/E/SzeETrehgiyDr8pP+CLlC+vSQxi XtjhWjt1ItFLXxb4/HLZbb/L4gYX7zbZ3NwkON6Ifn3VU7UwqxGLmKfUwu/mFV+DXif1cKSS v6vWkVQ6Go9jPsSMFxMXPA5317sZZk/v18TAkIiwFqda3/SSjwc3e8Y76/DwPvUQd36lEbva uBrUXDDhCoiZnjQaNz/J+o9iYjuMTpY1Wp+igjIretYr9+kLvGsoPo/kTPWyiuh/WiFU2d6J PMCGFGhodTS5qmQA6IOuazek1qSZIl475u3E2uG98AEX/kRhSzgpsbvADPEUPaz75uvlmOCX tv+Sye9QT4Z1QCh3lV/Zh4GlY5lt4MwYnqFCxroK/1LpkLgdyQ4rRVw= Message-ID: <0cbee3bd-8987-5f2f-519d-f8d1b426f2a3@ti.com> Date: Tue, 24 Apr 2018 19:04:16 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <8448e90a-4562-b564-c160-1b5c67e0f92f@axentia.se> Content-Type: text/plain; charset="utf-8" Content-Language: en-GB Content-Transfer-Encoding: 8bit X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 24/04/18 13:14, Peter Rosin wrote: > On 2018-04-24 10:08, Russell King - ARM Linux wrote: >> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote: >>> On 2018-04-23 18:08, Russell King - ARM Linux wrote: >>>> On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote: >>>>> static int tda998x_remove(struct i2c_client *client) >>>>> { >>>>> - component_del(&client->dev, &tda998x_ops); >>>>> + struct device *dev = &client->dev; >>>>> + struct tda998x_bridge *bridge = dev_get_drvdata(dev); >>>>> + >>>>> + drm_bridge_remove(&bridge->bridge); >>>>> + component_del(dev, &tda998x_ops); >>>>> + >>>> >>>> I'd like to ask a rather fundamental question about DRM bridge support, >>>> because I suspect that there's a major fsckup here. >>>> >>>> The above is the function that deals with the TDA998x device being >>>> unbound from the driver. With the component API, this results in the >>>> DRM device correctly being torn down, because one of the hardware >>>> devices has gone. >>>> >>>> With DRM bridge, the bridge is merely removed from the list of >>>> bridges: >>>> >>>> void drm_bridge_remove(struct drm_bridge *bridge) >>>> { >>>> mutex_lock(&bridge_lock); >>>> list_del_init(&bridge->list); >>>> mutex_unlock(&bridge_lock); >>>> } >>>> EXPORT_SYMBOL(drm_bridge_remove); >>>> >>>> and the memory backing the "struct tda998x_bridge" (which contains >>>> the struct drm_bridge) will be freed by the devm subsystem. >>>> >>>> However, there is no notification into the rest of the DRM subsystem >>>> that the device has gone away. Worse, the memory that is still in >>>> use by DRM has now been freed, so further use of the DRM device >>>> results in a use-after-free bug. >>>> >>>> This is really not good, and to me looks like a fundamental problem >>>> with the DRM bridge code. I see nothing in the DRM bridge code that >>>> deals with the lifetime of a "DRM bridge" or indeed the lifetime of >>>> the actual device itself. >>>> >>>> So, from what I can see, there seems to be a fundamental lifetime >>>> issue with the design of the DRM bridge code. This needs to be >>>> fixed. >>> >>> Oh crap. A gigantic can of worms... >> >> Yes, it's especially annoying for me, having put the effort in to >> the component helper to cover all these cases. >> >>> Would a patch (completely untested btw) along this line of thinking make >>> any difference whatsoever? >> >> It looks interesting - from what I can see of the device links code, >> it would have the effect of unbinding the DRM device just before >> TDA998x is unbound, so that's an improvement. >> >> However, from what I can see, the link vanishes at that point (as >> DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results >> in nothing further happening - the link will be recreated, but there >> appears to be nothing that triggers the "consumer" to rebind at that >> point. Maybe I've missed something? > > Right, auto-remove is a no-go. So, improving on the previous... > > (I think drm_panel might suffer from this issue too?) Yes it does and I took a shot at trying to fix it at the end of the previous merge window, but gave up as I run out of time. I re-spun the work now after reading this thread. I add you and Russell to cc. As far as I understand the re-binding problem can be solved with this patch: https://lists.freedesktop.org/archives/dri-devel/2018-February/166907.html The device_links are such a new concept that there is at least hope this change won't have too unwanted side effects. > > Cheers, > Peter > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index 1638bfe9627c..b1365cfee445 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -121,12 +121,17 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, > if (bridge->dev) > return -EBUSY; > > + bridge->link = device_link_add(encoder->dev->dev, bridge->owner, 0); > + if (!bridge->link) > + return -EINVAL; > + At least this piece code should prepare for the bridge owner and the master drm device being the same, I which case the link should not be needed. This happens at least when a panel is attached using drm_panel_bridge_add(). Best reagards, Jyri > bridge->dev = encoder->dev; > bridge->encoder = encoder; > > if (bridge->funcs->attach) { > ret = bridge->funcs->attach(bridge); > if (ret < 0) { > + device_link_del(bridge->link); > bridge->dev = NULL; > bridge->encoder = NULL; > return ret; > @@ -153,6 +158,8 @@ void drm_bridge_detach(struct drm_bridge *bridge) > if (bridge->funcs->detach) > bridge->funcs->detach(bridge); > > + device_link_del(bridge->link); > + > bridge->dev = NULL; > } > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c > index b8cb6237a38b..29eba4e9a39d 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > @@ -1857,6 +1857,7 @@ tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id) > bridge->dev = dev; > dev_set_drvdata(dev, bridge); > > + bridge->bridge.owner = dev; > bridge->bridge.funcs = &tda998x_bridge_funcs; > #ifdef CONFIG_OF > bridge->bridge.of_node = dev->of_node; > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index 682d01ba920c..b8f33aba3216 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -224,6 +224,8 @@ struct drm_bridge_funcs { > > /** > * struct drm_bridge - central DRM bridge control structure > + * @owner: device that owns the bridge > + * @link: drm consumer <-> bridge supplier > * @dev: DRM device this bridge belongs to > * @encoder: encoder to which this bridge is connected > * @next: the next bridge in the encoder chain > @@ -233,6 +235,8 @@ struct drm_bridge_funcs { > * @driver_private: pointer to the bridge driver's internal context > */ > struct drm_bridge { > + struct device *owner; > + struct device_link *link; > struct drm_device *dev; > struct drm_encoder *encoder; > struct drm_bridge *next; > > -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki