lkml.org 
[lkml]   [2018]   [May]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 6/6] coresight: allow to build as modules
From
Date
On 18/05/18 02:20, Kim Phillips wrote:
> Allow to build coresight as modules. This greatly enhances developer
> efficiency by allowing the development to take place exclusively on the
> target, and without needing to reboot in between changes.
>
> - Kconfig bools become tristates, to allow =m
>
> - use -objs to denote merge object directives in Makefile, adds a
> coresight-core nomenclature for the base module.
>
> - Export core functions so as to be able to be used by
> non-core modules.
>
> - add a coresight_exit() that unregisters the coresight bus, add
> remove fns for most others.
>
> - fix up modules with ID tables for autoloading on boot
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Signed-off-by: Kim Phillips <kim.phillips@arm.com>

Kim,

I see that you have addressed my comments on a previous version
of this series posted in April. But I don't see the version number
increased for this new version. Please add versioning to make it
easier to make it more obvious.

Also, generally it is a good idea to keep the people who reviewed
and commented on your previous versions in the newer versions.

Some comments below :

> diff --git a/drivers/hwtracing/coresight/coresight-dynamic-replicator.c b/drivers/hwtracing/coresight/coresight-dynamic-replicator.c
> index fc742215ab05..bc42b8022556 100644
> --- a/drivers/hwtracing/coresight/coresight-dynamic-replicator.c
> +++ b/drivers/hwtracing/coresight/coresight-dynamic-replicator.c
> @@ -37,7 +37,12 @@ struct replicator_state {
> static int replicator_enable(struct coresight_device *csdev, int inport,
> int outport)
> {
> - struct replicator_state *drvdata = dev_get_drvdata(csdev->dev.parent);
> + struct device *parent_dev = csdev->dev.parent;
> + struct replicator_state *drvdata = dev_get_drvdata(parent_dev);
> + struct module *module = parent_dev->driver->owner;
> +
> + if (!try_module_get(module))
> + return -ENODEV;
>
> CS_UNLOCK(drvdata->base);

What is the guarantee that the "csdev" is still available when we reach
here ?

A module could be unloaded "after the component was added to the path"
(via coresight_build_path) and before we invoke the "enable" on each
component in the path ?

Also, it is tedious to do module_get in "enable" and module_put in the
disable call backs for each component.

Instead, if we do a module_get() in build_path and module_put() in
release path, we could solve all these problems and keep it the module
refcount in a central place.

> +MODULE_DEVICE_TABLE(amba, replicator_ids);
> +
> static struct amba_driver replicator_driver = {
> .drv = {
> .name = "coresight-dynamic-replicator",
> @@ -207,9 +226,10 @@ static struct amba_driver replicator_driver = {
> .suppress_bind_attrs = true,
> },
> .probe = replicator_probe,
> + .remove = replicator_remove,
> .id_table = replicator_ids,
> };

Do we have the owner field set here for this driver ? I see that you
added it for some components and not others. e.g, you have added it for
etm4x, while not for replicator and others.


> +MODULE_DEVICE_TABLE(amba, etm4_ids);
> +
> static struct amba_driver etm4x_driver = {
> .drv = {
> .name = "coresight-etm4x",
> + .owner = THIS_MODULE,
> .suppress_bind_attrs = true,
> },
> .probe = etm4_probe,
> + .remove = etm4_remove,
> .id_table = etm4_ids,
> };
> -builtin_amba_driver(etm4x_driver);
> +module_amba_driver(etm4x_driver);


Suzuki

\
 
 \ /
  Last update: 2018-05-25 19:13    [W:0.293 / U:2.492 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site