RE: [PATCH v2 2/4] firmware: arm_scmi: Add machine_allowlist and machine_blocklist

Peng Fan
2025-02-10T13:21:24Z

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