Message-ID: <e11096bd-7053-d7e7-738c-1241f2351307@arm.com>
Date:   Tue, 2 Aug 2022 10:38:47 +0100
From:   Suzuki K Poulose <suzuki.poulose@....com>
To:     Besar Wicaksono <bwicaksono@...dia.com>,
        Mathieu Poirier <mathieu.poirier@...aro.org>
Cc:     Robin Murphy <robin.murphy@....com>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        "will@...nel.org" <will@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
        "sudeep.holla@....com" <sudeep.holla@....com>,
        "thanu.rangarajan@....com" <thanu.rangarajan@....com>,
        "Michael.Williams@....com" <Michael.Williams@....com>,
        Thierry Reding <treding@...dia.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Vikram Sethi <vsethi@...dia.com>,
        "mike.leach@...aro.org" <mike.leach@...aro.org>,
        "leo.yan@...aro.org" <leo.yan@...aro.org>
Subject: Re: [RESEND PATCH v3 1/2] perf: coresight_pmu: Add support for ARM
 CoreSight PMU driver

Hi

On 01/08/2022 23:27, Besar Wicaksono wrote:
> Hi
> 
>> -----Original Message-----
>> From: Mathieu Poirier <mathieu.poirier@...aro.org>
>> Sent: Thursday, July 21, 2022 10:36 AM
>> To: Suzuki K Poulose <suzuki.poulose@....com>
>> Cc: Besar Wicaksono <bwicaksono@...dia.com>; Robin Murphy
>> <robin.murphy@....com>; catalin.marinas@....com; will@...nel.org;
>> mark.rutland@....com; linux-arm-kernel@...ts.infradead.org; linux-
>> kernel@...r.kernel.org; linux-tegra@...r.kernel.org;
>> sudeep.holla@....com; thanu.rangarajan@....com;
>> Michael.Williams@....com; Thierry Reding <treding@...dia.com>; Jonathan
>> Hunter <jonathanh@...dia.com>; Vikram Sethi <vsethi@...dia.com>;
>> mike.leach@...aro.org; leo.yan@...aro.org
>> Subject: Re: [RESEND PATCH v3 1/2] perf: coresight_pmu: Add support for
>> ARM CoreSight PMU driver
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On Thu, 21 Jul 2022 at 03:19, Suzuki K Poulose <suzuki.poulose@....com>
>> wrote:
>>>
>>> Hi
>>>
>>> On 14/07/2022 05:47, Besar Wicaksono wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Robin Murphy <robin.murphy@....com>
>>>>> Sent: Wednesday, July 13, 2022 3:13 AM
>>>>> To: Mathieu Poirier <mathieu.poirier@...aro.org>; Besar Wicaksono
>>>>> <bwicaksono@...dia.com>
>>>>> Cc: Suzuki K Poulose <suzuki.poulose@....com>;
>> catalin.marinas@....com;
>>>>> will@...nel.org; mark.rutland@....com; linux-arm-
>>>>> kernel@...ts.infradead.org; linux-kernel@...r.kernel.org; linux-
>>>>> tegra@...r.kernel.org; sudeep.holla@....com;
>>>>> thanu.rangarajan@....com; Michael.Williams@....com; Thierry
>> Reding
>>>>> <treding@...dia.com>; Jonathan Hunter <jonathanh@...dia.com>;
>> Vikram
>>>>> Sethi <vsethi@...dia.com>; mike.leach@...aro.org; leo.yan@...aro.org
>>>>> Subject: Re: [RESEND PATCH v3 1/2] perf: coresight_pmu: Add support
>> for
>>>>> ARM CoreSight PMU driver
>>>>>
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> On 2022-07-12 17:36, Mathieu Poirier wrote:
>>>>> [...]
>>>>>>>> If we have decied to call this arm_system_pmu, (which I am
>> perfectly
>>>>>>>> happy with), could we please stick to that name for functions that
>> we
>>>>>>>> export ?
>>>>>>>>
>>>>>>>> e.g,
>>>>>>>>
>>>>>
>> s/coresight_pmu_sysfs_event_show/arm_system_pmu_event_show()/
>>>>>>>>
>>>>>>>
>>>>>>> Just want to confirm, is it just the public functions or do we need to
>>>>> replace
>>>>>>> all that has "coresight" naming ? Including the static functions, structs,
>>>>> filename.
>>>>>>
>>>>>> I think all references to "coresight" should be changed to
>>>>> "arm_system_pmu",
>>>>>> including filenames.  That way there is no doubt this IP block is not
>>>>>> related, and does not interoperate, with the any of the "coresight" IP
>>>>> blocks
>>>>>> already supported[1] in the kernel.
>>>>>>
>>>>>> I have looked at the documentation[2] in the cover letter and I agree
>>>>>> with an earlier comment from Sudeep that this IP has very little to do
>> with
>>>>> any
>>>>>> of the other CoreSight IP blocks found in the CoreSight framework[1].
>>>>> Using the
>>>>>> "coresight" naming convention in this driver would be _extremely_
>>>>> confusing,
>>>>>> especially when it comes to exported functions.
>>>>>
>>>>> But conversely, how is it not confusing to make up completely different
>>>>> names for things than what they're actually called? The CoreSight
>>>>> Performance Monitoring Unit is a part of the Arm CoreSight
>> architecture,
>>>>> it says it right there on page 1. What if I instinctively associate the
>>>>> name Mathieu with someone more familiar to me, so to avoid confusion
>> I'd
>>>>> prefer to call you Steve? Is that OK?
>>>>>
>>>>
>>>> What is the naming convention for modules under drivers/perf ?
>>>> In my observation, the names there correspond to the part monitored by
>>>> the PMU. The confusion on using "coresight_pmu" naming could be that
>>>> people may think the PMU monitors coresight system, i.e the trace
>> system under hwtracing.
>>>> However, the driver in this patch is for a new PMU standard that
>> monitors uncore
>>>> parts. Uncore was considered as terminology from Intel, so "system" was
>> picked instead.
>>>> Please see this thread for reference:
>>>>     https://lore.kernel.org/linux-arm-
>> kernel/20220510111318.GD27557@...lie-the-truck/
>>>
>>> I think we all understand the state of affairs.
>>>
>>> - We have an architecutre specification for PMUs, Arm CoreSight PMU
>>> Architecutre, which has absolutely no relationship with :
>>>
>>>      either CoreSight Self-Hosted Tracing (handled by "coresight"
>>> subsystem in the kernel under drivers/hwtracing/coresight/, with a user
>>> visible pmu as "cs_etm")
>>>
>>>      or the CoreSight Architecture (except for the name). This is of less
>>> significance in general. But has a significant impact on the "name"
>>> users might expect for the driver/Kconfig etc.
>>>
>>> - We want to be able to make it easier for the users/developers to
>>> choose what they want without causing confusion.
>>>
>>> For an end-user: Having the PMU instance named after the "System IP"
>>> (as implememented in the driver solves the problem and falling back to
>>> arm_system_pmu is a good enough choice. So let us stick with that)
>>>
>>> Kconfig: May be we can choose
>>> CONFIG_ARM_CORESIGHT_PMU_ARCH_PMU
>>> or even
>>> CONFIG_ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU
>>>
>>> with appropriate help text to ensure there is enough stress about what
>>> this is and what this is not would be sufficient.
>>>
> 
> CONFIG_ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU sounds good to me.
> 
>>> Now the remaining contention is about the name of the "subsystem" and
>>> also the dir/files.  This may sound insignificant. But it is also
>>> important to get this right. e.g., helps the reviewers unambiguously
>>> identify the change or maintainers accepting pull requests (remember
>>> these two PMUs (cs_etm and this one) go via different trees.). Not
>>> everyone who deals with this in the community may be aware of how
>>> these are different.
>>>
>>> We could choose arm_cspmu_ or simply cspmu. Given that only the
>>> "normal" users care about the "association" with the "architecture"
>>> and more advanced users (e.g, developers) can easily map "Kconfig"
>>> to driver files, may be we could even stick to the "arm_syspmu"
>>> (from "arm system pmu") ?
>>>
>>
>> +1 on "arm_syspmu"
>>
> 
> I am fine too with arm_syspmu.
> 
> If there is no objection, I am going to post new update by end of this week
> or early next week.

