Messages in this thread | | | From | Mathieu Poirier <> | Date | Fri, 25 May 2018 11:27:46 -0600 | Subject | Re: [PATCH 6/6] coresight: allow to build as modules |
| |
On 25 May 2018 at 11:12, Suzuki K Poulose <Suzuki.Poulose@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.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 ?
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
| |