component helper improvements
Subsystems (6)
cover component helper improvements
A while back, Laurent raised some comments about the component helper, which this patch set starts to address.
The first point it addresses is the repeated parsing inefficiency when deferred probing occurs. When DT is used, the structure of the component helper today means that masters end up parsing the device tree for each attempt to re-bind the driver.
We remove this inefficiency by creating an array of matching data and functions, which the component helper can use internally to match up components to their master.
The second point was the inefficiency of destroying the list of components each time we saw a failure. We did this to ensure that we kept things correctly ordered: component bind order matters. As we have an array instead, the array is already ordered, so we use this array to store the component pointers instead of a list, and remember which are duplicates (and thus should be avoided.) Avoiding the right duplicates matters as we walk the array in the opposite direction at tear down.
I would like to see patches 1-5 scheduled for the next merge window, with 6-8 for the following window - this gives us grace of one kernel cycle to ensure that any new component helper users are properly converted.
drivers/base/component.c | 248 ++++++++++++++++++++++----------- drivers/gpu/drm/msm/msm_drv.c | 81 +++++------ drivers/staging/imx-drm/imx-drm-core.c | 57 +------- include/linux/component.h | 8 +- 4 files changed, 206 insertions(+), 188 deletions(-)
Signature · 2 lines
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.
RFC PATCH 1/8 component: fix missed cleanup in case of devres failure
In try_to_bring_up_master(), we tear down the master's component list for each error case, except for devres group failure. Fix this oversight by making the code less prone to such mistakes.
--- drivers/base/component.c | 62 ++++++++++++++++++++++++------------------------ 1 file changed, 31 insertions(+), 31 deletions(-)
drivers/base/component.c +31 -31
@@ -113,44 +113,44 @@ static void master_remove_components(struct master *master)static int try_to_bring_up_master (struct master *master,struct component *component) { -int ret =0 ; +int ret; -if (!master->bound) { -/* -* Search the list of components, looking for components that -* belong to this master, and attach them to the master. -*/ -if (master->ops->add_components (master->dev, master)) { -/* Failed to find all components */ -master_remove_components (master); - ret =0 ; - goto out; - } +if (master->bound) +return 0 ; -if (component && component->master != master) { -master_remove_components (master); - ret =0 ; - goto out; - } +/* +* Search the list of components, looking for components that +* belong to this master, and attach them to the master. +*/ +if (master->ops->add_components (master->dev, master)) { +/* Failed to find all components */ + ret =0 ; + goto out; + } -if (!devres_open_group (master->dev,NULL ,GFP_KERNEL )) { - ret = -ENOMEM ; - goto out; - } +if (component && component->master != master) { + ret =0 ; + goto out; + } -/* 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); -master_remove_components (master); - goto out; - } +if (!devres_open_group (master->dev,NULL ,GFP_KERNEL )) { + ret = -ENOMEM ; + goto out; + } - master->bound = true; - ret =1 ; +/* 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); + goto out; } + + master->bound = true; +return 1 ; + out: +master_remove_components (master);return ret; }
Signature · 1 line
1.8.3.1
RFC PATCH 1/8 component: fix missed cleanup in case of devres failure
In reply to Russell King - ARM LinuxIn try_to_bring_up_master(), we tear down the master's component list for each error case, except for devres group failure. Fix this oversight by making the code less prone to such mistakes.
--- drivers/base/component.c | 62 ++++++++++++++++++++++++------------------------ 1 file changed, 31 insertions(+), 31 deletions(-)
drivers/base/component.c +31 -31
@@ -113,44 +113,44 @@ static void master_remove_components(struct master *master)static int try_to_bring_up_master (struct master *master,struct component *component) { -int ret =0 ; +int ret; -if (!master->bound) { -/* -* Search the list of components, looking for components that -* belong to this master, and attach them to the master. -*/ -if (master->ops->add_components (master->dev, master)) { -/* Failed to find all components */ -master_remove_components (master); - ret =0 ; - goto out; - } +if (master->bound) +return 0 ; -if (component && component->master != master) { -master_remove_components (master); - ret =0 ; - goto out; - } +/* +* Search the list of components, looking for components that +* belong to this master, and attach them to the master. +*/ +if (master->ops->add_components (master->dev, master)) { +/* Failed to find all components */ + ret =0 ; + goto out; + } -if (!devres_open_group (master->dev,NULL ,GFP_KERNEL )) { - ret = -ENOMEM ; - goto out; - } +if (component && component->master != master) { + ret =0 ; + goto out; + } -/* 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); -master_remove_components (master); - goto out; - } +if (!devres_open_group (master->dev,NULL ,GFP_KERNEL )) { + ret = -ENOMEM ; + goto out; + } - master->bound = true; - ret =1 ; +/* 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); + goto out; } + + master->bound = true; +return 1 ; + out: +master_remove_components (master);return ret; }
Signature · 1 line
1.8.3.1
RFC PATCH 2/8 component: ignore multiple additions of the same component
In reply to Russell King - ARM LinuxPermit masters to call component_master_add_child() and match the same child multiple times. This may happen if there's multiple connections to a single component device from other devices. In such scenarios, we should not return a failure, but instead ignore the attempt.
--- drivers/base/component.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/base/component.c +8 -2
@@ -69,6 +69,11 @@ static void component_detach_master(struct master *master, struct component *c) c->master =NULL ; } +/* +* Add a component to a master, finding the component via the compare +* function and compare data. This is safe to call for duplicate matches +* and will not result in the same component being added multiple times. +*/ int component_master_add_child (struct master *master,int (*compare)(struct device *,void *),void *compare_data) {
@@ -76,11 +81,12 @@ int component_master_add_child(struct master *master,int ret = -ENXIO ;list_for_each_entry (c, &component_list, node) { -if (c->master) +if (c->master && c->master != master)continue ;if (compare (c->dev, compare_data)) { -component_attach_master (master, c); +if (!c->master) +component_attach_master (master, c); ret =0 ;break ; }
Signature · 1 line
1.8.3.1
RFC PATCH 3/8 component: add support for component match array
In reply to Russell King - ARM LinuxAdd support for generating a set of component matches at master probe time, and submitting them to the component layer. This allows the component layer to perform the matches internally without needing to call into the master driver, and allows for further restructuring of the component helper.
--- drivers/base/component.c | 118 ++++++++++++++++++++++++++++++++++++++++++++-- include/linux/component.h | 7 +++ 2 files changed, 122 insertions(+), 3 deletions(-)
drivers/base/component.c +115 -3
@@ -18,6 +18,15 @@#include <linux/mutex.h> #include <linux/slab.h> +struct component_match { +size_t alloc; +size_t num; +struct { +void *data; +int (*fn)(struct device *,void *); + } compare[0 ]; +}; +struct master {struct list_head node;struct list_head components;
@@ -25,6 +34,7 @@ struct master {const struct component_master_ops *ops;struct device *dev; +struct component_match *match; };struct component {
@@ -96,6 +106,34 @@ int component_master_add_child(struct master *master, }EXPORT_SYMBOL_GPL (component_master_add_child); +static int find_components (struct master *master) +{ +struct component_match *match = master->match; +size_t i; +int ret =0 ; + +if (!match) { +/* +* Search the list of components, looking for components that +* belong to this master, and attach them to the master. +*/ +return master->ops->add_components (master->dev, master); + } + +/* +* Scan the array of match functions and attach +* any components which are found to this master. +*/ +for (i =0 ; i < match->num; i++) { + ret =component_master_add_child (master, + match->compare[i].fn, + match->compare[i].data); +if (ret) +break ; + } +return ret; +} +/* Detach all attached components from this master */ static void master_remove_components (struct master *master) {
@@ -128,7 +166,7 @@ static int try_to_bring_up_master(struct master *master,* Search the list of components, looking for components that * belong to this master, and attach them to the master. */ -if (master->ops->add_components (master->dev, master)) { +if (find_components (master)) {/* Failed to find all components */ ret =0 ; goto out;
@@ -186,18 +224,85 @@ static void take_down_master(struct master *master)master_remove_components (master); } -int component_master_add (struct device *dev, -const struct component_master_ops *ops) +static size_t component_match_size (size_t num) +{ + return offsetof(struct component_match, compare[num]); +} + +static struct component_match *component_match_realloc (struct device *dev, +struct component_match *match,size_t num) +{ +struct component_match *new; + +if (match && match->alloc == num) +return match; + + new =devm_kmalloc (dev,component_match_size (num),GFP_KERNEL ); +if (!new) +return ERR_PTR (-ENOMEM ); + +if (match) { +memcpy (new, match,component_match_size (min (match->num, num))); +devm_kfree (dev, match); + }else { + new->num =0 ; + } + + new->alloc = num; + +return new; +} + +/* +* Add a component to be matched. +* +* The match array is first created or extended if necessary. +*/ +void component_match_add (struct device *dev,struct component_match **matchptr, +int (*compare)(struct device *,void *),void *compare_data) +{ +struct component_match *match = *matchptr; + +if (IS_ERR (match)) +return ; + +if (!match || ++match->num == match->alloc) { +size_t new_size = match ? match->alloc +16 :15 ; + + match =component_match_realloc (dev, match, new_size); + + *matchptr = match; + +if (IS_ERR (match)) +return ; + } + + match->compare[match->num].fn = compare; + match->compare[match->num].data = compare_data; +} + +int component_master_add_with_match (struct device *dev, +const struct component_master_ops *ops, +struct component_match *match) {struct master *master;int ret; +if (ops->add_components && match) +return -EINVAL ; + +/* Reallocate the match array for its true size */ + match =component_match_realloc (dev, match, match->num); +if (IS_ERR (match)) +return PTR_ERR (match); + master =kzalloc (sizeof (*master),GFP_KERNEL );if (!master)return -ENOMEM ; master->dev = dev; master->ops = ops; + master->match = match;INIT_LIST_HEAD (&master->components);/* Add to the list of available masters. */
@@ -215,6 +320,13 @@ int component_master_add(struct device *dev,return ret <0 ? ret :0 ; } +EXPORT_SYMBOL_GPL (component_master_add_with_match); + +int component_master_add (struct device *dev, +const struct component_master_ops *ops) +{ +return component_master_add_with_match (dev, ops,NULL ); +}EXPORT_SYMBOL_GPL (component_master_add); void component_master_del(struct device *dev,
include/linux/component.h +7 -0
@@ -29,4 +29,11 @@ void component_master_del(struct device *,int component_master_add_child (struct master *master,int (*compare)(struct device *,void *),void *compare_data); +struct component_match ; + +int component_master_add_with_match (struct device *, +const struct component_master_ops *,struct component_match *); +void component_match_add (struct device *,struct component_match **, +int (*compare)(struct device *,void *),void *compare_data); +#endif
Signature · 1 line
1.8.3.1
RFC PATCH 3/8 component: add support for component match array
Quoted 13 lines · Russell King
Add support for generating a set of component matches at master probe time, and submitting them to the component layer. This allows the component layer to perform the matches internally without needing to call into the master driver, and allows for further restructuring of the component helper.
Signed-off-by Russell King <[email protected]>--- drivers/base/component.c | 118 ++++++++++++++++++++++++++++++++++++++++++++-- include/linux/component.h | 7 +++ 2 files changed, 122 insertions(+), 3 deletions(-)drivers/base/component.c +0
-0
[...]
drivers/base/component.c static void take_down_master(struct master *master) · line 270
Quoted 10 lines
drivers/base/component.c +10
-0+void component_match_add (struct device *dev,struct component_match **matchptr, +int (*compare)(struct device *,void *),void *compare_data) +{ +struct component_match *match = *matchptr; + +if (IS_ERR (match)) +return ; + +if (!match || ++match->num == match->alloc) { +size_t new_size = match ? match->alloc +16 :15 ;
Doesn't this allocate prematurely? If component_match_add() is called on the final component of a master, then match->num will be incremented to it's final value. If that matches match->alloc, then things are just fine, aren't they? No need to allocate another 16 entries.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140428/44ea3b9b/attachment.sig>
RFC PATCH 3/8 component: add support for component match array
Quoted 15 lines · Thierry Reding
Quoted 13 lines · Russell King
Add support for generating a set of component matches at master probe time, and submitting them to the component layer. This allows the component layer to perform the matches internally without needing to call into the master driver, and allows for further restructuring of the component helper.
Signed-off-by Russell King <[email protected]>--- drivers/base/component.c | 118 ++++++++++++++++++++++++++++++++++++++++++++-- include/linux/component.h | 7 +++ 2 files changed, 122 insertions(+), 3 deletions(-)drivers/base/component.c +0
-0[...]
Quoted 15 lines
drivers/base/component.c static void take_down_master(struct master *master) · line 270
Quoted 10 lines
drivers/base/component.c +10
-0+void component_match_add (struct device *dev,struct component_match **matchptr, +int (*compare)(struct device *,void *),void *compare_data) +{ +struct component_match *match = *matchptr; + +if (IS_ERR (match)) +return ; + +if (!match || ++match->num == match->alloc) { +size_t new_size = match ? match->alloc +16 :15 ;Doesn't this allocate prematurely? If
component_match_add()is called on the final component of a master, thenmatch->numwill be incremented to it's final value. If that matchesmatch->alloc, then things are just fine, aren't they? No need to allocate another 16 entries.
This code appears to be correct - `num' is actually one less than the number of components.
However, that makes code elsewhere wrong... v2 coming up soon with this fixed. Good catch.
Signature · 2 lines
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.
RFC PATCH 4/8 drm: msm: update to use component match support
In reply to Russell King - ARM LinuxUpdate MSM's DRM driver to use the component match support rather than add_components.
--- drivers/gpu/drm/msm/msm_drv.c | 81 ++++++++++++++++++------------------------- 1 file changed, 34 insertions(+), 47 deletions(-)
drivers/gpu/drm/msm/msm_drv.c +34 -47
@@ -865,12 +865,41 @@ static int compare_of(struct device *dev, void *data) {return dev->of_node == data; } +#else +static int compare_dev (struct device *dev,void *data) +{ +return dev == data; +} +#endif + +static int msm_drm_bind (struct device *dev) +{ +return drm_platform_init (&msm_driver,to_platform_device (dev)); +} + +static void msm_drm_unbind (struct device *dev) +{ +drm_put_dev (platform_get_drvdata (to_platform_device (dev))); +} + +static const struct component_master_ops msm_drm_ops = { + .bind = msm_drm_bind, + .unbind = msm_drm_unbind, +}; + +/* +* Platform driver: +*/ -static int msm_drm_add_components (struct device *master,struct master *m) +static int msm_pdev_probe (struct platform_device *pdev) { +struct component_match *match =NULL ; +#ifdef CONFIG_OF +/* NOTE: the CONFIG_OF case duplicates the same code as exynos or imx +* (or probably any other).. so probably some room for some helpers +*/ struct device_node *np = master->of_node;unsigned i; -int ret;for (i =0 ; ; i++) {struct device_node *node;
@@ -879,22 +908,9 @@ static int msm_drm_add_components(struct device *master, struct master *m)if (!node)break ; - ret =component_master_add_child (m, compare_of, node); -of_node_put (node); - -if (ret) - return ret; + component_match_add(&pdev->dev, &match, compare_of, node); } -return 0 ; -}#else -static int compare_dev (struct device *dev,void *data) -{ -return dev == data; -} - -static int msm_drm_add_components (struct device *master,struct master *m) -{/* For non-DT case, it kinda sucks. We don't actually have a way * to know whether or not we are waiting for certain devices (or if * they are simply not present). But for non-DT we only need to
@@ -918,41 +934,12 @@ static int msm_drm_add_components(struct device *master, struct master *m) return -EPROBE_DEFER; } - ret =component_master_add_child (m, compare_dev, dev); -if (ret) { -DBG ("could not add child: %d" , ret); -return ret; - } +component_match_add (&pdev->dev, &match, compare_dev, dev); } - -return 0 ; -}#endif -static int msm_drm_bind (struct device *dev) -{ -return drm_platform_init (&msm_driver,to_platform_device (dev)); -} - -static void msm_drm_unbind (struct device *dev) -{ -drm_put_dev (platform_get_drvdata (to_platform_device (dev))); -} - -static const struct component_master_ops msm_drm_ops = { - .add_components = msm_drm_add_components, - .bind = msm_drm_bind, - .unbind = msm_drm_unbind, -}; - -/* -* Platform driver: -*/ - -static int msm_pdev_probe (struct platform_device *pdev) -{ pdev->dev.coherent_dma_mask =DMA_BIT_MASK (32 ); -return component_master_add (&pdev->dev, &msm_drm_ops); +return component_master_add_with_match (&pdev->dev, &msm_drm_ops, match); }static int msm_pdev_remove (struct platform_device *pdev)
Signature · 1 line
1.8.3.1
Re: [PATCH RFC 4/8] drm: msm: update to use component match support
Quoted 2 lines · Russell King
Update MSM's DRM driver to use the component match support rather than
add_components.
Looks like the component helper improvements make things nicer for the driver. Let me know when you are ready to merge the set, if you want me to take this one via my tree or not.
Quoted 124 lines
Signed-off-by Russell King <[email protected]>--- drivers/gpu/drm/msm/msm_drv.c | 81 ++++++++++++++++++------------------------- 1 file changed, 34 insertions(+), 47 deletions(-)drivers/gpu/drm/msm/msm_drv.c +34
-48@@ -865,12 +865,41 @@ static int compare_of(struct device *dev, void *data) {return dev->of_node == data; } +#else +static int compare_dev (struct device *dev,void *data) +{ +return dev == data; +} +#endif + +static int msm_drm_bind (struct device *dev) +{ +return drm_platform_init (&msm_driver,to_platform_device (dev)); +} + +static void msm_drm_unbind (struct device *dev) +{ +drm_put_dev (platform_get_drvdata (to_platform_device (dev))); +} + +static const struct component_master_ops msm_drm_ops = { + .bind = msm_drm_bind, + .unbind = msm_drm_unbind, +}; + +/* +* Platform driver: +*/ -static int msm_drm_add_components (struct device *master,struct master *m) +static int msm_pdev_probe (struct platform_device *pdev) { +struct component_match *match =NULL ; +#ifdef CONFIG_OF +/* NOTE: the CONFIG_OF case duplicates the same code as exynos or imx +* (or probably any other).. so probably some room for some helpers +*/ struct device_node *np = master->of_node;unsigned i; -int ret;for (i =0 ; ; i++) {struct device_node *node;@@ -879,22 +908,9 @@ static int msm_drm_add_components(struct device *master, struct master *m)if (!node)break ; - ret =component_master_add_child (m, compare_of, node); -of_node_put (node); - -if (ret) - return ret; + component_match_add(&pdev->dev, &match, compare_of, node); } -return 0 ; -}#else -static int compare_dev (struct device *dev,void *data) -{ -return dev == data; -} - -static int msm_drm_add_components (struct device *master,struct master *m) -{/* For non-DT case, it kinda sucks. We don't actually have a way * to know whether or not we are waiting for certain devices (or if * they are simply not present). But for non-DT we only need to @@ -918,41 +934,12 @@ static int msm_drm_add_components(struct device *master, struct master *m) return -EPROBE_DEFER; } - ret =component_master_add_child (m, compare_dev, dev); -if (ret) { -DBG ("could not add child: %d" , ret); -return ret; - } +component_match_add (&pdev->dev, &match, compare_dev, dev); } - -return 0 ; -}#endif -static int msm_drm_bind (struct device *dev) -{ -return drm_platform_init (&msm_driver,to_platform_device (dev)); -} - -static void msm_drm_unbind (struct device *dev) -{ -drm_put_dev (platform_get_drvdata (to_platform_device (dev))); -} - -static const struct component_master_ops msm_drm_ops = { - .add_components = msm_drm_add_components, - .bind = msm_drm_bind, - .unbind = msm_drm_unbind, -}; - -/* -* Platform driver: -*/ - -static int msm_pdev_probe (struct platform_device *pdev) -{ pdev->dev.coherent_dma_mask =DMA_BIT_MASK (32 ); -return component_master_add (&pdev->dev, &msm_drm_ops); +return component_master_add_with_match (&pdev->dev, &msm_drm_ops, match); }static int msm_pdev_remove (struct platform_device *pdev) --1.8.3.1
RFC PATCH 5/8 imx-drm: update to use component match support
In reply to Russell King - ARM LinuxUpdate the imx-drm driver to use the component match support rather than add_components.
--- drivers/staging/imx-drm/imx-drm-core.c | 57 +++------------------------------- 1 file changed, 4 insertions(+), 53 deletions(-)
drivers/staging/imx-drm/imx-drm-core.c +4 -53
@@ -577,22 +577,6 @@ static int compare_of(struct device *dev, void *data)return dev->of_node == np; } -static LIST_HEAD (imx_drm_components); - -static int imx_drm_add_components (struct device *master,struct master *m) -{ -struct imx_drm_component *component; -int ret; - -list_for_each_entry (component, &imx_drm_components, list) { - ret =component_master_add_child (m, compare_of, - component->of_node); -if (ret) -return ret; - } -return 0 ; -} -static int imx_drm_bind (struct device *dev) {return drm_platform_init (&imx_drm_driver,to_platform_device (dev));
@@ -604,43 +588,14 @@ static void imx_drm_unbind(struct device *dev) }static const struct component_master_ops imx_drm_ops = { - .add_components = imx_drm_add_components, .bind = imx_drm_bind, .unbind = imx_drm_unbind, }; -static struct imx_drm_component *imx_drm_find_component (struct device *dev, -struct device_node *node) -{ -struct imx_drm_component *component; - -list_for_each_entry (component, &imx_drm_components, list) -if (component->of_node == node) -return component; - -return NULL ; -} - -static int imx_drm_add_component (struct device *dev,struct device_node *node) -{ -struct imx_drm_component *component; - -if (imx_drm_find_component (dev, node)) -return 0 ; - - component =devm_kzalloc (dev,sizeof (*component),GFP_KERNEL ); -if (!component) -return -ENOMEM ; - - component->of_node = node; -list_add_tail (&component->list, &imx_drm_components); - -return 0 ; -} -static int imx_drm_platform_probe (struct platform_device *pdev) {struct device_node *ep, *port, *remote; +struct component_match *match =NULL ;int ret;int i;
@@ -654,9 +609,7 @@ static int imx_drm_platform_probe(struct platform_device *pdev)if (!port)break ; - ret =imx_drm_add_component (&pdev->dev, port); -if (ret <0 ) -return ret; +component_match_add (&pdev->dev, &match, compare_of, port); }if (i ==0 ) {
@@ -677,10 +630,8 @@ static int imx_drm_platform_probe(struct platform_device *pdev)continue ; } - ret =imx_drm_add_component (&pdev->dev, remote); +component_match_add (&pdev->dev, &match, compare_of, remote);of_node_put (remote); -if (ret <0 ) -return ret; }of_node_put (port); }
@@ -689,7 +640,7 @@ static int imx_drm_platform_probe(struct platform_device *pdev)if (ret)return ret; -return component_master_add (&pdev->dev, &imx_drm_ops); +return component_master_add_with_match (&pdev->dev, &imx_drm_ops, match); }static int imx_drm_platform_remove (struct platform_device *pdev)
Signature · 1 line
1.8.3.1
RFC PATCH 6/8 component: remove old add_components method
In reply to Russell King - ARM LinuxNow that drivers create an array of component matches at probe time, we can retire the old methods. This involves removing the add_components master method, and removing component_master_add_child() from public view. We also remove component_add_master() as that interface is no longer useful.
--- drivers/base/component.c | 21 +-------------------- include/linux/component.h | 5 ----- 2 files changed, 1 insertion(+), 25 deletions(-)
drivers/base/component.c +1 -20
@@ -84,7 +84,7 @@ static void component_detach_master(struct master *master, struct component *c)* function and compare data. This is safe to call for duplicate matches * and will not result in the same component being added multiple times. */ -int component_master_add_child (struct master *master, +static int component_master_add_child (struct master *master,int (*compare)(struct device *,void *),void *compare_data) {struct component *c;
@@ -104,7 +104,6 @@ int component_master_add_child(struct master *master,return ret; } -EXPORT_SYMBOL_GPL (component_master_add_child);static int find_components (struct master *master) {
@@ -112,14 +111,6 @@ static int find_components(struct master *master)size_t i;int ret =0 ; -if (!match) { -/* -* Search the list of components, looking for components that -* belong to this master, and attach them to the master. -*/ -return master->ops->add_components (master->dev, master); - } -/* * Scan the array of match functions and attach * any components which are found to this master.
@@ -288,9 +279,6 @@ int component_master_add_with_match(struct device *dev,struct master *master;int ret; -if (ops->add_components && match) -return -EINVAL ; -/* Reallocate the match array for its true size */ match =component_match_realloc (dev, match, match->num);if (IS_ERR (match))
@@ -322,13 +310,6 @@ int component_master_add_with_match(struct device *dev, }EXPORT_SYMBOL_GPL (component_master_add_with_match); -int component_master_add (struct device *dev, -const struct component_master_ops *ops) -{ -return component_master_add_with_match (dev, ops,NULL ); -} -EXPORT_SYMBOL_GPL (component_master_add); -void component_master_del (struct device *dev,const struct component_master_ops *ops) {
include/linux/component.h +0 -5
@@ -17,18 +17,13 @@ void component_unbind_all(struct device *, void *);struct master ;struct component_master_ops { -int (*add_components)(struct device *,struct master *);int (*bind)(struct device *);void (*unbind)(struct device *); }; -int component_master_add (struct device *,const struct component_master_ops *);void component_master_del (struct device *,const struct component_master_ops *); -int component_master_add_child (struct master *master, -int (*compare)(struct device *,void *),void *compare_data); -struct component_match ; int component_master_add_with_match(struct device *,
Signature · 1 line
1.8.3.1
RFC PATCH 6/8 component: remove old add_components method
Quoted 11 lines · Russell King
Now that drivers create an array of component matches at probe time, we can retire the old methods. This involves removing the
add_componentsmaster method, and removingcomponent_master_add_child()from public view. We also removecomponent_add_master()as that interface is no longer useful.Signed-off-by Russell King <[email protected]>--- drivers/base/component.c | 21 +-------------------- include/linux/component.h | 5 ----- 2 files changed, 1 insertion(+), 25 deletions(-)
I'm wondering if there may be an advantage to keeping both interfaces. Even if currently what all implementations do is essentially creating the match table at probe time there may be use-cases where that doesn't work so well.
There's also some elegance to the .add_components() interface. I have a set of patches to convert Tegra DRM to use the component framework and it works out rather nicely. I'm sure I can get it to work with your proposed match array changes too, but it seems like it will require more code.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140428/ce62653f/attachment.sig>
RFC PATCH 6/8 component: remove old add_components method
Quoted 4 lines · Thierry Reding
I'm wondering if there may be an advantage to keeping both interfaces. Even if currently what all implementations do is essentially creating the match table at probe time there may be use-cases where that doesn't work so well.
Keeping both interfaces makes this whole change is pointless, because then there's no way to avoid having to rebuild the tracking of which components belong to which master - and we might as well stick with what we have.
Moreover, one of the other points Laurent raised is that we need to be able to do partial binds for some subsystems, and for that to work we need more information held within the component helpers and the teardown/rebuild of the master/component relationships to be eliminated.
The last point is that the repeated teardown/rebuild is already being used as a justification to go off and write a completely different infrastructure instead... it seems some people deem this to be far too wasteful of our billion cycles per second CPUs.
Signature · 2 lines
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.
RFC PATCH 6/8 component: remove old add_components method
Quoted 16 lines · Russell King - ARM Linux
Quoted 4 lines · Thierry Reding
I'm wondering if there may be an advantage to keeping both interfaces. Even if currently what all implementations do is essentially creating the match table at probe time there may be use-cases where that doesn't work so well.
Keeping both interfaces makes this whole change is pointless, because then there's no way to avoid having to rebuild the tracking of which components belong to which master - and we might as well stick with what we have.
Moreover, one of the other points Laurent raised is that we need to be able to do partial binds for some subsystems, and for that to work we need more information held within the component helpers and the teardown/rebuild of the master/component relationships to be eliminated.
Quoted 4 lines
The last point is that the repeated teardown/rebuild is already being used as a justification to go off and write a completely different infrastructure instead... it seems some people deem this to be far too wasteful of our billion cycles per second CPUs.
FWIW, I do like the simplicity of the ->add_components() variant, and I don't mind the few extra cycles wasted, but if this change makes sense for everybody else I'll go convert my Tegra patches on top of this.
By the way, any chance I could get you to look at two earlier patches[0] I did to make component/master work better for my use-case? The patches are part of a series to convert the Tegra DRM driver to use the component framework. The final patch[1] has the complete conversion with a diff that I really like, although it requires yet another patch to make it work (because Tegra has a somewhat unusual architecture).
Thierry
[0]: https://lkml.org/lkml/2014/4/22/535
https://lkml.org/lkml/2014/4/22/1123
[1]: https://lkml.org/lkml/2014/4/22/1099
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140428/5ef864d2/attachment-0001.sig>
RFC PATCH 7/8 component: move check for unbound master into try_to_bring_up_masters()
In reply to Russell King - ARM LinuxClean up the code a little; we don't need to check that the master is unbound for every invocation of try_to_bring_up_master(), so let's move it to where it's really needed - try_to_bring_up_masters(), where we may encounter already bound masters.
--- drivers/base/component.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)
drivers/base/component.c +5 -10
@@ -150,13 +150,6 @@ static int try_to_bring_up_master(struct master *master, {int ret; -if (master->bound) -return 0 ; - -/* -* Search the list of components, looking for components that -* belong to this master, and attach them to the master. -*/ if (find_components (master)) {/* Failed to find all components */ ret =0 ;
@@ -196,9 +189,11 @@ static int try_to_bring_up_masters(struct component *component)int ret =0 ;list_for_each_entry (m, &masters, node) { - ret =try_to_bring_up_master (m, component); -if (ret !=0 ) -break ; +if (!m->bound) { + ret =try_to_bring_up_master (m, component); +if (ret !=0 ) +break ; + } }return ret;
Signature · 1 line
1.8.3.1
RFC PATCH 7/8 component: move check for unbound master into try_to_bring_up_masters()
Quoted 9 lines · Russell King
Clean up the code a little; we don't need to check that the master is unbound for every invocation of
try_to_bring_up_master(), so let's move it to where it's really needed -try_to_bring_up_masters(), where we may encounter already bound masters.Signed-off-by Russell King <[email protected]>--- drivers/base/component.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140428/60a0e5e5/attachment.sig>
RFC PATCH 8/8 component: track components via array rather than list
In reply to Russell King - ARM LinuxSince we now have an array which defines each component, maintain the components to be bound in the array rather than a separate list. We also need duplicate tracking so we can eliminate multiple bind calls for the same component: we preserve the list-based component order in that the first match which adds the component determines its position.
--- drivers/base/component.c | 140 +++++++++++++++++++++-------------------------- 1 file changed, 62 insertions(+), 78 deletions(-)
drivers/base/component.c +62 -78
@@ -18,18 +18,21 @@#include <linux/mutex.h> #include <linux/slab.h> +struct component ; +struct component_match {size_t alloc;size_t num;struct {void *data;int (*fn)(struct device *,void *); +struct component *component; +bool duplicate; } compare[0 ]; };struct master {struct list_head node; -struct list_head components;bool bound;const struct component_master_ops *ops;
@@ -39,7 +42,6 @@ struct master {struct component {struct list_head node; -struct list_head master_node;struct master *master;bool bound;
@@ -63,46 +65,20 @@ static struct master *__master_find(struct device *dev,return NULL ; } -/* Attach an unattached component to a master. */ -static void component_attach_master (struct master *master,struct component *c) -{ - c->master = master; - -list_add_tail (&c->master_node, &master->components); -} - -/* Detach a component from a master. */ -static void component_detach_master (struct master *master,struct component *c) -{ -list_del (&c->master_node); - - c->master =NULL ; -} - -/* -* Add a component to a master, finding the component via the compare -* function and compare data. This is safe to call for duplicate matches -* and will not result in the same component being added multiple times. -*/ -static int component_master_add_child (struct master *master, +static struct component *find_component (struct master *master,int (*compare)(struct device *,void *),void *compare_data) {struct component *c; -int ret = -ENXIO ;list_for_each_entry (c, &component_list, node) {if (c->master && c->master != master)continue ; -if (compare (c->dev, compare_data)) { -if (!c->master) -component_attach_master (master, c); - ret =0 ; -break ; - } +if (compare (c->dev, compare_data)) +return c; } -return ret; +return NULL ; }static int find_components (struct master *master)
@@ -116,26 +92,35 @@ static int find_components(struct master *master)* any components which are found to this master. */ for (i =0 ; i < match->num; i++) { - ret =component_master_add_child (master, - match->compare[i].fn, - match->compare[i].data); -if (ret) +struct component *c; + +if (match->compare[i].component) +continue ; + + c =find_component (master, match->compare[i].fn, + match->compare[i].data); +if (!c) { + ret = -ENXIO ;break ; + } + +/* Attach this component to the master */ + match->compare[i].duplicate = !!c->master; + match->compare[i].component = c; + c->master = master; }return ret; } -/* Detach all attached components from this master */ -static void master_remove_components (struct master *master) +/* Detach component from associated master */ +static void remove_component (struct master *master,struct component *c) { -while (!list_empty (&master->components)) { -struct component *c =list_first_entry (&master->components, - struct component, master_node); - -WARN_ON (c->master != master); +size_t i; -component_detach_master (master, c); - } +/* Detach the component from this master. */ +for (i =0 ; i < master->match->num; i++) +if (master->match->compare[i].component == c) + master->match->compare[i].component =NULL ; }/*
@@ -150,37 +135,25 @@ static int try_to_bring_up_master(struct master *master, {int ret; -if (find_components (master)) { -/* Failed to find all components */ - ret =0 ; - goto out; - } +if (find_components (master)) +return 0 ; -if (component && component->master != master) { - ret =0 ; - goto out; - } +if (component && component->master != master) +return 0 ; -if (!devres_open_group (master->dev,NULL ,GFP_KERNEL )) { - ret = -ENOMEM ; - goto out; - } +if (!devres_open_group (master->dev,NULL ,GFP_KERNEL )) +return -ENOMEM ;/* 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); - goto out; +return ret; } master->bound = true;return 1 ; - -out: -master_remove_components (master); - -return ret; }static int try_to_bring_up_masters (struct component *component)
@@ -206,8 +179,6 @@ static void take_down_master(struct master *master)devres_release_group (master->dev,NULL ); master->bound = false; } - -master_remove_components (master); }static size_t component_match_size (size_t num)
@@ -265,6 +236,7 @@ void component_match_add(struct device *dev, struct component_match **matchptr, match->compare[match->num].fn = compare; match->compare[match->num].data = compare_data; + match->compare[match->num].component =NULL ; } int component_master_add_with_match(struct device *dev,
@@ -286,7 +258,6 @@ int component_master_add_with_match(struct device *dev, master->dev = dev; master->ops = ops; master->match = match; -INIT_LIST_HEAD (&master->components);/* Add to the list of available masters. */ mutex_lock (&component_mutex);
@@ -338,6 +309,7 @@ void component_unbind_all(struct device *master_dev, void *data) {struct master *master;struct component *c; +size_t i;WARN_ON (!mutex_is_locked (&component_mutex));
@@ -345,8 +317,12 @@ void component_unbind_all(struct device *master_dev, void *data)if (!master)return ; -list_for_each_entry_reverse (c, &master->components, master_node) -component_unbind (c, master, data); +/* Unbind components in reverse order */ +for (i = master->match->num; i--; ) +if (!master->match->compare[i].duplicate) { + c = master->match->compare[i].component; +component_unbind (c, master, data); + } }EXPORT_SYMBOL_GPL (component_unbind_all);
@@ -406,6 +382,7 @@ int component_bind_all(struct device *master_dev, void *data) {struct master *master;struct component *c; +size_t i;int ret =0 ;WARN_ON (!mutex_is_locked (&component_mutex));
@@ -414,16 +391,21 @@ int component_bind_all(struct device *master_dev, void *data)if (!master)return -EINVAL ; -list_for_each_entry (c, &master->components, master_node) { - ret =component_bind (c, master, data); -if (ret) -break ; - } +/* Bind components in match order */ +for (i =0 ; i < master->match->num; i++) +if (!master->match->compare[i].duplicate) { + c = master->match->compare[i].component; + ret =component_bind (c, master, data); +if (ret) +break ; + }if (ret !=0 ) { -list_for_each_entry_continue_reverse (c, &master->components, - master_node) -component_unbind (c, master, data); +for (; i--; ) +if (!master->match->compare[i].duplicate) { + c = master->match->compare[i].component; +component_unbind (c, master, data); + } }return ret;
@@ -471,8 +453,10 @@ void component_del(struct device *dev, const struct component_ops *ops)break ; } -if (component && component->master) +if (component && component->master) {take_down_master (component->master); +remove_component (component->master, component); + }mutex_unlock (&component_mutex);
Signature · 1 line
1.8.3.1
Quoted 21 lines · Russell King - ARM Linux
A while back, Laurent raised some comments about the component helper, which this patch set starts to address.
The first point it addresses is the repeated parsing inefficiency when deferred probing occurs. When DT is used, the structure of the component helper today means that masters end up parsing the device tree for each attempt to re-bind the driver.
We remove this inefficiency by creating an array of matching data and functions, which the component helper can use internally to match up components to their master.
The second point was the inefficiency of destroying the list of components each time we saw a failure. We did this to ensure that we kept things correctly ordered: component bind order matters. As we have an array instead, the array is already ordered, so we use this array to store the component pointers instead of a list, and remember which are duplicates (and thus should be avoided.) Avoiding the right duplicates matters as we walk the array in the opposite direction at tear down.
Quoted 4 lines
I would like to see patches 1-5 scheduled for the next merge window, with 6-8 for the following window - this gives us grace of one kernel cycle to ensure that any new component helper users are properly converted.
Afaict the actual patches haven't made it to dri-devel, only to linux-arm-kernel. Are they stuck somewhere?
-Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Quoted 23 lines · Daniel Vetter
Quoted 21 lines · Russell King - ARM Linux
A while back, Laurent raised some comments about the component helper, which this patch set starts to address.
The first point it addresses is the repeated parsing inefficiency when deferred probing occurs. When DT is used, the structure of the component helper today means that masters end up parsing the device tree for each attempt to re-bind the driver.
We remove this inefficiency by creating an array of matching data and functions, which the component helper can use internally to match up components to their master.
The second point was the inefficiency of destroying the list of components each time we saw a failure. We did this to ensure that we kept things correctly ordered: component bind order matters. As we have an array instead, the array is already ordered, so we use this array to store the component pointers instead of a list, and remember which are duplicates (and thus should be avoided.) Avoiding the right duplicates matters as we walk the array in the opposite direction at tear down.
Quoted 7 lines
Quoted 4 lines
I would like to see patches 1-5 scheduled for the next merge window, with 6-8 for the following window - this gives us grace of one kernel cycle to ensure that any new component helper users are properly converted.
Afaict the actual patches haven't made it to dri-devel, only to linux-arm-kernel. Are they stuck somewhere?
The patches themselves end up being Cc'd depending on their content and the contents of the MAINTAINERS file, unless I specifically tell my scripts that the patches are to be sent to/cc people - generally I do that for the primary recipients of the series.
That means only the patch(es) which touch DRM stuff were copied to dri-devel, in this case, that being the MSM one.
Signature · 2 lines
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.
Quoted 12 lines · Russell King - ARM Linux
A while back, Laurent raised some comments about the component helper, which this patch set starts to address.
The first point it addresses is the repeated parsing inefficiency when deferred probing occurs. When DT is used, the structure of the component helper today means that masters end up parsing the device tree for each attempt to re-bind the driver.
We remove this inefficiency by creating an array of matching data and functions, which the component helper can use internally to match up components to their master.
Quoted 8 lines
The second point was the inefficiency of destroying the list of components each time we saw a failure. We did this to ensure that we kept things correctly ordered: component bind order matters. As we have an array instead, the array is already ordered, so we use this array to store the component pointers instead of a list, and remember which are duplicates (and thus should be avoided.) Avoiding the right duplicates matters as we walk the array in the opposite direction at tear down.
I've been looking at converting the Tegra DRM driver to the component helpers for a while now and had to make some changes to make it work for that particular use-case. While updating the imx-drm and msm DRM drivers for those changes I noticed an oddity. Both of the existing drivers use the following pattern:
static int driver_component_bind(struct device *dev,
struct device *master,
void *data)
{
allocate memory
request resources
...
hook up to subsystem
...
enable hardware
}
static const struct component_ops driver_component_ops = {
.bind = driver_component_bind,
};
static int driver_probe(struct platform_device *pdev)
{
return component_add(&pdev->dev, &driver_component_ops);
}While converting Tegra DRM, what I intuitively did (I didn't actually look at the other drivers for inspiration) was something more along the lines of the following:
static int driver_component_bind(struct device *dev,
struct device *master,
void *data)
{
hook up to subsystem
...
enable hardware
}
static const struct component_ops driver_component_ops = {
.bind = driver_component_bind,
};
static int driver_probe(struct platform_device *pdev)
{
allocate memory
request resources
...
return component_add(&pdev->dev, &driver_component_ops);
}Since usually deferred probing is caused by resource allocations failing this has the side-effect of handling deferred probing before the master device is even bound (the component_add() happens as the very last step) and therefore there is less risk for component_bind_all() to fail. I've actually never seen it fail at all. Failure at that point is almost certainly irrecoverable anyway.
It would seem to me that if other drivers followed the same pattern, the second point above is solved by moving deferred probe handling one level up and reduce the work of the component helpers to gluing together the components on a subsystem level.
Another advantage to that pattern is that probe failure happens on a much more granular level. It's handled by each component device rather than all at once when the master is bound. By that time all components will be ready and the heavy work of building the subsystem device will usually not have to be undone as opposed to the former pattern where that process is bound to be interrupted possibly many times be deferred probing.
But perhaps I'm missing something. Was there another reason for choosing this particular pattern?
Thierry
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Quoted 52 lines · Thierry Reding
I've been looking at converting the Tegra DRM driver to the component helpers for a while now and had to make some changes to make it work for that particular use-case. While updating the imx-drm and msm DRM drivers for those changes I noticed an oddity. Both of the existing drivers use the following pattern:
static int driver_component_bind(struct device *dev, struct device *master, void *data) { allocate memory request resources ... hook up to subsystem ... enable hardware } static const struct component_ops driver_component_ops = { .bind = driver_component_bind, }; static int driver_probe(struct platform_device *pdev) { return component_add(&pdev->dev, &driver_component_ops); }While converting Tegra DRM, what I intuitively did (I didn't actually look at the other drivers for inspiration) was something more along the lines of the following:
static int driver_component_bind(struct device *dev, struct device *master, void *data) { hook up to subsystem ... enable hardware } static const struct component_ops driver_component_ops = { .bind = driver_component_bind, }; static int driver_probe(struct platform_device *pdev) { allocate memory request resources ... return component_add(&pdev->dev, &driver_component_ops); }
Quoted 6 lines
Since usually deferred probing is caused by resource allocations failing this has the side-effect of handling deferred probing before the master device is even bound (the
component_add()happens as the very last step) and therefore there is less risk forcomponent_bind_all()to fail. I've actually never seen it fail at all. Failure at that point is almost certainly irrecoverable anyway.
It isn't irrecoverable - that case is handled.
I really don't like two-stage driver initialisation - it increases the chances of bugs creeping in. Take for example this code:
probe()
{
priv = devm_kzalloc(dev, whatever);
priv->mem = devm_ioremap_resource(dev, res);
dev_set_drvdata(dev, priv);
return component_add(dev, &ops);
}So far so good, not much can go wrong at that point - we know exactly what state the 'priv' structure is at the point where the component_add call is made.
Now, when the ops' bind method is called, we retrieve the private data. At this point, we can no longer rely on the initialisation state of many of the members. We can't assume that they were zero when we're called, because we can have this sequence of events:
- driver is probed
- component is bound
- component is unbound
- component is bound
At this point, the private data will be dirty. This actually makes the use of devm_kzalloc() a joke in the probe function - although it does initialise all members to zero, we can't rely on that at all when the component is bound.
While the driver itself may be coded for this to be safe, can we say the same for any structures which are embedded into the private data, which may be private to other subsystems?
By way of illustration, ASoC can also have this two stage approach. I'll draw your attention to SGTL5000, and the recent patch I submitted (which I don't think will be taken.) This driver suffers badly if the ASoC "card" is bound, then unbound, and an attempt to rebind it again. That's because the driver gets some managed resources in both the first stage and the second stage, and expects them to be automatically released in when the second stage is torn down. This bug has existed for a very long time, and has gone unnoticed (it will be unnoticed until you try to debug by removing modules and trying to load replacements, which is how I found it.)
That exact bug can't happen with the component helpers, because I explicitly thought about the handling of managed resources, and added the necessary support to deal with these correctly. However, it serves as an example that, despite comments from people saying that my fear is unlikely to happen, we already have code which suffers from issues with two-stage initialisation.
The unfortunate thing is that validation testing for Linux tends not to venture much past "does it boot", "are my devices present" and "can I run some programs". It doesn't cover system shutdown/reboot very often (we've had bugs which have been present for ages there - my test farm explicitly does a power off after boot testing now) and it hardly ever covers drivers being unbound or module removal.
Signature · 2 lines
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.