Re: [RFC PATCH 3/3] [NOT FOR UPSTREAM] firmware: arm_scmi: quirk: Ignore FC bit in attributes

[Johan Hovold <johan@kernel.org>]

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