Date:   Tue, 2 Aug 2022 23:04:34 +0000
From:   Besar Wicaksono <bwicaksono@...dia.com>
To:     Suzuki K Poulose <suzuki.poulose@....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

> -----Original Message-----
> From: Suzuki K Poulose <suzuki.poulose@....com>
> Sent: Tuesday, August 2, 2022 4:39 AM
> To: Besar Wicaksono <bwicaksono@...dia.com>; Mathieu Poirier
> <mathieu.poirier@...aro.org>
> Cc: 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
> 
> 
> 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 ?
> 

In that case, including "Coresight Architecture" in the name makes more sense.
I am fine with "arm_cspmu" and "arm-cs-arch-pmu".

Thanks,
Besar

> 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