| 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
| ||
|
Message-ID:
<PAXPR04MB8459A3750512708D2972687388F22@PAXPR04MB8459.eurprd04.prod.outlook.com>
Date: Mon, 10 Feb 2025 13:19:14 +0000
From: Peng Fan <peng.fan@....com>
To: Cristian Marussi <cristian.marussi@....com>, "Peng Fan (OSS)"
<peng.fan@....nxp.com>
CC: Sudeep Holla <sudeep.holla@....com>, Saravana Kannan
<saravanak@...gle.com>, Linus Walleij <linus.walleij@...aro.org>, Aisheng
Dong <aisheng.dong@....com>, Fabio Estevam <festevam@...il.com>, Shawn Guo
<shawnguo@...nel.org>, Jacky Bai <ping.bai@....com>, Pengutronix Kernel Team
<kernel@...gutronix.de>, Sascha Hauer <s.hauer@...gutronix.de>,
"arm-scmi@...r.kernel.org" <arm-scmi@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "linux-gpio@...r.kernel.org"
<linux-gpio@...r.kernel.org>, "imx@...ts.linux.dev" <imx@...ts.linux.dev>
Subject: RE: [PATCH v2 2/4] firmware: arm_scmi: Add machine_allowlist and
machine_blocklist
Hi Cristian, Sudeep,
> Subject: Re: [PATCH v2 2/4] firmware: arm_scmi: Add
> machine_allowlist and machine_blocklist
>
> On Mon, Jan 20, 2025 at 03:13:30PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@....com>
> >
>
> Hi,
>
> > There are two cases:
> > pinctrl-scmi.c and pinctrl-imx-scmi.c, both use
> SCMI_PROTOCOL_PINCTRL.
> > If both drivers are built in, and the scmi device with name "pinctrl-
> imx"
> > is created earlier, and the fwnode device points to the scmi device,
> > non-i.MX platforms will never have the pinctrl supplier ready.
> >
>
> The pinctrl-imx case is an unfortunate exception because you have a
> custom Vendor SCMI driver using a STANDARD protocol: this was never
> meant to be an allowed normal practice: the idea of SCMI Vendor
> extensions is to allow Vendors to write their own Vendor protocols
> (>0x80) and their own SCMI drivers on top of their custom vendor
> protocols.
>
> This list-based generalization seems to me dangerous because allows
> the spreading of such bad practice of loading custom Vendor drivers on
> top of standard protocols using the same protocol to do the same thing
> in a slightly different manner, with all the rfelated issues of
> fragmentation
>
> ...also I feel it could lead to an umaintainable mess of tables of
> compatibles....what happens if I write a 3rd pinctrl-cristian driver on
> top of it...both the new allowlist and the general pinctrl blocklist will
> need to be updated.
>
> The issue as we know is the interaction with devlink given that all of
> these same-protocol devices are created with the same device_node,
> since there is only one of them per-protocol in the DT....
>
> ...not sure what Sudeep thinks..just my opinion...
>
> > Vendor A use 0x80 for feature X, Vendor B use 0x80 for feature Y.
> > With both drivers built in, two scmi devices will be created, and both
> > drivers will be probed. On A's patform, feature Y probe may fail, vice
> > verus.
> >
>
> That's definitely an issue much worse than fw_devlink above....we
> indeed take care to pick the right vendor-protocol at runtime BUT no
> check is peformed against the SCMI drivers so you could end up picking
> up a completely unrelated protocol operations...damn...
>
> I think this is more esily solvable though....by adding a Vendor tag in
> the device_table (like the protocols do) you could skip device creation
> based on a mismatching Vendor, instead of using compatibles that are
> doomed to grow indefinitely as a list....
>
> So at this point you would have an optional Vendor and an optional
> flags (as mentioned in the other thread) in the device_table...
>
> I wonder if we can use the same logic for the above pinctrl case,
> without using compatibles ?
> I have not really thougth this through properly....
>
> In general, most of these issues are somehow Vendor-dependent, so I
> was also wondering if it was not the case to frame all of this in some
> sort of general vendor-quirks framework that could be used also when
> there are evident and NOT fixable issues on deployed FW SCMI server,
> so that we will have to flex a bit the kernel tolerance to cope with
> existing deployed HW that cannot be fixed fw-wise....
I just have a prototype and tested on i.MX95.
How do you think?
Extend scmi_device_id with flags, allowed_ids, blocked_ids.
The ids are SCMI vendor/subvendor, so need to use compatible
anymore.
/* The scmi device does not have fwnode handle */
#define SCMI_DEVICE_NO_FWNODE BIT(0)
struct scmi_device_vendor_id
{
const char *vendor_id;
const char *sub_vendor_id;
};
struct scmi_device_id {
u8 protocol_id;
const char *name;
u32 flags;
/* Optional */
struct scmi_device_vendor_id *blocked_ids;
struct scmi_device_vendor_id *allowed_ids;
};
In scmi_create_device, check block and allowed.
struct scmi_device *scmi_device_create(struct device_node *np,
- struct device *parent, int protocol,
+ struct device *parent,
+ struct scmi_revision_info *revision,
+ int protocol,
const char *name, u32 flags)
{
struct list_head *phead;
@@ -476,8 +494,16 @@ struct scmi_device *scmi_device_create(struct device_node *np,
/* Walk the list of requested devices for protocol and create them */
list_for_each_entry(rdev, phead, node) {
+ struct scmi_device_vendor_id *allowed_ids = rdev->id_table->allowed_ids;
+ struct scmi_device_vendor_id *blocked_ids = rdev->id_table->blocked_ids;
struct scmi_device *sdev;
+ if (blocked_ids && __scmi_device_vendor_id_match(revision, blocked_ids))
+ continue;
+
+ if (allowed_ids && !__scmi_device_vendor_id_match(revision, allowed_ids))
+ continue;
+
sdev = __scmi_device_create(np, parent,
rdev->id_table->protocol_id,
rdev->id_table->name,
In for cpufreq, use below to set device node.
+static int
+__scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
+ int protocol, const char *name, u32 flags)
+{
+ if (flags & SCMI_DEVICE_NO_FWNODE) {
+ scmi_dev->dev.of_node = np;
+ return 0;
+ }
+
+ device_set_node(&scmi_dev->dev, of_fwnode_handle(np));
+
+ return 0;
+}
Are these looks good? I could post the patchset later to see whether Sudeep
has any more comments on the current patchset or the new proposal.
BTW: I also pushed patch to
https://github.com/MrVan/linux.git branch: b4/scmi-fwdevlink-v2
and
Following Dan's suggestion to merge patch 2-4.
Thanks,
Peng.
>
> ...BUT I thought about this even less thoroughly :P...so it could be just a
> bad idea of mine...
>
> Thanks,
> Cristian
Powered by blists - more mailing lists