| Message ID | 20200805025458.2978-12-tingwei@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: allow to build coresight as modules | expand |
On 08/05/2020 03:54 AM, Tingwei Zhang wrote: > From: Kim Phillips <kim.phillips@arm.com> > > Allow to build coresight-etb10 as a module, for ease of development. > > - Kconfig becomes a tristate, to allow =m > - add an etb_remove function, for module unload > - add a MODULE_DEVICE_TABLE for autoloading on boot > > Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > Cc: Leo Yan <leo.yan@linaro.org> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> > Cc: Randy Dunlap <rdunlap@infradead.org> > Cc: Suzuki K Poulose <Suzuki.Poulose@arm.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Russell King <linux@armlinux.org.uk> > Signed-off-by: Kim Phillips <kim.phillips@arm.com> > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org> > Tested-by: Mike Leach <mike.leach@linaro.org> > --- > drivers/hwtracing/coresight/Kconfig | 5 ++++- > drivers/hwtracing/coresight/coresight-etb10.c | 20 ++++++++++++++++++- > 2 files changed, 23 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig > index d6e107bbd30b..996d84a1edb8 100644 > --- a/drivers/hwtracing/coresight/Kconfig > +++ b/drivers/hwtracing/coresight/Kconfig > @@ -57,13 +57,16 @@ config CORESIGHT_SINK_TPIU > the on-board coresight memory can handle. > > config CORESIGHT_SINK_ETBV10 > - bool "Coresight ETBv1.0 driver" > + tristate "Coresight ETBv1.0 driver" > depends on CORESIGHT_LINKS_AND_SINKS > help > This enables support for the Embedded Trace Buffer version 1.0 driver > that complies with the generic implementation of the component without > special enhancement or added features. > > + To compile this driver as a module, choose M here: the > + module will be called coresight-etb10. > + > config CORESIGHT_SOURCE_ETM3X > tristate "CoreSight Embedded Trace Macrocell 3.x driver" > depends on !ARM64 > diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c > index 04ee9cda988d..b40756497c9a 100644 > --- a/drivers/hwtracing/coresight/coresight-etb10.c > +++ b/drivers/hwtracing/coresight/coresight-etb10.c > @@ -801,6 +801,16 @@ static int etb_probe(struct amba_device *adev, const struct amba_id *id) > return ret; > } > > +static int __exit etb_remove(struct amba_device *adev) > +{ > + struct etb_drvdata *drvdata = dev_get_drvdata(&adev->dev); > + > + misc_deregister(&drvdata->miscdev); > + coresight_unregister(drvdata->csdev); > + > + return 0; > +} > + I am worried about the dangling reference via the misc device to the drvdata. Not sure, if we need to grab the reference the module for each open fd on the misc device. The misc device infrastructure doesn't seem to do any of this implicitly. This is something worth checking. I will see if I can trigger this. i.e, CPU 0: CPU1: open("/dev/etb") etb_open() ... etb_remove() { .. coresight_unregister() } read() etb_read() deal with free'd drvdata ? Cheers Suzuki
On Wed, Aug 05, 2020 at 11:43:36PM +0800, Suzuki K Poulose wrote: > On 08/05/2020 03:54 AM, Tingwei Zhang wrote: > >From: Kim Phillips <kim.phillips@arm.com> > > > >Allow to build coresight-etb10 as a module, for ease of development. > > > >- Kconfig becomes a tristate, to allow =m > >- add an etb_remove function, for module unload > >- add a MODULE_DEVICE_TABLE for autoloading on boot > > > >Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > >Cc: Leo Yan <leo.yan@linaro.org> > >Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> > >Cc: Randy Dunlap <rdunlap@infradead.org> > >Cc: Suzuki K Poulose <Suzuki.Poulose@arm.com> > >Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > >Cc: Russell King <linux@armlinux.org.uk> > >Signed-off-by: Kim Phillips <kim.phillips@arm.com> > >Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org> > >Tested-by: Mike Leach <mike.leach@linaro.org> > > > >--- > > drivers/hwtracing/coresight/Kconfig | 5 ++++- > > drivers/hwtracing/coresight/coresight-etb10.c | 20 ++++++++++++++++++- > > 2 files changed, 23 insertions(+), 2 deletions(-) > > > >diff --git a/drivers/hwtracing/coresight/Kconfig > >b/drivers/hwtracing/coresight/Kconfig > >index d6e107bbd30b..996d84a1edb8 100644 > >--- a/drivers/hwtracing/coresight/Kconfig > >+++ b/drivers/hwtracing/coresight/Kconfig > >@@ -57,13 +57,16 @@ config CORESIGHT_SINK_TPIU > > the on-board coresight memory can handle. > > > > config CORESIGHT_SINK_ETBV10 > >- bool "Coresight ETBv1.0 driver" > >+ tristate "Coresight ETBv1.0 driver" > > depends on CORESIGHT_LINKS_AND_SINKS > > help > > This enables support for the Embedded Trace Buffer version 1.0 driver > > that complies with the generic implementation of the component > >without > > special enhancement or added features. > > > >+ To compile this driver as a module, choose M here: the > >+ module will be called coresight-etb10. > >+ > > config CORESIGHT_SOURCE_ETM3X > > tristate "CoreSight Embedded Trace Macrocell 3.x driver" > > depends on !ARM64 > >diff --git a/drivers/hwtracing/coresight/coresight-etb10.c > >b/drivers/hwtracing/coresight/coresight-etb10.c > >index 04ee9cda988d..b40756497c9a 100644 > >--- a/drivers/hwtracing/coresight/coresight-etb10.c > >+++ b/drivers/hwtracing/coresight/coresight-etb10.c > >@@ -801,6 +801,16 @@ static int etb_probe(struct amba_device *adev, const > >struct amba_id *id) > > return ret; > > } > > > >+static int __exit etb_remove(struct amba_device *adev) > >+{ > >+ struct etb_drvdata *drvdata = dev_get_drvdata(&adev->dev); > >+ > >+ misc_deregister(&drvdata->miscdev); > >+ coresight_unregister(drvdata->csdev); > >+ > >+ return 0; > >+} > >+ > > I am worried about the dangling reference via the misc device to > the drvdata. Not sure, if we need to grab the reference the module > for each open fd on the misc device. The misc device infrastructure > doesn't seem to do any of this implicitly. This is something worth > checking. I will see if I can trigger this. > Misc device is one char device. cdev_get() is called in chrdev_open() to grab reference of module. Look like we are safe here. Thanks, Tingwei > i.e, > > CPU 0: CPU1: > > open("/dev/etb") > etb_open() > ... etb_remove() { > .. > coresight_unregister() > } > > > read() > etb_read() > deal with free'd drvdata ? > > > Cheers > Suzuki
On 08/06/2020 12:19 PM, Tingwei Zhang wrote: > On Wed, Aug 05, 2020 at 11:43:36PM +0800, Suzuki K Poulose wrote: >> On 08/05/2020 03:54 AM, Tingwei Zhang wrote: >>> From: Kim Phillips <kim.phillips@arm.com> >>> >>> Allow to build coresight-etb10 as a module, for ease of development. >>> >>> - Kconfig becomes a tristate, to allow =m >>> - add an etb_remove function, for module unload >>> - add a MODULE_DEVICE_TABLE for autoloading on boot >>> >>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org> >>> Cc: Leo Yan <leo.yan@linaro.org> >>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> >>> Cc: Randy Dunlap <rdunlap@infradead.org> >>> Cc: Suzuki K Poulose <Suzuki.Poulose@arm.com> >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>> Cc: Russell King <linux@armlinux.org.uk> >>> Signed-off-by: Kim Phillips <kim.phillips@arm.com> >>> Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org> >>> Tested-by: Mike Leach <mike.leach@linaro.org> >> >> >>> --- >>> drivers/hwtracing/coresight/Kconfig | 5 ++++- >>> drivers/hwtracing/coresight/coresight-etb10.c | 20 ++++++++++++++++++- >>> 2 files changed, 23 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/hwtracing/coresight/Kconfig >>> b/drivers/hwtracing/coresight/Kconfig >>> index d6e107bbd30b..996d84a1edb8 100644 >>> --- a/drivers/hwtracing/coresight/Kconfig >>> +++ b/drivers/hwtracing/coresight/Kconfig >>> @@ -57,13 +57,16 @@ config CORESIGHT_SINK_TPIU >>> the on-board coresight memory can handle. >>> >>> config CORESIGHT_SINK_ETBV10 >>> - bool "Coresight ETBv1.0 driver" >>> + tristate "Coresight ETBv1.0 driver" >>> depends on CORESIGHT_LINKS_AND_SINKS >>> help >>> This enables support for the Embedded Trace Buffer version 1.0 driver >>> that complies with the generic implementation of the component >>> without >>> special enhancement or added features. >>> >>> + To compile this driver as a module, choose M here: the >>> + module will be called coresight-etb10. >>> + >>> config CORESIGHT_SOURCE_ETM3X >>> tristate "CoreSight Embedded Trace Macrocell 3.x driver" >>> depends on !ARM64 >>> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c >>> b/drivers/hwtracing/coresight/coresight-etb10.c >>> index 04ee9cda988d..b40756497c9a 100644 >>> --- a/drivers/hwtracing/coresight/coresight-etb10.c >>> +++ b/drivers/hwtracing/coresight/coresight-etb10.c >>> @@ -801,6 +801,16 @@ static int etb_probe(struct amba_device *adev, const >>> struct amba_id *id) >>> return ret; >>> } >>> >>> +static int __exit etb_remove(struct amba_device *adev) >>> +{ >>> + struct etb_drvdata *drvdata = dev_get_drvdata(&adev->dev); >>> + >>> + misc_deregister(&drvdata->miscdev); >>> + coresight_unregister(drvdata->csdev); >>> + >>> + return 0; >>> +} >>> + >> >> I am worried about the dangling reference via the misc device to >> the drvdata. Not sure, if we need to grab the reference the module >> for each open fd on the misc device. The misc device infrastructure >> doesn't seem to do any of this implicitly. This is something worth >> checking. I will see if I can trigger this. >> > > Misc device is one char device. cdev_get() is called in chrdev_open() > to grab reference of module. Look like we are safe here. That is not the case, as the misc_device doesn't have any pointers to who the owner is, that triggered my question. Actually, it is the misc_open() which holds a refcount on the f_ops (which is etb_fops for etb10) owner, thus we are safe here. It may be a good idea to add this comment in etb_remove(), so that we don't have to dig this again. with that: Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> Cheers Suzuki
On Thu, Aug 06, 2020 at 10:56:57PM +0800, Suzuki K Poulose wrote: > On 08/06/2020 12:19 PM, Tingwei Zhang wrote: > >On Wed, Aug 05, 2020 at 11:43:36PM +0800, Suzuki K Poulose wrote: > >>On 08/05/2020 03:54 AM, Tingwei Zhang wrote: > >>>From: Kim Phillips <kim.phillips@arm.com> > >>> > >>>Allow to build coresight-etb10 as a module, for ease of development. > >>> > >>>- Kconfig becomes a tristate, to allow =m > >>>- add an etb_remove function, for module unload > >>>- add a MODULE_DEVICE_TABLE for autoloading on boot > >>> > >>>Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > >>>Cc: Leo Yan <leo.yan@linaro.org> > >>>Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> > >>>Cc: Randy Dunlap <rdunlap@infradead.org> > >>>Cc: Suzuki K Poulose <Suzuki.Poulose@arm.com> > >>>Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > >>>Cc: Russell King <linux@armlinux.org.uk> > >>>Signed-off-by: Kim Phillips <kim.phillips@arm.com> > >>>Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org> > >>>Tested-by: Mike Leach <mike.leach@linaro.org> > >> > >> > >>>--- > >>> drivers/hwtracing/coresight/Kconfig | 5 ++++- > >>> drivers/hwtracing/coresight/coresight-etb10.c | 20 ++++++++++++++++++- > >>> 2 files changed, 23 insertions(+), 2 deletions(-) > >>> > >>>diff --git a/drivers/hwtracing/coresight/Kconfig > >>>b/drivers/hwtracing/coresight/Kconfig > >>>index d6e107bbd30b..996d84a1edb8 100644 > >>>--- a/drivers/hwtracing/coresight/Kconfig > >>>+++ b/drivers/hwtracing/coresight/Kconfig > >>>@@ -57,13 +57,16 @@ config CORESIGHT_SINK_TPIU > >>> the on-board coresight memory can handle. > >>> > >>> config CORESIGHT_SINK_ETBV10 > >>>- bool "Coresight ETBv1.0 driver" > >>>+ tristate "Coresight ETBv1.0 driver" > >>> depends on CORESIGHT_LINKS_AND_SINKS > >>> help > >>> This enables support for the Embedded Trace Buffer version 1.0 > >>>driver > >>> that complies with the generic implementation of the component > >>>without > >>> special enhancement or added features. > >>> > >>>+ To compile this driver as a module, choose M here: the > >>>+ module will be called coresight-etb10. > >>>+ > >>> config CORESIGHT_SOURCE_ETM3X > >>> tristate "CoreSight Embedded Trace Macrocell 3.x driver" > >>> depends on !ARM64 > >>>diff --git a/drivers/hwtracing/coresight/coresight-etb10.c > >>>b/drivers/hwtracing/coresight/coresight-etb10.c > >>>index 04ee9cda988d..b40756497c9a 100644 > >>>--- a/drivers/hwtracing/coresight/coresight-etb10.c > >>>+++ b/drivers/hwtracing/coresight/coresight-etb10.c > >>>@@ -801,6 +801,16 @@ static int etb_probe(struct amba_device *adev, > >>>const > >>>struct amba_id *id) > >>> return ret; > >>> } > >>> > >>>+static int __exit etb_remove(struct amba_device *adev) > >>>+{ > >>>+ struct etb_drvdata *drvdata = dev_get_drvdata(&adev->dev); > >>>+ > >>>+ misc_deregister(&drvdata->miscdev); > >>>+ coresight_unregister(drvdata->csdev); > >>>+ > >>>+ return 0; > >>>+} > >>>+ > >> > >>I am worried about the dangling reference via the misc device to > >>the drvdata. Not sure, if we need to grab the reference the module > >>for each open fd on the misc device. The misc device infrastructure > >>doesn't seem to do any of this implicitly. This is something worth > >>checking. I will see if I can trigger this. > >> > > > >Misc device is one char device. cdev_get() is called in chrdev_open() > >to grab reference of module. Look like we are safe here. > > That is not the case, as the misc_device doesn't have any pointers to > who the owner is, that triggered my question. > > Actually, it is the misc_open() which holds a refcount on the > f_ops (which is etb_fops for etb10) owner, thus we are safe here. > You are correct. It's in fops_get(c->fops. > It may be a good idea to add this comment in etb_remove(), so that > we don't have to dig this again. > I'll add comment to etb_remove. Thanks, Tingwei > with that: > > Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> > > Cheers > Suzuki
diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig index d6e107bbd30b..996d84a1edb8 100644 --- a/drivers/hwtracing/coresight/Kconfig +++ b/drivers/hwtracing/coresight/Kconfig @@ -57,13 +57,16 @@ config CORESIGHT_SINK_TPIU the on-board coresight memory can handle. config CORESIGHT_SINK_ETBV10 - bool "Coresight ETBv1.0 driver" + tristate "Coresight ETBv1.0 driver" depends on CORESIGHT_LINKS_AND_SINKS help This enables support for the Embedded Trace Buffer version 1.0 driver that complies with the generic implementation of the component without special enhancement or added features. + To compile this driver as a module, choose M here: the + module will be called coresight-etb10. + config CORESIGHT_SOURCE_ETM3X tristate "CoreSight Embedded Trace Macrocell 3.x driver" depends on !ARM64 diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c index 04ee9cda988d..b40756497c9a 100644 --- a/drivers/hwtracing/coresight/coresight-etb10.c +++ b/drivers/hwtracing/coresight/coresight-etb10.c @@ -801,6 +801,16 @@ static int etb_probe(struct amba_device *adev, const struct amba_id *id) return ret; } +static int __exit etb_remove(struct amba_device *adev) +{ + struct etb_drvdata *drvdata = dev_get_drvdata(&adev->dev); + + misc_deregister(&drvdata->miscdev); + coresight_unregister(drvdata->csdev); + + return 0; +} + #ifdef CONFIG_PM static int etb_runtime_suspend(struct device *dev) { @@ -835,6 +845,8 @@ static const struct amba_id etb_ids[] = { { 0, 0}, }; +MODULE_DEVICE_TABLE(amba, etb_ids); + static struct amba_driver etb_driver = { .drv = { .name = "coresight-etb10", @@ -844,6 +856,12 @@ static struct amba_driver etb_driver = { }, .probe = etb_probe, + .remove = etb_remove, .id_table = etb_ids, }; -builtin_amba_driver(etb_driver); +module_amba_driver(etb_driver); + +MODULE_AUTHOR("Pratik Patel <pratikp@codeaurora.org>"); +MODULE_AUTHOR("Mathieu Poirier <mathieu.poirier@linaro.org>"); +MODULE_DESCRIPTION("Arm CoreSight Embedded Trace Buffer driver"); +MODULE_LICENSE("GPL v2");