From patchwork Wed Feb 21 07:18:42 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Lukas Wunner X-Patchwork-Id: 10231365 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 16F9D60209 for ; Wed, 21 Feb 2018 07:18:50 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id F24FA28A4E for ; Wed, 21 Feb 2018 07:18:49 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id E4E2528A57; Wed, 21 Feb 2018 07:18:49 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id C235228A4E for ; Wed, 21 Feb 2018 07:18:48 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 2B1136E523; Wed, 21 Feb 2018 07:18:47 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from bmailout3.hostsharing.net (bmailout3.hostsharing.net [176.9.242.62]) by gabe.freedesktop.org (Postfix) with ESMTPS id D55E36E51D for ; Wed, 21 Feb 2018 07:18:44 +0000 (UTC) Received: from h08.hostsharing.net (h08.hostsharing.net [83.223.95.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "*.hostsharing.net", Issuer "COMODO RSA Domain Validation Secure Server CA" (not verified)) by bmailout3.hostsharing.net (Postfix) with ESMTPS id 92C4310334687; Wed, 21 Feb 2018 08:18:42 +0100 (CET) Received: by h08.hostsharing.net (Postfix, from userid 100393) id 41BAE31D10; Wed, 21 Feb 2018 08:18:42 +0100 (CET) Date: Wed, 21 Feb 2018 08:18:42 +0100 From: Lukas Wunner To: Jyri Sarha Subject: Re: [PATCH RFC] drm/bridge: panel: Add module_get/but calls to attached panel driver Message-ID: <20180221071842.GA4194@wunner.de> 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> <20180220120301.GC9556@ulmo> <2a58b75b-98b5-3297-1c7b-78d723f31aae@ti.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <2a58b75b-98b5-3297-1c7b-78d723f31aae@ti.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: airlied@linux.ie, "Rafael J. Wysocki" , dri-devel@lists.freedesktop.org, tomi.valkeinen@ti.com, Thierry Reding , laurent.pinchart@ideasonboard.com Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP [+cc Rafael] On Tue, Feb 20, 2018 at 05:04:00PM +0200, Jyri Sarha wrote: > On 20/02/18 14:03, Thierry Reding wrote: > > 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 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. [...] > >>> 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. > >>> > >>> struct device_link was "recently" introduced to fix that issue. > >> > >> 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. > > > > 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 = DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE; > > struct device *consumer = connector->dev->dev; > > > > link = 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. > > Thanks for the hint. Adding the link indeed unbinds the master drm > device when the panel is unloaded without any complaints. > > The annoying thing is that the master drm device does not probe again > and bind when the supplier device becomes available again. However, > forcing a manual bind brings the whole device back without a problem. > > Is there any trivial way to fix this? How about the below, would this work for you? Or is the device link added in the consumer's driver, hence is gone when the consumer is unbound? If so, it should be added somewhere else, or it shouldn't be removed on consumer unbind. @Thierry: Thanks for shifting the discussion in the right direction, it's good to see device links getting more adoption, instead of proliferating yet more kludges. However I don't understand why you say we don't have a struct device for the connector, drm_sysfs_connector_add() does create one. -- >8 -- Subject: [PATCH] driver core: Ensure consumers are probed when supplier is bound When a device link supplier is unbound (either manually or because one of its own suppliers was unbound), its consumers are unbound as well. However when that supplier is subsequently rebound, its consumers should likewise be given a chance to rebind. Achieve that by putting them on the deferred probe list in device_links_driver_bound(). The sole caller of that function, driver_bound(), triggers deferred probing afterwards. Reported-by: Jyri Sarha Cc: Thierry Reding Cc: Rafael J. Wysocki Signed-off-by: Lukas Wunner --- drivers/base/base.h | 1 + drivers/base/core.c | 4 +++- drivers/base/dd.c | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/base/base.h b/drivers/base/base.h index d800de6..39370eb 100644 --- a/drivers/base/base.h +++ b/drivers/base/base.h @@ -114,6 +114,7 @@ extern void device_release_driver_internal(struct device *dev, extern void driver_detach(struct device_driver *drv); extern int driver_probe_device(struct device_driver *drv, struct device *dev); +extern void driver_deferred_probe_add(struct device *dev); extern void driver_deferred_probe_del(struct device *dev); static inline int driver_match_device(struct device_driver *drv, struct device *dev) diff --git a/drivers/base/core.c b/drivers/base/core.c index 920af22..2869d21 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -402,7 +402,8 @@ int device_links_check_suppliers(struct device *dev) * @dev: Device to update the links for. * * The probe has been successful, so update links from this device to any - * consumers by changing their status to "available". + * consumers by changing their status to "available". Mark the consumers + * for deferred probing in case the supplier was unbound and is now rebound. * * Also change the status of @dev's links to suppliers to "active". * @@ -420,6 +421,7 @@ void device_links_driver_bound(struct device *dev) WARN_ON(link->status != DL_STATE_DORMANT); WRITE_ONCE(link->status, DL_STATE_AVAILABLE); + driver_deferred_probe_add(link->consumer); } list_for_each_entry(link, &dev->links.suppliers, c_node) { diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 2c964f5..ac5a21a 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -141,7 +141,7 @@ static void deferred_probe_work_func(struct work_struct *work) } static DECLARE_WORK(deferred_probe_work, deferred_probe_work_func); -static void driver_deferred_probe_add(struct device *dev) +void driver_deferred_probe_add(struct device *dev) { mutex_lock(&deferred_probe_mutex); if (list_empty(&dev->p->deferred_probe)) {