linux-kernel - Re: [PATCH 2/4] firmware: arm

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: <Z__UJUKaMRoFLYLc@hovoldconsulting.com>
Date: Wed, 16 Apr 2025 18:00:37 +0200
From: Johan Hovold <johan@...nel.org>
To: Cristian Marussi <cristian.marussi@....com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	arm-scmi@...r.kernel.org, sudeep.holla@....com,
	james.quinlan@...adcom.com, f.fainelli@...il.com,
	vincent.guittot@...aro.org, peng.fan@....nxp.com,
	michal.simek@....com, quic_sibis@...cinc.com,
	dan.carpenter@...aro.org, maz@...nel.org
Subject: Re: [PATCH 2/4] firmware: arm_scmi: Add Quirks framework

On Tue, Apr 15, 2025 at 03:29:31PM +0100, Cristian Marussi wrote:
> Add a common framework to describe SCMI quirks and associate them with a
> specific platform or a specific set of SCMI firmware versions.
> 
> All the matching SCMI quirks will be enabled when the SCMI core stack
> probes and after all the needed SCMI firmware versioning information was
> retrieved using Base protocol.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@....com>
> ---
> RFC->V1
> - added handling of impl_ver ranges in quirk definition
> - make Quirks Framework default-y
> - match on all NULL/0 too..these are quirks that apply always anywhere !
> - depends on COMPILE_TEST too
> - move quirk enable calling logic out of Base protocol init (triggers a
>   LOCKDEP issues around cpu locks (cpufreq, static_branch_enable interactions..)

> +static void scmi_enable_matching_quirks(struct scmi_info *info)
> +{
> +	struct scmi_revision_info *rev = &info->version;
> +	const char *compatible = NULL;
> +	struct device_node *root;
> +
> +	root = of_find_node_by_path("/");
> +	if (root) {
> +		of_property_read_string(root, "compatible", &compatible);

Looks like you still only allow matching on the most specific compatible
string.

As we discussed in the RFC thread, this will result in one quirk entry
for each machine in a SoC family in case the issue is not machine
specific.

> +		of_node_put(root);
> +	}
> +
> +	/* Enable applicable quirks */
> +	scmi_quirks_enable(info->dev, compatible,
> +			   rev->vendor_id, rev->sub_vendor_id, rev->impl_ver);
> +}

> +static int scmi_quirk_range_parse(struct scmi_quirk *quirk)
> +{
> +	const char *last, *first = quirk->impl_ver_range;
> +	size_t len;
> +	char *sep;
> +	int ret;
> +
> +	quirk->start_range = 0;
> +	quirk->end_range = 0xFFFFFFFF;
> +	len = quirk->impl_ver_range ? strlen(quirk->impl_ver_range) : 0;
> +	if (!len)
> +		return 0;
> +
> +	last = first + len - 1;
> +	sep = strchr(quirk->impl_ver_range, '-');
> +	if (sep)
> +		*sep = '\0';
> +
> +	if (sep == first) // -X
> +		ret = kstrtouint(first + 1, 0, &quirk->end_range);
> +	else // X OR X- OR X-y
> +		ret = kstrtouint(first, 0, &quirk->start_range);
> +	if (ret)
> +		return ret;
> +
> +	if (!sep)
> +		quirk->end_range = quirk->start_range;
> +	else if (sep != last) //x-Y

nit: space after // (if you insist on using c99 comments)

> +		ret = kstrtouint(sep + 1, 0, &quirk->end_range);
> +
> +	if (quirk->start_range > quirk->end_range)
> +		return -EINVAL;
> +
> +	return ret;
> +}

> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * System Control and Management Interface (SCMI) Message Protocol Quirks
> + *
> + * Copyright (C) 2025 ARM Ltd.
> + */
> +#ifndef _SCMI_QUIRKS_H
> +#define _SCMI_QUIRKS_H
> +
> +#include <linux/static_key.h>
> +#include <linux/types.h>
> +
> +#ifdef CONFIG_ARM_SCMI_QUIRKS
> +
> +#define DECLARE_SCMI_QUIRK(_qn)						\
> +	DECLARE_STATIC_KEY_FALSE(scmi_quirk_ ## _qn)
> +
> +#define SCMI_QUIRK(_qn, _blk)						\
> +	do {								\
> +		if (static_branch_unlikely(&(scmi_quirk_ ## _qn)))	\
> +			(_blk);						\
> +	} while (0)
> +
> +void scmi_quirks_initialize(void);
> +void scmi_quirks_enable(struct device *dev, const char *compat,
> +			const char *vend, const char *subv, const u32 impl);
> +
> +#else
> +
> +#define DECLARE_SCMI_QUIRK(_qn)
> +#define SCMI_QUIRK(_qn, _blk)

As I mentioned earlier, wouldn't it be useful to always compile the
quirk code in order to make sure changes to surrounding code does not
break builds where the quirks are enabled?

For example, by adding something like

#define SCMI_QUIRK(_qn, _blk)						\
	do {								\
		if (0)							\
			(_blk);						\
	} while (0)

here?

I didn't really review this in detail, but it seems to work as intended
when matching on vendor and version:

Tested-by: Johan Hovold <johan+linaro@...nel.org>