lkml.org 
[lkml]   [2022]   [Jul]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [RESEND PATCH v3 1/2] perf: coresight_pmu: Add support for ARM CoreSight PMU driver
    Date


    > -----Original Message-----
    > From: Robin Murphy <robin.murphy@arm.com>
    > Sent: Wednesday, July 13, 2022 3:13 AM
    > To: Mathieu Poirier <mathieu.poirier@linaro.org>; Besar Wicaksono
    > <bwicaksono@nvidia.com>
    > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>; catalin.marinas@arm.com;
    > will@kernel.org; mark.rutland@arm.com; linux-arm-
    > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
    > tegra@vger.kernel.org; sudeep.holla@arm.com;
    > thanu.rangarajan@arm.com; Michael.Williams@arm.com; Thierry Reding
    > <treding@nvidia.com>; Jonathan Hunter <jonathanh@nvidia.com>; Vikram
    > Sethi <vsethi@nvidia.com>; mike.leach@linaro.org; leo.yan@linaro.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@willie-the-truck/

    > 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
    \
     
     \ /
      Last update: 2022-07-14 06:59    [W:3.500 / U:0.440 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site