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

[Suzuki K Poulose]

On 08/06/2020 06:39 PM, Robin Murphy wrote:
> 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
>>>>> ---
>>>>>   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 at 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.

Right. It should be the coresight module, as it holds the PMU code
and THIS_MODULE is sufficient.

> 
> 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.

This is taken care of by holding module reference whenever we prepare
a perf session.

Suzuki