From mboxrd@z Thu Jan 1 00:00:00 1970 From: Archit Taneja Subject: Re: [RFC] component: Fix: Unassign components' masters if bringing up master fails Date: Tue, 16 Feb 2016 12:46:55 +0530 Message-ID: <56C2CCE7.70103@codeaurora.org> References: <1455183351-22823-1-git-send-email-architt@codeaurora.org> <1455564722.2855.61.camel@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from smtp.codeaurora.org ([198.145.29.96]:54490 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753875AbcBPHRB (ORCPT ); Tue, 16 Feb 2016 02:17:01 -0500 In-Reply-To: <1455564722.2855.61.camel@linaro.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: "Jon Medhurst (Tixy)" Cc: linux@arm.linux.org.uk, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, Daniel Stone On 02/16/2016 01:02 AM, Jon Medhurst (Tixy) wrote: > On Thu, 2016-02-11 at 15:05 +0530, Archit Taneja wrote: >> component_master_add_with_match can fail if the master's bind op doesn't >> go through successfully. In such a scenario, all the components in the >> master's match array have their 'master' pointer set to the given master. >> These pointers need to be set to NULL again. If they aren't, successive >> calls to component_master_add_with_match will fail because the driver >> thinks these components already have a master. >> >> This issue can be seen when a driver defers probe because of missing >> resources. It is seen after the introduction of commit: >> >> "component: track components via array rather than list" >> >> Add 'master_remove_components' which sets the all the components's masters >> in the match array to NULL. This function is also re-used in >> component_master_del and replaces code that did the same thing. >> >> Signed-off-by: Archit Taneja > > As Daniel pointed out in his reply, there is already a fix for this > issue in Linux which makes sure no components point to a master if it is > deleted. See commit 57480484f9f7 ("component: Detach components when > deleting master struct") > > Similarly, Daniel's fix for the mirror case has just been applied, which > makes sure masters don't refer to components when they are deleted. > Commit 8e7199c2c50f ("component: remove device from master match list on > failed add"). > I gave these fixes a try. As expected, they resolve the issue I observed. > It seems to me that for other error cases (that don't result in deletion > of objects) we would want to leave the references between components and > masters intact once they have been created. > > With regard to the $subject patch (below) it looks like it would go > wrong in this situation... > > - component_add() is called to add a component > > - This calls try_to_bring_up_masters() which calls > try_to_bring_up_master() for each master in the system > > - If that master doesn't yet have all components available yet > find_components() returned false, then > > - master_remove_components() is called > > But, this isn't an error situation that needs rolling back, and as > written the patch only half does this, because it stops components > pointing to the master, but leaves the master's match list pointing to > those components. You're right. I didn't realize this, this would have really messed things up. > > The actual real error conditions in try_to_bring_up_master() only get > triggered when actually trying to bring up a master, and that only > happens when either: > > a) The last component for that master is being added with > component_add() > > b) A master is added by component_master_add_with_match() and all the > components it required where already registered. > > Both a) and b) should now be handled correctly by the deletion of the > relevant component/master that was being added (thanks to the two fixes > I mentioned at the beginning). > > The other components or master should subsequently get cleaned up by > calling component_del() or component_master_del(), which take care of > updating the relevant references between components and master. > > For component_master_del this is not immediately obvious, but > take_down_master calls devres_release_group which causes > devm_component_match_release to be called. > Thanks for the explanation. Archit -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation