Re: [PATCH 6/6] coresight: allow to build as modules

[Suzuki K Poulose]

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@xxxxxxxxxx>
> Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> Cc: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
> Signed-off-by: Kim Phillips <kim.phillips@xxxxxxx>

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