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

[Peng Fan <peng.fan@xxxxxxx>]

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@xxxxxxx>
> >
> 
> 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