Received: by 10.192.165.148 with SMTP id m20csp1128392imm; Wed, 25 Apr 2018 13:03:56 -0700 (PDT) X-Google-Smtp-Source: AIpwx4++ExOKZ+8jh62NFtP8X6Qpa3IA43Akl/zUgiMaiC9nOTYo19R7A+e0Ljcwv8OjOQiipFII X-Received: by 10.98.13.71 with SMTP id v68mr29129379pfi.69.1524686636127; Wed, 25 Apr 2018 13:03:56 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1524686636; cv=none; d=google.com; s=arc-20160816; b=WAADhYrO9Mi83Z/DxdCahbqg1wgC10rRfLrmU38gaupMsQl2pjIYzuShLKnUV5JXqb D/RkNUWmTiXK2AdZQZKwebdepB4RnWa9gRM4mi+bVtDhlUmXA2fBeyRuKAbbO2XGv+Nc JXZ0ceJflriupAj+K676uese7Z71XQivW0dr8U7NmoxgXfiK5EDV5h2q/IwZtjJqlFZ7 vEcU4i7wZaPw/+w1XcRwo167t2/G+G+jl+FfHB7saAaolr8C2LWr+8M+F/nxPCL/Wgqn BVrgFwGe1hFHsW3Q1PvHMiDuItm3XwZUS4KJBMbYXWbYOm5Dlqgu3zq4lrl1WXaU74qu OHeA== 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:references:cc:to:subject:from :dkim-signature:arc-authentication-results; bh=cy6TruFUG28Y8QFrIaBk5tSsIKhF5r/k+up4LzVhnGo=; b=zz85+RRGo80E2NOIfduWKNLUvwnjVHTKCjdh6kdtjCXASLWvHKsTS08nX/PCGk04P3 7pYHgPsC3gesrvJk2EY3rx1KS9XmjvxzNb4QDS1fTBjy41KRWnvFGGjHB3rqE98YmD1z 43kPERpA/ksznXHLjIfpaq8UfKgpOlFMhVN/+nMBWS7AdIqgk+2kSZCnixlpjNHv9XfG QM/7k57ajXXtG3LGrh5tMeeUJMMicQZh6SGNU3AlG9lWYghAbfzEhxKGB2xvJjAFTkp9 VABq1M4qMMJzeUZGXkOULg/wgJTx2qbHSUIEpgOky3vwNE3RUkcRf15+PGdLCNM/vrVx 8QfQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@ti.com header.s=ti-com-17Q1 header.b=IiXil2f7; 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 b1si5429182pgr.464.2018.04.25.13.03.41; Wed, 25 Apr 2018 13:03: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=IiXil2f7; 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 S1756309AbeDYUCI (ORCPT + 99 others); Wed, 25 Apr 2018 16:02:08 -0400 Received: from fllnx209.ext.ti.com ([198.47.19.16]:31688 "EHLO fllnx209.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751750AbeDYUCF (ORCPT ); Wed, 25 Apr 2018 16:02:05 -0400 Received: from dlelxv90.itg.ti.com ([172.17.2.17]) by fllnx209.ext.ti.com (8.15.1/8.15.1) with ESMTP id w3PK1Kur001479; Wed, 25 Apr 2018 15:01:20 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ti.com; s=ti-com-17Q1; t=1524686480; bh=f3WTEWZPp6zb7rVroLN71FsZ4wa//C4YQjp1hVKolHY=; h=From:Subject:To:CC:References:Date:In-Reply-To; b=IiXil2f7gOEMuVPJwdw8cD/k68ekJpTnHMon4EuUFWTmZIkpi3fKRrqtegFK6vfQM E2bXS8dga0ZJ/lbXssHHAe3MjM/AcmCOzitomtq/VMq3e20WRRKJJ0kx0+Av4xVBD0 nZ5g74Dx5DYn01oS+IRrnPsrmLz9dDxqh5j6RChw= Received: from DLEE103.ent.ti.com (dlee103.ent.ti.com [157.170.170.33]) by dlelxv90.itg.ti.com (8.14.3/8.13.8) with ESMTP id w3PK1K8E003386; Wed, 25 Apr 2018 15:01:20 -0500 Received: from DLEE115.ent.ti.com (157.170.170.26) by DLEE103.ent.ti.com (157.170.170.33) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1466.3; Wed, 25 Apr 2018 15:01:19 -0500 Received: from dflp32.itg.ti.com (10.64.6.15) by DLEE115.ent.ti.com (157.170.170.26) with Microsoft SMTP Server (version=TLS1_0, cipher=TLS_RSA_WITH_AES_256_CBC_SHA) id 15.1.1466.3 via Frontend Transport; Wed, 25 Apr 2018 15:01:19 -0500 Received: from [10.1.3.6] (ileax41-snat.itg.ti.com [10.172.224.153]) by dflp32.itg.ti.com (8.14.3/8.13.8) with ESMTP id w3PK1FlR002638; Wed, 25 Apr 2018 15:01:16 -0500 From: Jyri Sarha Subject: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge To: Russell King - ARM Linux CC: Peter Rosin , , 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> <0cbee3bd-8987-5f2f-519d-f8d1b426f2a3@ti.com> <20180424170625.GL16141@n2100.armlinux.org.uk> <20180424232523.GN16141@n2100.armlinux.org.uk> 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: <7e8cdcf6-c816-1e01-24f1-a526fb0a591f@ti.com> Date: Wed, 25 Apr 2018 23:01:15 +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: <20180424232523.GN16141@n2100.armlinux.org.uk> Content-Type: text/plain; charset="utf-8" Content-Language: en-GB Content-Transfer-Encoding: 7bit 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 25/04/18 02:25, Russell King - ARM Linux wrote: > On Tue, Apr 24, 2018 at 09:25:44PM +0300, Jyri Sarha wrote: >> On 24/04/18 20:06, Russell King - ARM Linux wrote: >>> On Tue, Apr 24, 2018 at 07:04:16PM +0300, Jyri Sarha wrote: >>>> 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. >>> >>> Right, and these exact problems are what the component helper is >>> there to sort out, in a subsystem independent way. >>> >>> What is the problem with the component helper that people seem to >>> be soo loathed to use it, instead preferring to come up with sub- >>> standard and broken alternatives? >>> >> >> Nothing but time. Embedding component helpers seamlessly into drm >> framework does not sound like a couple of days job. Right now I simply >> do not have time to take on a challenge like that. If someone does it I >> am all for it. >> >> However, I would not call device links substandard. They are in the >> device core after all. > > Umm, no, I was not talking about the device links, but the tendency to > have subsystem or component specific solutions to this problem. > Oh yes. But in this case the substandard solution is already there and it is already widely used, despite it being severely broken. I am merely trying to fix the existing substandard solution. I admit that a full integration with component helpers would probably be more elegant solution to the problem, but the amount of work is just too much. The change would impact the way all the master drm drivers pull them selves together. The drivers that already use the component helpers for some internal stuff will add their own challenge. Separate component matching implementations are needed for device-tree and ACPI (are ther more flavors?) etc. I just do not see this happening any time soon (am happy to be wrong about this). > We're now heading down the path of trying to retrofit the functionality > that one expects and is provided by the component helpers into DRM > bridge and DRM panel by using device links, which appears to only > partially resolve the problem. > > On the point of device links, I've been wondering whether the component > helpers could take advantage of the device links, but at the moment > that would cause a regression if there's no facility to re-probe the > "consumer" when a "supplier" returns. > I sent a patch[1] in February that solves the re-probing problem at least in my environment. > As you bring that up, I would say that they _are_ a substandard way to > solve this problem _if_, as I suspect, they would cause a regression > to the component helper if the component helper were to use them as a > means to control the probing of the "master" device. This is not a > matter of which part of the kernel they are "in", but the functionality > that they can offer vs what would be expected. > The component helpers certainly have many features in that are not (yet?) in device core's device links, but the amount of work needed to fix the most pressing problem of attached drm panels or bridges is couple of magnitudes less with device links when compared to full blown component helper usage. Best regards, Jyri [1] https://lists.freedesktop.org/archives/dri-devel/2018-February/166907.html -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki