Re: [PATCH v7 11/25] coresight: etb: allow etb to be built as a module

[Suzuki K Poulose <suzuki.poulose@arm.com>]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel