From patchwork Tue Jan 26 17:59:13 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Jon Medhurst (Tixy)" X-Patchwork-Id: 8125631 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id C43EABEEE5 for ; Tue, 26 Jan 2016 18:01:14 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D2C6F202D1 for ; Tue, 26 Jan 2016 18:01:12 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E8506202AE for ; Tue, 26 Jan 2016 18:01:10 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1aO7u5-0000Xu-F1; Tue, 26 Jan 2016 17:59:45 +0000 Received: from smarthost03b.mail.zen.net.uk ([212.23.1.21]) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1aO7tz-0000QE-RL for linux-arm-kernel@lists.infradead.org; Tue, 26 Jan 2016 17:59:41 +0000 Received: from [82.69.122.217] (helo=[192.168.2.110]) by smarthost03b.mail.zen.net.uk with esmtpsa (TLS1.2:RSA_AES_128_CBC_SHA1:128) (Exim 4.80) (envelope-from ) id 1aO7tZ-0003f8-T3; Tue, 26 Jan 2016 17:59:14 +0000 Message-ID: <1453831153.2850.107.camel@linaro.org> Subject: Problem with component helpers and probe deferral in 4.5-rc1 From: "Jon Medhurst (Tixy)" To: Russell King - ARM Linux Date: Tue, 26 Jan 2016 17:59:13 +0000 X-Mailer: Evolution 3.12.9-1+b1 Mime-Version: 1.0 X-Originating-smarthost03b-IP: [82.69.122.217] X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160126_095940_060827_A9338A26 X-CRM114-Status: GOOD ( 23.48 ) X-Spam-Score: 0.6 (/) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Greg Kroah-Hartman , Liviu Dudau , linux-arm-kernel@lists.infradead.org, dri-devel@lists.freedesktop.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-1.7 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, SUSPICIOUS_RECIPS, UNPARSEABLE_RELAY autolearn=no version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP I believe I've found a problem with the component helpers and/or how drivers use them. I discovered this whilst trying to get ARM's HDLCD driver [1] working on 4.5-rc1, however I believe that code is following a pattern used by drivers already in 4.5 and the problem isn't specific to it. This is what I have observed... The master device's probe function uses component_match_add to create a match list then it passes this to component_master_add_with_match. That creates a struct master and then calls try_to_bring_up_master which: - Calls find_components to attach all components to the master. - Calls master->ops->bind() If this bind call fails (with -EPROBE_DEFER due to missing clock in my case) then this error is returned to component_master_add_with_match which proceeds to delete the master struct. However, find_components has already attached components to that deleted master, so I think we also need something to detach components as well. I've added a patch at the end of this email which does that directly, but I wonder if instead it's the responsibility of the driver for the master to call component_master_del on error? Finally, with my scenario which has probe deferring, some time later the original master device will be probed again, repeating all the above. Except that now find_components doesn't find the components because they are already attached to a different master (the old master struct which was deleted) this results in a permanent error. Which is what lead me to investigate. [1] https://lkml.org/lkml/2015/12/22/451 The patch to detach components when master is deleted... ------------------------------------------------------------------------- From: Jon Medhurst Subject: [PATCH] component: Detach components when deleting master struct component_master_add_with_match calls find_components which, if any components already exist, it attaches to the master struct. However, if we later encounter an error the master struct is deleted, leaving components with a dangling pointer to it. If the error was a temporary one, e.g. for probe deferral, then when the master device is re-probed, it will fail to find the required components as they appear to already be attached to a master. Fix this by nulling components pointers to the master struct when it is deleted. This code is factored out into a separate function so it can be shared with component_master_del. Signed-off-by: Jon Medhurst --- drivers/base/component.c | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/drivers/base/component.c b/drivers/base/component.c index 89f5cf68..a3a1394 100644 --- a/drivers/base/component.c +++ b/drivers/base/component.c @@ -283,6 +283,24 @@ void component_match_add_release(struct device *master, } EXPORT_SYMBOL(component_match_add_release); +static void free_master(struct master *master) +{ + struct component_match *match = master->match; + int i; + + list_del(&master->node); + + if (match) { + for (i = 0; i < match->num; i++) { + struct component *c = match->compare[i].component; + if (c) + c->master = NULL; + } + } + + kfree(master); +} + int component_master_add_with_match(struct device *dev, const struct component_master_ops *ops, struct component_match *match) @@ -309,11 +327,9 @@ int component_master_add_with_match(struct device *dev, ret = try_to_bring_up_master(master, NULL); - if (ret < 0) { - /* Delete off the list if we weren't successful */ - list_del(&master->node); - kfree(master); - } + if (ret < 0) + free_master(master); + mutex_unlock(&component_mutex); return ret < 0 ? ret : 0; @@ -324,25 +340,12 @@ void component_master_del(struct device *dev, const struct component_master_ops *ops) { struct master *master; - int i; mutex_lock(&component_mutex); master = __master_find(dev, ops); if (master) { - struct component_match *match = master->match; - take_down_master(master); - - list_del(&master->node); - - if (match) { - for (i = 0; i < match->num; i++) { - struct component *c = match->compare[i].component; - if (c) - c->master = NULL; - } - } - kfree(master); + free_master(master); } mutex_unlock(&component_mutex); }