[RFC] component: Fix: Unassign components' masters if bringing up master fails

[RFC] component: Fix: Unassign components' masters if bringing up master fails

[Archit Taneja]

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@codeaurora.org>
---
 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);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [RFC] component: Fix: Unassign components' masters if bringing up master fails

[Daniel Stone]

Hi Archit,

On 11 February 2016 at 09:35, Archit Taneja <architt@codeaurora.org> 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.

Jon already fixed this (in a slightly more limited way perhaps?) in 57480484f9.

Cheers,
Daniel

Re: [RFC] component: Fix: Unassign components' masters if bringing up master fails

[Jon Medhurst (Tixy)]

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@codeaurora.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);

Re: [RFC] component: Fix: Unassign components' masters if bringing up master fails

[Russell King - ARM Linux]

On Mon, Feb 15, 2016 at 07:32:02PM +0000, Jon Medhurst (Tixy) wrote:
> 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.

Indeed we do - because we want to avoid having to redo the matching
work each and every time we try to bring up the master.  It's needless
expense to keep re-running all the matches every time.

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

Correct.

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

Also correct.

For component_master_del(), the list of components will be going away,
so there's no point cleaning that list (it's freed when the device
model releases its devres group.)

However, the components themselves must not be left pointing at the
freed memory, otherwise they'll effectively be marked "in-use" by a
non-existent master - that's what "free_master()" is about - ensuring
that when 'struct master' is freed, there are no components left
pointing at the to-be-freed master device.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [RFC] component: Fix: Unassign components' masters if bringing up master fails

[Archit Taneja]

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 <architt@codeaurora.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").
>

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