linux-kernel - Re: [PATCH 2/2] ACPI: ARM Performance Monitoring Unit Table (APMT) initial support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220504182633.a3mwuiohfqtjvpep@bogus>
Date:   Wed, 4 May 2022 19:26:33 +0100
From:   Sudeep Holla <sudeep.holla@....com>
To:     Besar Wicaksono <bwicaksono@...dia.com>, rafael@...nel.org
Cc:     lenb@...nel.org, robert.moore@...el.com, catalin.marinas@....com,
        will@...nel.org, lorenzo.pieralisi@....com, guohanjun@...wei.com,
        linux-tegra@...r.kernel.org, treding@...dia.com,
        jonathanh@...dia.com, linux-acpi@...r.kernel.org, devel@...ica.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 2/2] ACPI: ARM Performance Monitoring Unit Table (APMT)
 initial support

Hi Besar, Rafael,

Sorry for the delayed response.

On Tue, Apr 19, 2022 at 03:54:32PM -0500, Besar Wicaksono wrote:
> ARM Performance Monitoring Unit Table describes the properties of PMU
> support in ARM-based system. The APMT table contains a list of nodes,
> each represents a PMU in the system that conforms to ARM CoreSight PMU
> architecture. The properties of each node include information required
> to access the PMU (e.g. MMIO base address, interrupt number) and also
> identification. For more detailed information, please refer to the
> specification below:
>  * APMT: https://developer.arm.com/documentation/den0117/latest
>  * ARM Coresight PMU:
>         https://developer.arm.com/documentation/ihi0091/latest
> 
> The initial support adds the detection of APMT table and generic
> infrastructure to create platform devices for ARM CoreSight PMUs.
> Similar to IORT the root pointer of APMT is preserved during runtime
> and each PMU platform device is given a pointer to the corresponding
> APMT node.
>

Hi Besar,

This patch on its own looks fine and happy to ack. However I would like
to know general process on such changes that add platform or any bus
device but the driver itself is not upstream.

Any particular reason why you would like to rush and push this without
the actual driver to probe the device being added here ?

> Signed-off-by: Besar Wicaksono <bwicaksono@...dia.com>
> ---
>  arch/arm64/Kconfig          |   1 +
>  drivers/acpi/arm64/Kconfig  |   3 +
>  drivers/acpi/arm64/Makefile |   1 +
>  drivers/acpi/arm64/apmt.c   | 176 ++++++++++++++++++++++++++++++++++++
>  drivers/acpi/bus.c          |   2 +
>  include/linux/acpi_apmt.h   |  19 ++++
>  6 files changed, 202 insertions(+)
>  create mode 100644 drivers/acpi/arm64/apmt.c
>  create mode 100644 include/linux/acpi_apmt.h
>

[...]

> diff --git a/drivers/acpi/arm64/apmt.c b/drivers/acpi/arm64/apmt.c
> new file mode 100644
> index 000000000000..8b8b9f480548
> --- /dev/null
> +++ b/drivers/acpi/arm64/apmt.c
> @@ -0,0 +1,176 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ARM APMT table support.
> + * Design document number: ARM DEN0117.
> + *
> + * Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES.
> + *
> + */
> +
> +#define pr_fmt(fmt)	"ACPI: APMT: " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +
> +#define DEV_NAME "arm-csite-pmu"
> +

I really don't prefer this name:
1. arm-coresight-pmu is much better than "csite". I see the short form
   used elsewhere in the kernel is just "cs" as in cs_etm,...etc
2. Since APMT is more generic than just coresight(I understand coresight
   was the initial motivation for the generic specification) and also
   the type list seem to cover memory controller, SMMU,..etc, does
   it make sense to call it "arm-generic-pmu" or something similar.

Anyways, it can be part of the driver discussion as people might have
opinion based on what and how the driver covers the variety of PMU
types possible as described in APMT.

Not sure if the same device name will be re-used or PMUs can be registered
with different name under perf subsystem, but the name matters for the user
space tools and decoders. They may use the name or type information to analyse
the data samples.

So it is better to wait for all those discussion as part of the driver
upstreaming before you use this device name unless we are absolutely sure
the PMUs can be registered with different names in the driver(which could
be possible, I just don't know)

Apart from this name, I am OK with the changes here and happy to ack if it
is OK to merge this without any driver to probe this device yet.

-- 
Regards,
Sudeep