Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 5 Jun 2023 00:12:23 -0700 (PDT)
From:   Ilkka Koskinen <ilkka@os.amperecomputing.com>
To:     Robin Murphy <robin.murphy@arm.com>
cc:     will@kernel.org, mark.rutland@arm.com, suzuki.poulose@arm.com,
        bwicaksono@nvidia.com, ilkka@os.amperecomputing.com,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/4] perf/arm_cspmu: Decouple APMT dependency
In-Reply-To: <3509b299b19b8bf89700c77c2bb695c740926ae7.1685619571.git.robin.murphy@arm.com>
Message-ID: <be916ba8-1d2a-89dd-7374-b07618fa9b29@os.amperecomputing.com>
References: <cover.1685619571.git.robin.murphy@arm.com> <3509b299b19b8bf89700c77c2bb695c740926ae7.1685619571.git.robin.murphy@arm.com>
Content-Type: text/plain; format=flowed; charset=US-ASCII
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?7uNvCAYVUPJBVtdqwhDkoxDPufTEW9QKwnpWwuJg6SQhjwHLM2A2WzqEvYJn?=
 =?us-ascii?Q?i7zT+xBdnAuACLoIf7dnjJ9tDqsXXUg//o75TIevOGIQAFdEhSLC4uYKQ9LG?=
 =?us-ascii?Q?S9M23X6n8S/BKCB51fl65wyU2lsfsu3f/1oIwLpRqN5ucsTQQjQ9gCWTTkTV?=
 =?us-ascii?Q?CuyI9t8T7lALVs+toqRUxeyKAvBvOj1+dnosXMvLicPzXAfb2BBXbw+T8GV3?=
 =?us-ascii?Q?9XBc49nytKj46Gz3AMylIJS+47H6yjh5C/vAuLkid+ayboq0W9+VJyMI2wgQ?=
 =?us-ascii?Q?/2XnQBp5eDYvTL5tbuSZ6Or4w214LYzXYY1pngdgAgWyaJ7xqQI9Ya73LVQu?=
 =?us-ascii?Q?po5t7x9nFCtXWEk/pLdPSKhPRg1vFai2xF8A7HMLNJJraJv6iqcHbCHi7k2h?=
 =?us-ascii?Q?J78pVgHZhKw/3CVHaH+UurNp9ijm0vrK6nmTR1gZazTuiamAjGJSrDfFPFmt?=
 =?us-ascii?Q?xu+QdXLn6/csDnFmPrmrm3vJ2JcKVKyF1OP2bP24qHQcd6X47k0yjV/+pn11?=
 =?us-ascii?Q?Wmzbyr0STjaLZSWE7zftHQl9p5LuFgHBXSwtVfCCW/DyJP0rXdblDJTorf2o?=
 =?us-ascii?Q?yWqb7Ucs/TJz4Injinh+M3FpQ9wwo3nWBU97Z0R+ElV2d6NUvN69Cf248AQH?=
 =?us-ascii?Q?0w/wkJYyJbtSnFlR+55RNKRbSo/SmgsdQ/GFpz4MbMplndbNhrGzKVKRskQ3?=
 =?us-ascii?Q?EHZA9J5HgCvQmizTI7DfJHPROfjXxhROTzlzmDyCRRsPbgY2sFOGe6EOK1QO?=
 =?us-ascii?Q?6thfntgbvwEBJtgSjqItSES2RpPkJeYPCCs+wC6V9/MXHyLC/0s9RwA9DLTG?=
 =?us-ascii?Q?9zrZE2+qcx+xfVjHlw/cXAp4T+ihmv6gMpa7W59YaqeKuh0SDoPBEeRDugh4?=
 =?us-ascii?Q?XPxfSebmPUWx4KfvUhZX5ZCmwW3XTgBmX5g+n4uz6LqDe0pm3jhTepMT6Jyk?=
 =?us-ascii?Q?w+HqmFyZerh31+VSwf7OO8nKRz7gPfUP3iMA87yJ5IPDFezaXQaHuzUluFZl?=
 =?us-ascii?Q?kY2msUNh0K/gu+m6Uhmy16Dxb6XZom+Yrm+Ilk3N0JSLXPdBjiAGrI0N9Z1K?=
 =?us-ascii?Q?u4ShXgsfhaokV5UJoKxSAROQR0JqZJV15wkFSJ7bYrRFJUV/DCaAKwK7YkHa?=
 =?us-ascii?Q?O3igyVuHXdKSQkcIEuWk6dOsdR7iVeIjk/Lh+ne1FLhFDG2VV/ITzljJu8rZ?=
 =?us-ascii?Q?4pr8XhjMqzJqP6p2kPoMx873jDETKanEgxWBBLDc6wm5eAtankkqh/scc1iU?=
 =?us-ascii?Q?JxZ06Mi140eN96aYlj0d+rp2dQw3UzNJA5ewmkYbIhsCnVzAtaanggdYOgw3?=
 =?us-ascii?Q?j6nemMmTtO7iNQZs68hnrYkpwfk3vPmG8RG5dNeMS1Q2aStOuP2AehQ6S03I?=
 =?us-ascii?Q?rZZwgIuEMO5/zH9VVOF/fy7+2Sg5vgudw9q7zQsOPkNaU2ceyuNCwyxL/wlj?=
 =?us-ascii?Q?KXiFNj1/AWotdyHUP6x2nWbEkYJszGl5Pn6Xx7AwvE7oC5Z/mc7cjzKcqSNK?=
 =?us-ascii?Q?7C7sE7tLhp/W0ubgLrjLdivLa+DUUeJ6Gwr1/lIvuG60GVmZvRfnh3RfnXsu?=
 =?us-ascii?Q?4QUqZhg8kMEwSBu0Ld9YQut3cgv38i9zux7EwAik4OYez2HUq9Y3LpWmP+Lp?=
 =?us-ascii?Q?YwAmqWlcT+NQvUPIdFSRxTw=3D?=
