From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH RFC] drm/bridge: panel: Add module_get/but calls to attached panel driver Date: Tue, 20 Feb 2018 13:03:01 +0100 Message-ID: <20180220120301.GC9556@ulmo> References: <1519070782-20834-1-git-send-email-jsarha@ti.com> <20180219225923.GG22199@phenom.ffwll.local> <20180220103453.GH23425@ulmo> <864d2538-97ee-3060-efd3-c0accb662e70@ti.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1332562795==" Return-path: Received: from mail-qk0-x242.google.com (mail-qk0-x242.google.com [IPv6:2607:f8b0:400d:c09::242]) by gabe.freedesktop.org (Postfix) with ESMTPS id 8AD066E3C2 for ; Tue, 20 Feb 2018 12:03:05 +0000 (UTC) Received: by mail-qk0-x242.google.com with SMTP id s188so6351771qkb.2 for ; Tue, 20 Feb 2018 04:03:05 -0800 (PST) In-Reply-To: <864d2538-97ee-3060-efd3-c0accb662e70@ti.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Jyri Sarha Cc: airlied@linux.ie, tomi.valkeinen@ti.com, dri-devel@lists.freedesktop.org, laurent.pinchart@ideasonboard.com List-Id: dri-devel@lists.freedesktop.org --===============1332562795== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Bu8it7iiRSEf40bY" Content-Disposition: inline --Bu8it7iiRSEf40bY Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Feb 20, 2018 at 01:28:48PM +0200, Jyri Sarha wrote: > On 20/02/18 12:34, Thierry Reding wrote: > > On Mon, Feb 19, 2018 at 11:59:23PM +0100, Daniel Vetter wrote: > >> On Mon, Feb 19, 2018 at 10:06:22PM +0200, Jyri Sarha wrote: > >>> Currently there is no way for a master drm driver to protect against = an > >>> attached panel driver from being unloaded while it is in use. The > >>> least we can do is to indicate the usage by incrementing the module > >>> reference count. > >>> > >>> Signed-off-by: Jyri Sarha > >>> cc: eric@anholt.net > >>> cc: laurent.pinchart@ideasonboard.com > >>> --- > >>> I do not see any module_get/put code in drm core. Is there is a reason > >>> for that? > >>> > >>> There is two more alternative places for adding the module_get/put > >>> code. One is puting it directly to drm_panel_attach() and > >>> drm_panel_detach(). However, if the same module implements both the > >>> master drm driver and the panel (like tilcdc does with its > >>> tilcdc_panel.c), then attaching the panel will lock the module in for > >>> no good reason. Still, this solution should work with drm bridges as I > >>> do not see any reason why anybody would implement bridge drivers in > >>> the same module with the master drm driver. > >>> > >>> The other place to put the code would in the master drm driver. But > >>> for handling the situation with bridges would need the device pointer > >>> in struct drm_bridge. > >> > >> I think this looks like a reasonable place to do this. Looking at the = code > >> we seem to have a similar issue with the bridge driver itself. I think > >> we need to wire through the module owner stuff and add a try_modeul_ge= t to > >> of_drm_find_bridge (and any other helper used to find bridge instances= ). > >=20 > > I disagree. module_get() is only going to protect you from unloading a > > module that's in use, but there are other ways to unbind a driver from > > the device and cause subsequent mayhem. > >=20 >=20 > Yes. This is not a full fix for the issue. But AFAIK at the moment there > is no infrastructure to tear down the whole drm device grace fully when > a bridge driver or panel is removed. So this should be considered as a > partial remedy protecting user from accidentally crashing the drm driver > by unloading the wrong module first. That is why I wrote "least we can do= ". >=20 > > struct device_link was "recently" introduced to fix that issue. > >=20 >=20 > Unfortunately I do not have time to do full fix for the issue, but in > any case I think this patch is a step to the right direction. The module > reference count should anyway be increased at least to know that the > driver code remains in memory, even if the device is not there any more. >=20 > Then again the display panels or bridges are hardly ever hot pluggable > devices, so they do not normally vanish just like that, unless someone > is being nasty and forcibly unbinding them. But yes, I know that is a > bad excuse for broken behaviour. I think device links are actually a lot easier to use and give you a full solution for this. Essentially the only thing you need to do is add a call to device_link_add() in the correct place. That said, it seems to me like drm_panel_bridge_add() is not a good place to add this, because the panel/bridge does not follow a typical consumer/supplier model. It is rather a helper construct with a well- defined lifetime. What you do want to protect is the panel (the "parent" of the panel/ bridge) from going away unexpectedly. I think the best way to achieve that is to make the link in drm_panel_attach() with something like this: u32 flags =3D DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE; struct device *consumer =3D connector->dev->dev; link =3D device_link_add(consumer, panel->dev, flags); if (!link) { dev_err(panel->dev, "failed to link panel %s to %s\n", dev_name(panel->dev), dev_name(consumer)); return -EINVAL; } Ideally I think we'd link the panel to the connector rather than the top-level DRM device, but we don't have a struct device * for that, so this is probably as good as it gets for now. You might get away without the DL_FLAG_PM_RUNTIME because DRM should handle that properly already. Thierry --Bu8it7iiRSEf40bY Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAlqMDnIACgkQ3SOs138+ s6FbIw/9EWiCv8OoSjygMUFayBJNNRrWEsO3nBCih1LfPLcj0TZCHS9syUvWJhkd +UPsNPKektuPu4RiJ6wph1V/8zP/LEMtAJ8JaqS7Gj5jKV+VjyRMce5vARpxHsDY 246qD/vQuELIYmm5IdkRXPrx4iONXJQ+r3pXPp2fY6i3CqVIf+ax/hWUaPtEkw2K 497UXIx+eO8DV/FvfZfbRpnglMxLYmLINh/DK69NmIrvZdkCxtsK7SI+9AQsvNpG 9fqFWUKtvMv9BvWzDN0h9ohJyK8gbNA2kSw+OM5vngOogs0xptj3v5oYYRac+eEN MT9sWNp1OrgIXf9czirHCS9nvLOtAPyCTg7y2FgJki64I/SAxd/GvEUzi91UU5xE 16jIOYe7v5jjB9Yf9oIbw6K0oEc+J1EhUMuENn9NtunlQhjk1ofs+bPE8NfHpyyT Qjp1PudKQ+bxQrefif/2XqTlQUHbjDcWeJ9x/7oKS7GQYNbJmkKdH0GqRgwIrCUq mppsN36zY7YzcOiYUAJWUdNWwfAaRt0axhsKXqV43T+exBjNAkUijLWYcIA5iiAg 5yZBj0vyk4zv0Grhj7c5Xa8gdoWsCG7SBeQa0oOtc8NyMhND4Lkn2ywsvPZSWz6k hgsIpk9idiWSZ9h3kEpJx9ZPRXFrowsz6TZoqwjuZwnTZ5r55Mw= =r8/b -----END PGP SIGNATURE----- --Bu8it7iiRSEf40bY-- --===============1332562795== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============1332562795==--