From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C1045C369C2 for ; Tue, 22 Apr 2025 12:39:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=6fgesda1FWG50Vjp07EaonJY4BgVwPXoSNedSB2Hh2Q=; b=3UseLHWaTsS6BLQSObwt0PwILD uHlnA5kk+UBZnjWFzxpprslF1brsllcHpcIPqxek7bFThsQFlN2BZwuCo/E4sDeHjC4bGjiJfQZDx I3ppk4ZpDCjPztD8VAyhwh1qmQXpX0p6Qc4XrK8ImjkpefV4jk/OoY8t+OYCVn2NDzswO2+xjH52R ybexxoU2gdawoSdgtNrx+rlxdI0TNPoLBXqeqJt8GwMVfb1A2JDiW8Ft5LS9odQf9+PdoApHAdaCn Ixzt9YquLaKD7DeqeOQjg+m1R0UexNRj9HhvaFRSZUefn/Jqf4fi+vo0cmgvwOgldRDIxVrW1wB2q RrjW3mew==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1u7Cu1-000000079N7-2S17; Tue, 22 Apr 2025 12:39:05 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1u7Bsv-00000006xVZ-1IIB for linux-arm-kernel@lists.infradead.org; Tue, 22 Apr 2025 11:33:55 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 0B0C8152B; Tue, 22 Apr 2025 04:33:46 -0700 (PDT) Received: from pluto (usa-sjc-mx-foss1.foss.arm.com [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E7D1B3F5A1; Tue, 22 Apr 2025 04:33:47 -0700 (PDT) Date: Tue, 22 Apr 2025 12:33:40 +0100 From: Cristian Marussi To: Johan Hovold Cc: Sudeep Holla , Cristian Marussi , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, arm-scmi@vger.kernel.org, james.quinlan@broadcom.com, f.fainelli@gmail.com, vincent.guittot@linaro.org, peng.fan@oss.nxp.com, michal.simek@amd.com, quic_sibis@quicinc.com, dan.carpenter@linaro.org, maz@kernel.org Subject: Re: [PATCH 2/4] firmware: arm_scmi: Add Quirks framework Message-ID: References: <20250415142933.1746249-1-cristian.marussi@arm.com> <20250415142933.1746249-3-cristian.marussi@arm.com> <20250417-teal-sidewinder-of-courtesy-d0473d@sudeepholla> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20250422_043353_434446_7E464ACB X-CRM114-Status: GOOD ( 46.86 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Apr 22, 2025 at 12:41:47PM +0200, Johan Hovold wrote: > On Thu, Apr 17, 2025 at 03:41:56PM +0100, Sudeep Holla wrote: > > On Thu, Apr 17, 2025 at 12:10:25PM +0100, Cristian Marussi wrote: > > > On Thu, Apr 17, 2025 at 10:44:24AM +0200, Johan Hovold wrote: > > > > On Wed, Apr 16, 2025 at 05:37:13PM +0100, Cristian Marussi wrote: > > > > > On Wed, Apr 16, 2025 at 06:00:37PM +0200, Johan Hovold wrote: > > > > > > On Tue, Apr 15, 2025 at 03:29:31PM +0100, Cristian Marussi wrote: > > > > > > > > > > > +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. > > Hi, a quick one that I am away from the keyboard currently ... > > Agreed, but we can predict that. You can infer just from the current state > > of affairs. Today all machines based on soc X may need the quirk but the > > firmware may vary across machines with same SoC. > > Sure, I was just highlighting this limitation in the current > implementation... > > > > > I was referring to the need to match on other compatible strings than > > > > the most specific one. For the ThinkPad T14s the strings are: > > > > > > > > "lenovo,thinkpad-t14s-lcd", "lenovo,thinkpad-t14s", > > > > "qcom,x1e78100", "qcom,x1e80100" > > > > > > > > Here you most certainly would not want to match on > > > > "lenovo,thinkpad-t14s-lcd" but rather on "lenovo,thinkpad-t14s" or one > > > > of the SoC compatibles. > > ...and the fact that even if you want to avoid matching on SoC > compatible, the current implementation seems to be too limited to allow > matching on machine compatibles generally (i.e. given that there may be > variants of a particular machine). > > We may not even need this for the FC quirk, this was just a general > observation. > > > > > of_machine_is_compatible() can be used to match on any compatible > > > > string, but not sure if that fits with your current implementation. > > > > > > > > I was thinking about the same when I looked at the code. Using it is > > more elegant IMO. > > It would be more flexible, even if you never intend to accept any quirks > that matches for an entire SoC. > I already have a V2 under test that will define a quirk using a __VA_ARGS__ so that you can specify a variable number of compats to match against the platform running using of_machine_compatible_match() +/* + * Define a quirk by name (_qn) and provide the matching tokens where: + * + * _ven : SCMI Vendor ID string, NULL means any. + * _sub : SCMI SubVendor ID string, NULL means any. + * _impl : SCMI Implementation Version string, NULL means any. + * This version string can express ranges using the following + * syntax: + * + * NULL [0, 0xFFFFFFFF] + * "X" [X, X] + * "X-" [X, 0xFFFFFFFF] + * "-X" [0, X] + * "X-Y" [X, Y] + * + * where in [MIN, MAX] means: + * + * MIN <= <= MAX && MIN <= MAX + * + * _comps : NULL terminated array of compatible strings. + * An empty array means any. + * + * This implicitly define also a properly named global static-key that + * will be used to dynamically enable the quirk at initialization time. + * + * Note that it is possible to associate multiple quirks to the same + * matching pattern, if your firmware quality is really astounding :P + */ +#define DEFINE_SCMI_QUIRK(_qn, _ven, _sub, _impl, ...) \ I will post V2 soon. > > > ...moreover this kind of carpet-quirking that hides the issue on any possible > > > fw version gives ZERO incentives to the aforementioned vendor to fix its > > > firmware...(or it fw-release process)... > > If a vendor truly only cares about some other OS then perhaps this > argument isn't that strong and we'll just increase our own maintenance > burden. > indeed. > > > Indeed, cross posting from your other mail thread, as of now we cannot > > > even be sure if the Vendor has somehow already updated the SCMI-related > > > firmware NEITHER we can be sure about this in the future since it has not > > > even confirmed how they are (or they will) be handling the Impl_Version field... > > > > > > Having said that, I would add that in this specific case (FC quirk) since the > > > only way to make use of all of this SCMI stuff from the MicroZoft/ACPI world > > > is having working FCs and, since it's clear that our lovely vendor wont > > > certainly break this other lovely OS way of working with SCMI, MAYBE it could > > > be safe to just apply the quirk to this Vendor forever no matter what the > > > platform or FW version will be in the future...(so not using compats at all) > > My understanding is that the version has been bumped now albeit for > other reasons than fixing this particular bug. And since enabling FC for > these messages should be safe we will probably be able to match on > vendor/impl_version here. So what is your latest Firmware version read ? no more 0x20000 [ 0.116746] arm-scmi arm-scmi.0.auto: SCMI Protocol ?? 'Qualcom:' Firmware version ??????? > > Sibi is looking into this and should be able to provide an answer > shortly. > Good. Thanks, Cristian