Message-ID: <1455564722.2855.61.camel@linaro.org>
Date:	Mon, 15 Feb 2016 19:32:02 +0000
From:	"Jon Medhurst (Tixy)" <tixy@...aro.org>
To:	Archit Taneja <architt@...eaurora.org>
Cc:	linux@....linux.org.uk, linux-arm-msm@...r.kernel.org,
	linux-kernel@...r.kernel.org, Daniel Stone <daniel@...ishbar.org>
Subject: Re: [RFC] component: Fix: Unassign components' masters if bringing
 up master fails

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 <architt@...eaurora.org>

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").

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.

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.

-- 
Tixy

> ---
>  drivers/base/component.c | 45 +++++++++++++++++++++++++++++----------------
>  1 file changed, 29 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index 89f5cf68..a2ecc35 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -130,6 +130,23 @@ static void remove_component(struct master *master, struct component *c)
>  			master->match->compare[i].component = NULL;
>  }
>  
> +/* Detach all components from associated master */
> +static void master_remove_components(struct master *master)
> +{
> +	struct component_match *match = master->match;
> +	size_t i;
> +
> +	if (!match)
> +		return;
> +
> +	for (i = 0; i < match->num; i++) {
> +		struct component *c = match->compare[i].component;
> +
> +		if (c)
> +			c->master = NULL;
> +	}
> +}
> +
>  /*
>   * Try to bring up a master.  If component is NULL, we're interested in
>   * this master, otherwise it's a component which must be present to try
> @@ -140,34 +157,39 @@ static void remove_component(struct master *master, struct component *c)
>  static int try_to_bring_up_master(struct master *master,
>  	struct component *component)
>  {
> -	int ret;
> +	int ret = 0;
>  
>  	dev_dbg(master->dev, "trying to bring up master\n");
>  
>  	if (find_components(master)) {
>  		dev_dbg(master->dev, "master has incomplete components\n");
> -		return 0;
> +		goto err;
>  	}
>  
>  	if (component && component->master != master) {
>  		dev_dbg(master->dev, "master is not for this component (%s)\n",
>  			dev_name(component->dev));
> -		return 0;
> +		goto err;
>  	}
>  
> -	if (!devres_open_group(master->dev, NULL, GFP_KERNEL))
> -		return -ENOMEM;
> +	if (!devres_open_group(master->dev, NULL, GFP_KERNEL)) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
>  
>  	/* Found all components */
>  	ret = master->ops->bind(master->dev);
>  	if (ret < 0) {
>  		devres_release_group(master->dev, NULL);
>  		dev_info(master->dev, "master bind failed: %d\n", ret);
> -		return ret;
> +		goto err;
>  	}
>  
>  	master->bound = true;
>  	return 1;
> +err:
> +	master_remove_components(master);
> +	return ret;
>  }
>  
>  static int try_to_bring_up_masters(struct component *component)
> @@ -324,24 +346,15 @@ 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;
> -			}
> -		}
> +		master_remove_components(master);
>  		kfree(master);
>  	}
>  	mutex_unlock(&component_mutex);

Powered by blists - more mailing lists