On Wed, Oct 21, 2015 at 10:37:27PM +0200, Takashi Iwai wrote:
On Wed, 21 Oct 2015 20:19:38 +0200, Russell King - ARM Linux wrote:
On Wed, Oct 21, 2015 at 07:59:06PM +0200, Takashi Iwai wrote:
On Wed, 21 Oct 2015 19:34:37 +0200, Russell King - ARM Linux wrote:
On Wed, Oct 21, 2015 at 09:49:28PM +0530, Vinod Koul wrote:
On Wed, Oct 21, 2015 at 03:37:47PM +0100, Russell King - ARM Linux wrote:
In any case, this doesn't (and can't) solve the CEC problem, so it's not a solution to the problem at hand.
Sorry am not sure I follow the reasons for that, wouldn't CEC be another slave in such an interface? I though component fwk did allow us to have multiple slaves..
Not with the way you're using the component helper here.
I guess that not all my message is being read, because people keep replying half-way down my messages...
You can only register a struct device _once_ as a slave device.
With the way you're using it here for audio, you're registering the i915 DRM device as a slave component device, and the audio side as the master. That means the audio master can bind to the DRM slave component device.
You can't then have a CEC master bind to the i915 DRM slave device (it's already bound to the audio master device), and you can't register the i915 DRM device as a second slave component device. It becomes indistinguishable from the first, and there's no way to tell which of the two different 'ops' structures should be used with which master.
I said this in my message 20151021140307.GE32532@n2100.arm.linux.org.uk which was two of my replies ago in this sub-thread.
Can't the limitation of single slave dev be extended simply? e.g. add some matching semantics to component_master_add_child() like a shared key in both master and slave, and let assign only the matched slave.
I've already explained that in the email message I referred to by message ID above (which was a reply to one of your previous messages)
This is why I find email such a poor communication medium - I'm quite sure most people only read half an email message before hitting reply, and then they stick their reply after where they stopped reading and never bother reading the rest of the message. Normally, at this point, I'll start getting grumpy and sending frustrated emails... which would eventually deride into flames, but let's _try_ to keep this civil.
Thank you for patience!
Here's the relevant paragraph from the above referenced message, and to make the appropriate bit stand out, I've underlined it with ^.
However, there's a lurking problem: you can't register the same struct device as a slave more than once into the component helpers - that's because it's designed to look for devices. There's intentionally no
^^^^^^^^^^^^^^^^^^^^^^^^linkage between the slave ops and master ops to allow for generic
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
slave drivers (like tda998x) to be used with different master drivers
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(armada, tilcdc, etc). In theory, you can register the master device
^^^^^^^^^^^^^^^^^^^^^
of one componentised system as a slave device of another, but that requires a much more complicated locking setup in component.c (I have patches for that, but I've resisted sending them because it makes the locking very messy.)
In a subsequent sentence, I also address the issue that would occur if any of the already componentised DRM drivers were to attempt what you're doing in i915 - although I say it's solvable, I've resisted that because it makes the locking in there _much_ more hairy, and I'm now not certain that my more complex locking implementation would even cope with that scenario.
I understood that the original component design wasn't for cross subsystems. OTOH I wondered why it can't be extended for wider use -- that was the simple question; I haven't seen so complex locking in some upstream drm drivers code through a quick glance, so the mess about locking you mentioned wasn't clear to me. Now point taken.
You can find my patch for the "more complex locking" problem at the end of this mail. This may not apply to the current version of component.c, but illustrates at least some of the problem of getting rid of the global component_mutex lock, which would be necessary to allow a binding master to call back into the component helper to then register a slave. Today, that action would deadlock on component_mutex.
I'm not actually convinced that even this patch is correct as it stands...
To be noted, a weak dependency is still necessary for i915 audio case, and a simple notifier type registration won't work, unfortunately. GPU power adjustment is mandatory even at probing the audio hardware to judge whether HDMI codec is present or not. Meanwhile the dependency to the graphics must not be tight, either. The very same audio driver is for a controller without graphics, too. That's the reason we took the component as an alternative implementation, which is a bit cleaner than the direct symbol resolving in the earlier code.
So, if any better solution that covers a use case like i915/hda is present, we're willing to switch. As repeatedly written, the current implementation is a stop gap. Although this doesn't look too bad, there are still some obvious pitfalls as you pointed out. We can paper over them (maybe I'll do for 4.4), but a fundamentally better solution would be more welcome, of course.
Well, I guess it's fine as a temporary stop-gap, but what I don't want to see is this stop-gap becoming more common. It may work for your i915 case, but it won't work everywhere, so it can't become a generic solution to this problem.
drivers/base/component.c | 142 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 116 insertions(+), 26 deletions(-)
diff --git a/drivers/base/component.c b/drivers/base/component.c index f748430bb654..d72fe9bc7033 100644 --- a/drivers/base/component.c +++ b/drivers/base/component.c @@ -30,7 +30,10 @@ struct component_match { struct master { struct list_head node; struct list_head components; + struct kref kref; + struct mutex mutex; bool bound; + bool dead;
const struct component_master_ops *ops; struct device *dev; @@ -49,8 +52,33 @@ struct component {
static DEFINE_MUTEX(component_mutex); static LIST_HEAD(component_list); +static DEFINE_MUTEX(master_mutex); static LIST_HEAD(masters);
+static void master_release(struct kref *kref) __releases(master_mutex) +{ + struct master *master = container_of(kref, struct master, kref); + + list_del(&master->node); + mutex_unlock(&master_mutex); + WARN_ON(!list_empty(&master->components)); + kfree(master); +} + +static void master_release_nolock(struct kref *kref) +{ + struct master *master = container_of(kref, struct master, kref); + + list_del(&master->node); + WARN_ON(!list_empty(&master->components)); + kfree(master); +} + +static void master_norelease(struct kref *kref) +{ + WARN_ON(kref); +} + static struct master *__master_find(struct device *dev, const struct component_master_ops *ops) { @@ -63,9 +91,30 @@ static struct master *__master_find(struct device *dev, return NULL; }
+/* + * Find the master structure for this device, and make sure that the + * per-master lock is held. This is used from within the master device + * drivers bind and unbind callbacks, which should only be called from + * paths where the per-master lock is already held. + */ +static struct master *master_find_locked(struct device *dev, + const struct component_master_ops *ops) +{ + struct master *master; + + mutex_lock(&master_mutex); + master = __master_find(dev, NULL); + WARN_ON(master && !mutex_is_locked(&master->mutex)); + mutex_unlock(&master_mutex); + + return master; +} + /* Attach an unattached component to a master. */ static void component_attach_master(struct master *master, struct component *c) { + kref_get(&master->kref); + c->master = master;
list_add_tail(&c->master_node, &master->components); @@ -77,6 +126,9 @@ static void component_detach_master(struct master *master, struct component *c) list_del(&c->master_node);
c->master = NULL; + + /* This kref_put should never release the master */ + kref_put(&master->kref, master_norelease); }
/* @@ -90,6 +142,7 @@ int component_master_add_child(struct master *master, struct component *c; int ret = -ENXIO;
+ mutex_lock(&component_mutex); list_for_each_entry(c, &component_list, node) { if (c->master && c->master != master) continue; @@ -101,6 +154,7 @@ int component_master_add_child(struct master *master, break; } } + mutex_unlock(&component_mutex);
return ret; } @@ -137,6 +191,7 @@ static int find_components(struct master *master) /* Detach all attached components from this master */ static void master_remove_components(struct master *master) { + mutex_lock(&component_mutex); while (!list_empty(&master->components)) { struct component *c = list_first_entry(&master->components, struct component, master_node); @@ -145,6 +200,7 @@ static void master_remove_components(struct master *master)
component_detach_master(master, c); } + mutex_unlock(&component_mutex); }
/* @@ -201,20 +257,43 @@ static int try_to_bring_up_master(struct master *master,
static int try_to_bring_up_masters(struct component *component) { - struct master *m; + struct master *m, *last = NULL; int ret = 0;
+ mutex_lock(&master_mutex); list_for_each_entry(m, &masters, node) { + if (last) { + kref_put(&last->kref, master_release_nolock); + last = NULL; + } + + if (m->dead || m->bound) + continue; + + kref_get(&m->kref); + mutex_unlock(&master_mutex); + + mutex_lock(&m->mutex); ret = try_to_bring_up_master(m, component); + mutex_unlock(&m->mutex); + + mutex_lock(&master_mutex); + last = m; + if (ret != 0) break; }
+ if (last) + kref_put(&last->kref, master_release_nolock); + mutex_unlock(&master_mutex); + return ret; }
static void take_down_master(struct master *master) { + mutex_lock(&master->mutex); if (master->bound) { master->ops->unbind(master->dev); devres_release_group(master->dev, NULL); @@ -222,6 +301,7 @@ static void take_down_master(struct master *master) }
master_remove_components(master); + mutex_unlock(&master->mutex); }
static size_t component_match_size(size_t num) @@ -307,20 +387,25 @@ int component_master_add_with_match(struct device *dev, master->dev = dev; master->ops = ops; master->match = match; + mutex_init(&master->mutex); INIT_LIST_HEAD(&master->components); + kref_init(&master->kref);
- /* Add to the list of available masters. */ - mutex_lock(&component_mutex); + /* + * Add to the list of available masters. This is so + * the component code below can find this master. + */ + mutex_lock(&master_mutex); list_add(&master->node, &masters); + mutex_unlock(&master_mutex);
+ mutex_lock(&master->mutex); ret = try_to_bring_up_master(master, NULL); + mutex_unlock(&master->mutex);
- if (ret < 0) { - /* Delete off the list if we weren't successful */ - list_del(&master->node); - kfree(master); - } - mutex_unlock(&component_mutex); + /* Delete off the list if we weren't successful */ + if (ret < 0) + kref_put_mutex(&master->kref, master_release, &master_mutex);
return ret < 0 ? ret : 0; } @@ -338,15 +423,17 @@ void component_master_del(struct device *dev, { struct master *master;
- mutex_lock(&component_mutex); + mutex_lock(&master_mutex); master = __master_find(dev, ops); + if (master) + master->dead = true; + mutex_unlock(&master_mutex); + if (master) { take_down_master(master);
- list_del(&master->node); - kfree(master); + kref_put_mutex(&master->kref, master_release, &master_mutex); } - mutex_unlock(&component_mutex); } EXPORT_SYMBOL_GPL(component_master_del);
@@ -364,12 +451,9 @@ static void component_unbind(struct component *component,
void component_unbind_all(struct device *master_dev, void *data) { - struct master *master; + struct master *master = master_find_locked(master_dev, NULL); struct component *c;
- WARN_ON(!mutex_is_locked(&component_mutex)); - - master = __master_find(master_dev, NULL); if (!master) return;
@@ -432,13 +516,10 @@ static int component_bind(struct component *component, struct master *master,
int component_bind_all(struct device *master_dev, void *data) { - struct master *master; + struct master *master = master_find_locked(master_dev, NULL); struct component *c; int ret = 0;
- WARN_ON(!mutex_is_locked(&component_mutex)); - - master = __master_find(master_dev, NULL); if (!master) return -EINVAL;
@@ -474,14 +555,16 @@ int component_add(struct device *dev, const struct component_ops *ops)
mutex_lock(&component_mutex); list_add_tail(&component->node, &component_list); + mutex_unlock(&component_mutex);
ret = try_to_bring_up_masters(component); if (ret < 0) { + mutex_lock(&component_mutex); list_del(&component->node); + mutex_unlock(&component_mutex);
kfree(component); } - mutex_unlock(&component_mutex);
return ret < 0 ? ret : 0; } @@ -490,21 +573,28 @@ EXPORT_SYMBOL_GPL(component_add); void component_del(struct device *dev, const struct component_ops *ops) { struct component *c, *component = NULL; + struct master *master = NULL;
mutex_lock(&component_mutex); list_for_each_entry(c, &component_list, node) if (c->dev == dev && c->ops == ops) { + master = c->master; + if (master) + kref_get(&master->kref); list_del(&c->node); component = c; break; } + mutex_unlock(&component_mutex);
- if (component && component->master) - take_down_master(component->master); + if (WARN_ON(!component)) + return;
- mutex_unlock(&component_mutex); + if (master) { + take_down_master(master); + kref_put_mutex(&master->kref, master_release, &master_mutex); + }
- WARN_ON(!component); kfree(component); } EXPORT_SYMBOL_GPL(component_del);