Re: [PATCH v7 25/25] coresight: allow the coresight core driver to be built as a module

[Robin Murphy <robin.murphy@arm.com>]

On 2020-08-06 18:25, Robin Murphy wrote:
> On 2020-08-06 17:33, Suzuki K Poulose wrote:
>> On 08/05/2020 05:29 PM, Suzuki K Poulose wrote:
>>> On 08/05/2020 03:54 AM, Tingwei Zhang wrote:
>>>> Enhance coresight developer's efficiency to debug coresight drivers.
>>>> - Kconfig becomes a tristate, to allow =m
>>>> - append -core to source file name to allow module to
>>>>    be called coresight by the Makefile
>>>> - modules can have only one init/exit, so we add the etm_perf
>>>>    register/unregister function calls to the core init/exit
>>>>    functions.
>>>> - 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/Makefile          |  5 ++-
>>>>   .../{coresight.c => coresight-core.c}         | 42 
>>>> ++++++++++++++-----
>>>>   .../hwtracing/coresight/coresight-etm-perf.c  |  8 +++-
>>>>   .../hwtracing/coresight/coresight-etm-perf.h  |  3 ++
>>>>   5 files changed, 48 insertions(+), 15 deletions(-)
>>>>   rename drivers/hwtracing/coresight/{coresight.c => 
>>>> coresight-core.c} (98%)
>>>
>>> Personally, I would like to rename this to core.c dropping the
>>> "coresight-" prefix here (now that we have to do a rename). And we
>>> should do that ideally for all the other files (but not proposing
>>> it to be part of this series, and could be something that we could
>>> pursue if everyone agrees to it).
>>>
>>> We are inside the coresight directory anyways and having a prefix
>>> doesn't help with anything.
>>>
>>> The patch as such looks good to me.
>>>
>>> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>
>> On a second look, I believe for the sake of completion, we
>> should set the "owner" of the etm, now that we are a module.
>> The question is, which one should that be. It could be the
>> "coresight" or the "coresight-etm{3,4}x".
>>
>> I believe the "coresight" is the better choice.
> 
> If you mean pmu->owner, you shouldn't really have much of a choice - it

...by which I meant pmu->module, obviously. Oops :)

Anyway, that should certainly be set not just for completeness but for 
correctness, since there do exist users who are adventurous enough to 
try unloading modules while perf is running.

Robin.

> should be the module containing the actual PMU callbacks, such that they 
> can't suddenly disappear while the PMU is in use. Allowing perf to take 
> a reference to some other module and not actually protect itself would 
> not be good. It should be pretty rare that the correct owner is anything 
> other than THIS_MODULE ;)
> 
> Hopefully the dependencies are such that the core module automatically 
> holds its own reference to the individual ETM driver module(s) by the 
> time it registers the PMU.
> 
> Robin.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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