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

[Cristian Marussi <cristian.marussi@arm.com>]

On Thu, Apr 03, 2025 at 12:05:30PM +0200, Johan Hovold wrote:
> 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? :)

yes...but not attached to a message_id == 0x5 :P 

> 
> > > > +			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)?
> 

I'll think about it.

> 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.
> 

Done for V1

> > > >  /* 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.
> 

Ok.

> > ...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?
> 

Right good point...we cannot assume anything really...quirks can be
needed on a single version or on a range...we need ranges...

I will rework the logic for V1.

Thanks of the review and the feedback
Cristian