linux-kernel - Re: [RFC PATCH 3/3] [NOT FOR UPSTREAM] 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-5daoJn22XTprwk@hovoldconsulting.com>
Date: Thu, 3 Apr 2025 12:05:30 +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: [RFC PATCH 3/3] [NOT FOR UPSTREAM] firmware: arm_scmi: quirk:
 Ignore FC bit in attributes

On Thu, Apr 03, 2025 at 10:08:41AM +0100, Cristian Marussi wrote:
> On Thu, Apr 03, 2025 at 10:25:21AM +0200, Johan Hovold wrote:
> > On Tue, Apr 01, 2025 at 01:25:45PM +0100, Cristian Marussi wrote:
 
> > > +#define QUIRK_PERF_FC_FORCE						\
> > > +	({								\
> > > +		if (pi->proto->id == SCMI_PROTOCOL_PERF ||		\
> > > +		    message_id == 0x5 /* PERF_LEVEL_GET */)		\
> > 
> > This should be logical AND and PERF_LEVEL_GET is 0x8 (currently
> > fastchannel is enabled for all PERF messages).
> 
> ...right...not sure how I botched this condition completely...my bad...
> (even the comment is wrong :P...)

The PERF_LEVEL_GET comment? That one is correct, right? :)

> > > +			attributes |= BIT(0);				\
> > > +	})
> > > +
> > >  static void
> > >  scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph,
> > >  			     u8 describe_id, u32 message_id, u32 valid_size,
> > > @@ -1924,6 +1931,7 @@ scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph,
> > >  
> > >  	/* Check if the MSG_ID supports fastchannel */
> > >  	ret = scmi_protocol_msg_check(ph, message_id, &attributes);
> > > +	SCMI_QUIRK(perf_level_get_fc_force, QUIRK_PERF_FC_FORCE);
> > 
> > This is cool and I assume can be used to minimise overhead in hot paths.
> > Perhaps you can have concerns about readability and remembering to
> > update the quirk implementation if the code here changes.
> 
> My main aim here was to be able to define the quirk code as much as
> possible in the proximity of where it is used...so that is clear what it
> does and dont get lost in some general common table....and the macro was
> a way to uniform the treatment of the static keys...
> 
> ...but I am still not sure if all of these macros just degrade visibility
> and we could get rid of them...would be really cool to somehow break the
> build if the code "sorrounding" the SCMI_QUIRK changes and you dont update
> (somehow) the quirk too...so as to be sure that the quirk is taking care of
> and maintained...but I doubt that is feasible, because, really, how do you
> even deternine which code changes are in proximity enough to the quirk to
> justify a break...same block ? same functions ? you cannot really know
> semantically where some changes can impact this part of the code...
> ..I supppose reviews and testing is the key and the only possible answer
> to this..

Yeah, it goes both ways. Getting the quirk implementation out of the way
makes it easier to follow the normal flow, but also makes it a bit
harder to review the quirk. Your implementation may be a good trade-off.

> > Does it even get compile tested if SCMI_QUIRKS is disabled?
> 
> It evaluates to nothing when CONFIG_ARM_SCMI_QUIRKS is disabled...
> ...so maybe I could add a Kconfig dep on COMPILE_TEST ....if this is what
> you mean..

Perhaps there's some way to get the quirk code always compiled but
discarded when CONFIG_ARM_SCMI_QUIRKS is disabled (e.g. by using
IS_ENABLED() in the macro)?

CONFIG_ARM_SCMI_QUIRKS may also need to be enabled by default as it can
be hard to track down random crashes to a missing quirk.

> > >  /* Global Quirks Definitions */
> > > +DEFINE_SCMI_QUIRK(perf_level_get_fc_force,
> > > +		  "your-bad-compatible", NULL, NULL, 0x0);
> > 
> > At first I tried matching on the SoC (fallback) compatible without
> > success until I noticed you need to put the primary machine compatible
> > here. For the SoC at hand, that would mean adding 10 or so entries since
> > all current commercial devices would be affected by this.
> > 
> 
> Ah right...I tested on a number of combinations BUT assumed only one
> compatible was to be found...you can potentially add dozens of this
> definitions for a number of platforms associating the same quirk to all
> of them and let the match logic enabling only the proper on...BUT this
> clearly does NOT scale indeed and you will have to endlessly add new
> platform if fw does NOT get fixed ever...
> 
> > Matching on vendor and protocol works.
> > 
> 
> That is abosutely the preferred way, BUT the match should be on
> Vendor/SubVendor/ImplVersion ... if the platform properly uses
> ImplementationVersion to differentiate between firmware builds...

We don't seem to have a subvendor here and if IIUC the version has not
been bumped (yet) after fixing the FC issue.

> ...if not you will end up applying the quirk on ANY current and future
> FW from this Vendor...maybe not an issue in this case...BUT they should
> seriously thinking about using ImplementationVersion properly in their
> future FW releases...especially if, as of now, no new fixed FW release
> has ever been released...

Right, in this case it would probably be OK.

But what if the version is bumped for some other reason (e.g. before a
bug has been identified)? Then you'd currently need an entry for each
affected revision or does the implementation assume it applies to
anything <= ImplVersion? Do we want to add support for version ranges?

Johan