linux-kernel - Re: [PATCH v2 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: <aBC16CGDhZ_XO3Xr@hovoldconsulting.com>
Date: Tue, 29 Apr 2025 13:20:08 +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 v2 2/4] firmware: arm_scmi: Add Quirks framework

On Tue, Apr 29, 2025 at 11:47:54AM +0100, Cristian Marussi wrote:
> On Sun, Apr 27, 2025 at 04:38:48PM +0200, Johan Hovold wrote:
> > On Fri, Apr 25, 2025 at 01:52:48PM +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.

> > > +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);
> > > +		of_node_put(root);
> > > +	}
> > > +
> > > +	dev_dbg(info->dev, "Looking for quirks matching: %s/%s/%s/0x%08X\n",
> > > +		compatible, rev->vendor_id, rev->sub_vendor_id, rev->impl_ver);
> > 
> > You're now just looking up the most specific compatible string in order
> > to include it in this dev_dbg(). Since you're now matching on all
> > compatible strings, perhaps you can consider just dropping it.
> > 
> 
> Yes indeed I was not sure to keep all of this machinery just to print
> the machine compatible that is used to try to match against the
> (possible) list of compats....on the other side seemed useful to know
> exactly what you are trying to match against....but maybe we can simply
> assume that the machine compatible is well-known....

Perhaps it would have been more useful if you printed all the compatible
strings here and not just the most specific one, but yeah, there are
other ways to read this strings through sysfs.

Keeping as-is should be fine too.

> > > +		/*
> > > +		 * Note that there could be multiple matches so we
> > > +		 * will enable multiple quirk part of an hash collision
> > 
> > nit: "quirks that are part of a"?
> > 
> 
> mmm...as a non-native and poor English speaker I am, though, reasonably
> confident that a/an is chosen based on the vowel/consonant SOUND of the
> next word NOT on the effective letter...am I wrong ?
> (then we could digress about which is the sound of a[n] 'hash' :P ...)

That's my understanding as well, but 'hash' begins with a consonant
sound so I'm pretty sure it's "a hash".

> > > +		 * domain...BUT we cannot assume that ALL quirks on the
> > > +		 * same collision domain are a full match.
> > > +		 */
> > > +		hash_for_each_possible(scmi_quirks_ht, quirk, hash, hkey) {
> > > +			if (quirk->enabled || quirk->hkey != hkey ||
> > > +			    impl < quirk->start_range ||
> > > +			    impl > quirk->end_range)
> > > +				continue;
> > > +
> > > +			if (quirk->compats[0] &&
> > > +			    !of_machine_compatible_match(quirk->compats))
> > > +				continue;
> > > +
> > > +			dev_info(dev, "Enabling SCMI Quirk [%s]\n",
> > > +				 quirk->name);
> > > +
> > > +			dev_dbg(dev,
> > > +				"Quirk matched on: %s/%s/%s/[0x%08X-0x%08X]\n",
> > > +				quirk->compats[0], quirk->vendor,
> > 
> > You can now match on more than one compats string, but I guess printing
> > just the first one is fine.
> > 
> 
> Yes, same as above dev_dbg...not sure if it was meaningful really to
> dump all the list and overload the log with all such info...

Right, indicating there is some compatible string that's being used is
still useful to know.

> > > +				quirk->sub_vendor_id,
> > > +				quirk->start_range, quirk->end_range);
> > > +
> > > +			static_branch_enable(quirk->key);
> > > +			quirk->enabled = true;
> > > +		}
> > > +	}
> > > +}
> > 
> > This seems to work as intended and I've tried matching on machine and
> > SoC compatible strings and/or vendor and protocol version:
> > 
> > Tested-by: Johan Hovold <johan+linaro@...nel.org>
> > 
> 
> Thanks for the review Johan.
> 
> Since you have tested the effective final quirk patch, may I ask you to
> post straight away your final tested quirk patch on top of my next V3
> (with your authorship of course..)...I will drop the [NOT FOR UPSTREAM]
> example patch so that Sudeep can easily pick-up your patch.

Sure, I'll do so. Thanks for your help with getting this sorted!

Johan