Unfortunately, I have been told that we have a potential problem with
"arm_syspmu" and even choosing "arm-system-pmu" for the name as it may
conflict with something that is coming soon. So we may have to go back
to something else, to avoid this exact same conversation in the near
future. Apologies for that.

Could we use "arm_cspmu" for the code/subsystem and may be
"arm-csarch-pmu" / "arm-cs-arch-pmu" for the device name ?

Other suggestions ?

Suzuki


> 
> Thanks,
> Besar
> 
>>> Suzuki
>>>
>>>
>>>>
>>>>> As it happens, Steve, I do actually agree with you that "coresight_" is
>>>>> a bad prefix here, but only for the reason that it's too general. TBH I
>>>>> think that's true of the existing Linux subsystem too, but that damage
>>>>> is already done, and I'd concur that there's little value in trying to
>>>>> unpick that now, despite the clear existence of products like CoreSight
>>>>> DAP and CoreSight ELA which don't have all that much to do with
>> program
>>>>> trace either.
>>>>>
>>>>> However, hindsight and inertia are hardly good reasons to double down
>> on
>>>>> poor decisions, so if I was going to vote for anything here it would be
>>>>> "cspmu_", which is about as
>>>>> obviously-related-to-the-thing-it-actually-is as we can get while also
>>>>> being pleasantly concise.
>>>>>
>>>>> [ And no, this isn't bikeshedding. Naming things right is *important* ]
>>>>>
>>>>
>>>> I agree having the correct name is important, especially at this early stage.
>>>> A direction of what the naming should describe would be very helpful
>> here.
>>>>
>>>>> Cheers,
>>>>> Robin.
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Steve
>>>>>>
>>>>>> [1]. drivers/hwtracing/coresight/
>>>>>> [2]. https://developer.arm.com/documentation/ihi0091/latest
>>>

Powered by blists - more mailing lists