X-OriginatorOrg: os.amperecomputing.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c2e64e73-d11c-4142-bc82-08db65943f06
X-MS-Exchange-CrossTenant-AuthSource: DM5PR0102MB3590.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Jun 2023 07:12:39.1981
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3bc2b170-fd94-476d-b0ce-4229bdc904a7
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: /BAwNSLvb+8t9lPTItnwAc666ZYQaMvhWG8JKLRHCT7qlYRTnR3WWrMNA7acQHnJr5Mpko3bl6BYKr7M0urMf8z5ELvdE9LHUysVdGv+IBMeGas2iNzg5ayAUAIVr97W
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR01MB7292
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,
        SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


Hi Robin,

I have a couple of comments below

On Thu, 1 Jun 2023, Robin Murphy wrote:
> The functional paths of the driver need not care about ACPI, so abstract
> the property of atomic doubleword access as its own flag (repacking the
> structure for a better fit). We also do not need to go poking directly
> at the APMT for standard resources which the ACPI layer has already
> dealt with, so deal with the optional MMIO page and interrupt in the
> normal firmware-agnostic manner. The few remaining portions of probing
> that *are* APMT-specific can still easily retrieve the APMT pointer as
> needed without us having to carry a duplicate copy around everywhere.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> drivers/perf/arm_cspmu/arm_cspmu.c | 45 ++++++++----------------------
> drivers/perf/arm_cspmu/arm_cspmu.h |  4 +--
> 2 files changed, 13 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index 3b91115c376d..f8daf252a488 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c

...

> @@ -319,7 +309,7 @@ static const char *arm_cspmu_get_name(const struct arm_cspmu *cspmu)
> 	static atomic_t pmu_idx[ACPI_APMT_NODE_TYPE_COUNT] = { 0 };
>
> 	dev = cspmu->dev;
> -	apmt_node = cspmu->apmt_node;
> +	apmt_node = dev_get_platdata(dev);

Was platdata changed too? If not, I think this should be

 	apmt_node = *(struct acpi_apmt_node **) dev_get_platdata(dev);

> 	pmu_type = apmt_node->type;
>
> 	if (pmu_type >= ACPI_APMT_NODE_TYPE_COUNT) {
> @@ -396,8 +386,8 @@ static const struct impl_match impl_match[] = {
> static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
> {
> 	int ret;
> -	struct acpi_apmt_node *apmt_node = cspmu->apmt_node;
> 	struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops;
> +	struct acpi_apmt_node *apmt_node = dev_get_platdata(cspmu->dev);

Ditto

> 	const struct impl_match *match = impl_match;
>
> 	/*

...

> @@ -910,24 +900,18 @@ static struct arm_cspmu *arm_cspmu_alloc(struct platform_device *pdev)
> {
> 	struct acpi_apmt_node *apmt_node;
> 	struct arm_cspmu *cspmu;
> -	struct device *dev;
> -
> -	dev = &pdev->dev;
> -	apmt_node = *(struct acpi_apmt_node **)dev_get_platdata(dev);
> -	if (!apmt_node) {
> -		dev_err(dev, "failed to get APMT node\n");
> -		return NULL;
> -	}
> +	struct device *dev = &pdev->dev;
>
> 	cspmu = devm_kzalloc(dev, sizeof(*cspmu), GFP_KERNEL);
> 	if (!cspmu)
> 		return NULL;
>
> 	cspmu->dev = dev;
> -	cspmu->apmt_node = apmt_node;
> -
> 	platform_set_drvdata(pdev, cspmu);
>
> +	apmt_node = dev_get_platdata(dev);

Ditto

> +	cspmu->has_atomic_dword = apmt_node->flags & ACPI_APMT_FLAGS_ATOMIC;
> +
> 	return cspmu;
> }

...

> @@ -1047,19 +1029,14 @@ static int arm_cspmu_request_irq(struct arm_cspmu *cspmu)
> 	int irq, ret;
> 	struct device *dev;
> 	struct platform_device *pdev;
> -	struct acpi_apmt_node *apmt_node;
>
> 	dev = cspmu->dev;
> 	pdev = to_platform_device(dev);
> -	apmt_node = cspmu->apmt_node;
>
> 	/* Skip IRQ request if the PMU does not support overflow interrupt. */
> -	if (apmt_node->ovflw_irq == 0)
> -		return 0;
> -
> -	irq = platform_get_irq(pdev, 0);
> +	irq = platform_get_irq_optional(pdev, 0);
> 	if (irq < 0)
> -		return irq;
> +		return irq == -ENXIO ? 0 : irq;
>
> 	ret = devm_request_irq(dev, irq, arm_cspmu_handle_irq,
> 			       IRQF_NOBALANCING | IRQF_NO_THREAD, dev_name(dev),
> @@ -1109,7 +1086,7 @@ static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu)
> 	int cpu;
>
> 	dev = cspmu->pmu.dev;

You didn't touch this one but shouldn't this be

 	dev = cspmu->dev;

> -	apmt_node = cspmu->apmt_node;
> +	apmt_node = dev_get_platdata(dev);

Ditto

> 	affinity_flag = apmt_node->flags & ACPI_APMT_FLAGS_AFFINITY;


Otherwise the patch looks good to me.

Cheers, Ilkka