On 25 May 2018 at 11:12, Suzuki K Poulose <suzuki.poul...@arm.com> wrote:
> 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.poir...@linaro.org>
>> Cc: Alexander Shishkin <alexander.shish...@linux.intel.com>
>> Cc: Randy Dunlap <rdun...@infradead.org>
>> Signed-off-by: Kim Phillips <kim.phill...@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 ?
Very good point - this is invariably racy.
>
> 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.
Good idea, it does streamline things a lot.
>
>> +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
