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:

quoted

From: Peng Fan <peng.fan@nxp.com>

Hi,

quoted

There are two cases:
pinctrl-scmi.c and pinctrl-imx-scmi.c, both use

SCMI_PROTOCOL_PINCTRL.

quoted

If both drivers are built in, and the scmi device with name "pinctrl-

imx"

quoted

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

quoted

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