| Message ID | 20240221220426.1205146-1-cristian.marussi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | firmware: arm_scmi: Add support for multiple vendors custom protocols | expand |
Hi Cristian, > Subject: [PATCH] firmware: arm_scmi: Add support for multiple vendors > custom protocols Do you have an example how to test this? Could this be used for pinctrl imx? Thanks, Peng. > > Add a mechanism to be able to tag vendor protocol modules at compile-time > with a vendor/sub_vendor string and an implementation version and then to > choose to load, at run-time, only those vendor protocol modules matching as > close as possible the vendor/subvendor identification advertised by the SCMI > platform server. > > In this way, any in-tree existent vendor protocol module can be build and > shipped by default in a single kernel image, even when using the same > clashing protocol identification numbers, since the SCMI core will take care at > run-time to load only the ones pertinent to the running system. > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > --- > Hi all, > > this is meant to address the possibility of having multiple vendors > implementing distinct SCMI vendor protocols with possibly the same > overlapping protocol number without the need of crafting the Kconfig at > compile time to include only wanted protos (and without the need for a > lottery :P) > > Basic idea is simple: > > - vendor protos HAS to be 'tagged' at build time with a vendor_id > - vendor protos COULD also optionally be tagged at build time with > sub_vendor_id and implemetation version > > - at init all the build vendor protos are registerd with the SCMI core > using a key derived from the above tags > > - at SCMI probe time the platform is identified via Base protocol as > usual and all the required vendor protos (i.e. IDs defined in the DT > as usual) are loaded after a lookup process based on the following rules: > > + protocol DB is searched using the platform/Base runtime provided tags > > <vendor> / <sub_vendor> / <impl_ver> > > using the the following search logic (keys), first match: > > 1. proto_id / <vendor_id> / <sub_vendor_id> / <impl_ver> > > 2. proto_id / <vendor_id> / <sub_vendor_id> / 0 > > 3. proto_id / <vendor_id> / NULL / 0 > > IOW, closest match, depending on how much fine grained is your > protocol tuned (tagged) for the platform. > > I am doubtful about the usage of <impl_ver>, and tempted to drop it > since we have anyway protocol version and > NEGOTIATE_PROTOCOL_VERSION > to deal with slight difference in fw revision... > > Based on sudeep/for-linux-next on top of > > 1c2c88cfcb2b ("clk: scmi: Support get/set duty_cycle operations") > > Minimally tested .... > > Any feedback welcome > > Thanks, > Cristian > --- > drivers/firmware/arm_scmi/driver.c | 166 ++++++++++++++++++++++---- > drivers/firmware/arm_scmi/protocols.h | 5 + > 2 files changed, 149 insertions(+), 22 deletions(-) > > diff --git a/drivers/firmware/arm_scmi/driver.c > b/drivers/firmware/arm_scmi/driver.c > index 34d77802c990..8fb2903698c9 100644 > --- a/drivers/firmware/arm_scmi/driver.c > +++ b/drivers/firmware/arm_scmi/driver.c > @@ -33,6 +33,7 @@ > #include <linux/processor.h> > #include <linux/refcount.h> > #include <linux/slab.h> > +#include <linux/xarray.h> > > #include "common.h" > #include "notify.h" > @@ -44,8 +45,7 @@ > > static DEFINE_IDA(scmi_id); > > -static DEFINE_IDR(scmi_protocols); > -static DEFINE_SPINLOCK(protocol_lock); > +static DEFINE_XARRAY(scmi_protocols); > > /* List of all SCMI devices active in system */ static LIST_HEAD(scmi_list); > @@ -194,11 +194,90 @@ struct scmi_info { > #define bus_nb_to_scmi_info(nb) container_of(nb, struct scmi_info, > bus_nb) > #define req_nb_to_scmi_info(nb) container_of(nb, struct scmi_info, > dev_req_nb) > > -static const struct scmi_protocol *scmi_protocol_get(int protocol_id) > +static unsigned long > +scmi_vendor_protocol_signature(unsigned protocol_id, char *vendor_id, > + char *sub_vendor_id, u32 impl_ver) > { > - const struct scmi_protocol *proto; > + char *signature, *p; > + unsigned long hash = 0; > > - proto = idr_find(&scmi_protocols, protocol_id); > + /* vendor_id/sub_vendor_id guaranteed <= > SCMI_SHORT_NAME_MAX_SIZE */ > + signature = kasprintf(GFP_KERNEL, "%02X|%s|%s|0x%08X", > protocol_id, > + vendor_id ?: "", sub_vendor_id ?: "", impl_ver); > + if (!signature) > + return 0; > + > + p = signature; > + while (*p) > + hash = partial_name_hash(tolower(*p++), hash); > + hash = end_name_hash(hash); > + > + kfree(signature); > + > + return hash; > +} > + > +static unsigned long > +scmi_protocol_key_calculate(int protocol_id, char *vendor_id, > + char *sub_vendor_id, u32 impl_ver) { > + if (protocol_id < SCMI_PROTOCOL_VENDOR) > + return protocol_id; > + else > + return scmi_vendor_protocol_signature(protocol_id, > vendor_id, > + sub_vendor_id, > impl_ver); > +} > + > +static const struct scmi_protocol * > +scmi_vendor_protocol_lookup(int protocol_id, char *vendor_id, > + char *sub_vendor_id, u32 impl_ver) { > + unsigned long key; > + struct scmi_protocol *proto = NULL; > + > + /* Searching for closest match ...*/ > + key = scmi_protocol_key_calculate(protocol_id, vendor_id, > + sub_vendor_id, impl_ver); > + if (key) > + proto = xa_load(&scmi_protocols, key); > + > + if (proto) > + return proto; > + > + /* Any match on vendor/sub_vendor ? */ > + if (impl_ver) { > + key = scmi_protocol_key_calculate(protocol_id, vendor_id, > + sub_vendor_id, 0); > + if (key) > + proto = xa_load(&scmi_protocols, key); > + > + if (proto) > + return proto; > + } > + > + /* Any match on just the vendor ? */ > + if (sub_vendor_id) { > + key = scmi_protocol_key_calculate(protocol_id, vendor_id, > + NULL, 0); > + if (key) > + proto = xa_load(&scmi_protocols, key); > + } > + > + return proto; > +} > + > +static const struct scmi_protocol * > +scmi_protocol_get(int protocol_id, struct scmi_revision_info *version) > +{ > + const struct scmi_protocol *proto = NULL; > + > + if (protocol_id < SCMI_PROTOCOL_VENDOR) > + proto = xa_load(&scmi_protocols, protocol_id); > + else > + proto = scmi_vendor_protocol_lookup(protocol_id, > + version->vendor_id, > + version->sub_vendor_id, > + version->impl_ver); > if (!proto || !try_module_get(proto->owner)) { > pr_warn("SCMI Protocol 0x%x not found!\n", protocol_id); > return NULL; > @@ -206,21 +285,47 @@ static const struct scmi_protocol > *scmi_protocol_get(int protocol_id) > > pr_debug("Found SCMI Protocol 0x%x\n", protocol_id); > > + if (protocol_id >= SCMI_PROTOCOL_VENDOR) > + pr_info("Loaded SCMI Vendor Protocol 0x%x - %s %s %X\n", > + protocol_id, proto->vendor_id ?: "", > + proto->sub_vendor_id ?: "", proto->impl_ver); > + > return proto; > } > > -static void scmi_protocol_put(int protocol_id) > +static void scmi_protocol_put(const struct scmi_protocol *proto) > { > - const struct scmi_protocol *proto; > - > - proto = idr_find(&scmi_protocols, protocol_id); > if (proto) > module_put(proto->owner); > } > > +static int scmi_vendor_protocol_check(const struct scmi_protocol > +*proto) { > + if (!proto->vendor_id) { > + pr_err("missing vendor_id for protocol 0x%x\n", proto->id); > + return -EINVAL; > + } > + > + if (proto->vendor_id && > + strlen(proto->vendor_id) >= SCMI_SHORT_NAME_MAX_SIZE) { > + pr_err("malformed vendor_id for protocol 0x%x\n", proto- > >id); > + return -EINVAL; > + } > + > + if (proto->sub_vendor_id && > + strlen(proto->sub_vendor_id) >= SCMI_SHORT_NAME_MAX_SIZE) { > + pr_err("malformed sub_vendor_id for protocol 0x%x\n", > + proto->id); > + return -EINVAL; > + } > + > + return 0; > +} > + > int scmi_protocol_register(const struct scmi_protocol *proto) { > int ret; > + unsigned long key; > > if (!proto) { > pr_err("invalid protocol\n"); > @@ -232,12 +337,23 @@ int scmi_protocol_register(const struct > scmi_protocol *proto) > return -EINVAL; > } > > - spin_lock(&protocol_lock); > - ret = idr_alloc(&scmi_protocols, (void *)proto, > - proto->id, proto->id + 1, GFP_ATOMIC); > - spin_unlock(&protocol_lock); > - if (ret != proto->id) { > - pr_err("unable to allocate SCMI idr slot for 0x%x - err %d\n", > + if (proto->id >= SCMI_PROTOCOL_VENDOR && > + scmi_vendor_protocol_check(proto)) > + return -EINVAL; > + > + /* > + * Calculate a protocol key to register this protocol with the core; > + * key value 0 is considered invalid. > + */ > + key = scmi_protocol_key_calculate(proto->id, proto->vendor_id, > + proto->sub_vendor_id, > + proto->impl_ver); > + if (!key) > + return -EINVAL; > + > + ret = xa_insert(&scmi_protocols, key, (void *)proto, GFP_KERNEL); > + if (ret) { > + pr_err("unable to allocate SCMI protocol slot for 0x%x - > err %d\n", > proto->id, ret); > return ret; > } > @@ -250,9 +366,15 @@ EXPORT_SYMBOL_GPL(scmi_protocol_register); > > void scmi_protocol_unregister(const struct scmi_protocol *proto) { > - spin_lock(&protocol_lock); > - idr_remove(&scmi_protocols, proto->id); > - spin_unlock(&protocol_lock); > + unsigned long key; > + > + key = scmi_protocol_key_calculate(proto->id, proto->vendor_id, > + proto->sub_vendor_id, > + proto->impl_ver); > + if (!key) > + return; > + > + xa_erase(&scmi_protocols, key); > > pr_debug("Unregistered SCMI Protocol 0x%x\n", proto->id); } @@ - > 1888,7 +2010,7 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info, > /* Protocol specific devres group */ > gid = devres_open_group(handle->dev, NULL, GFP_KERNEL); > if (!gid) { > - scmi_protocol_put(proto->id); > + scmi_protocol_put(pi->proto); > goto out; > } > > @@ -1952,7 +2074,7 @@ scmi_alloc_init_protocol_instance(struct scmi_info > *info, > > clean: > /* Take care to put the protocol module's owner before releasing all > */ > - scmi_protocol_put(proto->id); > + scmi_protocol_put(proto); > devres_release_group(handle->dev, gid); > out: > return ERR_PTR(ret); > @@ -1986,7 +2108,7 @@ scmi_get_protocol_instance(const struct > scmi_handle *handle, u8 protocol_id) > const struct scmi_protocol *proto; > > /* Fails if protocol not registered on bus */ > - proto = scmi_protocol_get(protocol_id); > + proto = scmi_protocol_get(protocol_id, &info->version); > if (proto) > pi = scmi_alloc_init_protocol_instance(info, proto); > else > @@ -2041,7 +2163,7 @@ void scmi_protocol_release(const struct > scmi_handle *handle, u8 protocol_id) > > idr_remove(&info->protocols, protocol_id); > > - scmi_protocol_put(protocol_id); > + scmi_protocol_put(pi->proto); > > devres_release_group(handle->dev, gid); > dev_dbg(handle->dev, "De-Initialized protocol: 0x%X\n", diff > --git a/drivers/firmware/arm_scmi/protocols.h > b/drivers/firmware/arm_scmi/protocols.h > index 693019fff0f6..cb8dbc93584b 100644 > --- a/drivers/firmware/arm_scmi/protocols.h > +++ b/drivers/firmware/arm_scmi/protocols.h > @@ -29,6 +29,8 @@ > #define PROTOCOL_REV_MAJOR(x) > ((u16)(FIELD_GET(PROTOCOL_REV_MAJOR_MASK, (x)))) > #define PROTOCOL_REV_MINOR(x) > ((u16)(FIELD_GET(PROTOCOL_REV_MINOR_MASK, (x)))) > > +#define SCMI_PROTOCOL_VENDOR 0x80 > + > enum scmi_common_cmd { > PROTOCOL_VERSION = 0x0, > PROTOCOL_ATTRIBUTES = 0x1, > @@ -330,6 +332,9 @@ struct scmi_protocol { > const void *ops; > const struct scmi_protocol_events *events; > unsigned int supported_version; > + u32 impl_ver; > + char *vendor_id; > + char *sub_vendor_id; > }; > > #define DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(name, proto) \ > -- > 2.43.0
On Thu, Feb 22, 2024 at 08:32:05AM +0000, Peng Fan (OSS) wrote: > Hi Cristian, > > > Subject: [PATCH] firmware: arm_scmi: Add support for multiple vendors > > custom protocols > > Do you have an example how to test this? Could this be used for pinctrl imx? Hi Peng, as an example usecase for this, you can consider your upcoming series which adds 2 NXP SCMI vendor protocol (which I hope to review today) that you numbered as 0x81, 0x84: in order to avoid clashes with other vendors potentially registering in the future their own vendor protocols while using the same numbers as yours, you will have to add a vendor_id string specification to your scmi_protocol definition, so that it matches the vendor_id string reported by your SCMI server at runtime via the Base protocol. This mechanism basically allows a vendor to own the whole space of vendor protocols numbers, since the numbers are really now referred to a specific vendor namespace at runtime. You can also optionally add a sub_vendor_id and implementation version if you want to use different protocols of you own with the same number but different capabilties on different platform of yours...but I am doubtful about the opprtunity of this further customization...lets see what the review process bring up... BUT, all of this stands true for custom vendor protocols, pinctrl-imx does use the standard 0x19 Pinctrl protocol, and the clash is really on the device node underlying the device which is created...re-implementing and duplicating fully a std protocol as a vendor one, just with a different number to workaround this issue it is not something can go upstream... ...so at this point you will be better off with just that couple of downstream pinctrl-imx patches that you have, until we get creative and found a better way to support your specific usecase...or you move to the generic Linux Pinctrl subsystem that is supported by the pinctrl-scmi driver originally by Oleksii (but I understand this is problematic for you in other ways too...) Thanks, Cristian
On 2/22/24 03:34, Cristian Marussi wrote: > Add a mechanism to be able to tag vendor protocol modules at compile-time > with a vendor/sub_vendor string and an implementation version and then to > choose to load, at run-time, only those vendor protocol modules matching > as close as possible the vendor/subvendor identification advertised by > the SCMI platform server. > > In this way, any in-tree existent vendor protocol module can be build and > shipped by default in a single kernel image, even when using the same > clashing protocol identification numbers, since the SCMI core will take > care at run-time to load only the ones pertinent to the running system. > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> Cristian, Thanks for the patch. > --- > Hi all, > > this is meant to address the possibility of having multiple vendors > implementing distinct SCMI vendor protocols with possibly the same > overlapping protocol number without the need of crafting the Kconfig > at compile time to include only wanted protos > (and without the need for a lottery :P) > > Basic idea is simple: > > - vendor protos HAS to be 'tagged' at build time with a vendor_id > - vendor protos COULD also optionally be tagged at build time with > sub_vendor_id and implemetation version > > - at init all the build vendor protos are registerd with the SCMI core > using a key derived from the above tags > > - at SCMI probe time the platform is identified via Base protocol as > usual and all the required vendor protos (i.e. IDs defined in the DT > as usual) are loaded after a lookup process based on the following rules: > > + protocol DB is searched using the platform/Base runtime provided tags > > <vendor> / <sub_vendor> / <impl_ver> > > using the the following search logic (keys), first match: > > 1. proto_id / <vendor_id> / <sub_vendor_id> / <impl_ver> > > 2. proto_id / <vendor_id> / <sub_vendor_id> / 0 > > 3. proto_id / <vendor_id> / NULL / 0 > > IOW, closest match, depending on how much fine grained is your > protocol tuned (tagged) for the platform. > > I am doubtful about the usage of <impl_ver>, and tempted to drop it > since we have anyway protocol version and NEGOTIATE_PROTOCOL_VERSION > to deal with slight difference in fw revision... > > Based on sudeep/for-linux-next on top of > > 1c2c88cfcb2b ("clk: scmi: Support get/set duty_cycle operations") > > Minimally tested .... > > Any feedback welcome > > Thanks, > Cristian > --- > drivers/firmware/arm_scmi/driver.c | 166 ++++++++++++++++++++++---- > drivers/firmware/arm_scmi/protocols.h | 5 + > 2 files changed, 149 insertions(+), 22 deletions(-) > > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c > index 34d77802c990..8fb2903698c9 100644 > --- a/drivers/firmware/arm_scmi/driver.c > +++ b/drivers/firmware/arm_scmi/driver.c > @@ -33,6 +33,7 @@ > #include <linux/processor.h> > #include <linux/refcount.h> > #include <linux/slab.h> > +#include <linux/xarray.h> > > #include "common.h" > #include "notify.h" > @@ -44,8 +45,7 @@ > > static DEFINE_IDA(scmi_id); > > -static DEFINE_IDR(scmi_protocols); > -static DEFINE_SPINLOCK(protocol_lock); > +static DEFINE_XARRAY(scmi_protocols); > > /* List of all SCMI devices active in system */ > static LIST_HEAD(scmi_list); > @@ -194,11 +194,90 @@ struct scmi_info { > #define bus_nb_to_scmi_info(nb) container_of(nb, struct scmi_info, bus_nb) > #define req_nb_to_scmi_info(nb) container_of(nb, struct scmi_info, dev_req_nb) > > -static const struct scmi_protocol *scmi_protocol_get(int protocol_id) > +static unsigned long > +scmi_vendor_protocol_signature(unsigned protocol_id, char *vendor_id, > + char *sub_vendor_id, u32 impl_ver) > { > - const struct scmi_protocol *proto; > + char *signature, *p; > + unsigned long hash = 0; > > - proto = idr_find(&scmi_protocols, protocol_id); > + /* vendor_id/sub_vendor_id guaranteed <= SCMI_SHORT_NAME_MAX_SIZE */ > + signature = kasprintf(GFP_KERNEL, "%02X|%s|%s|0x%08X", protocol_id, > + vendor_id ?: "", sub_vendor_id ?: "", impl_ver); > + if (!signature) > + return 0; > + > + p = signature; > + while (*p) > + hash = partial_name_hash(tolower(*p++), hash); > + hash = end_name_hash(hash); > + > + kfree(signature); > + > + return hash; > +} > + > +static unsigned long > +scmi_protocol_key_calculate(int protocol_id, char *vendor_id, > + char *sub_vendor_id, u32 impl_ver) > +{ > + if (protocol_id < SCMI_PROTOCOL_VENDOR) > + return protocol_id; > + else > + return scmi_vendor_protocol_signature(protocol_id, vendor_id, > + sub_vendor_id, impl_ver); > +} > + > +static const struct scmi_protocol * > +scmi_vendor_protocol_lookup(int protocol_id, char *vendor_id, > + char *sub_vendor_id, u32 impl_ver) > +{ > + unsigned long key; > + struct scmi_protocol *proto = NULL; > + > + /* Searching for closest match ...*/ > + key = scmi_protocol_key_calculate(protocol_id, vendor_id, > + sub_vendor_id, impl_ver); > + if (key) > + proto = xa_load(&scmi_protocols, key); > + > + if (proto) > + return proto; > + > + /* Any match on vendor/sub_vendor ? */ > + if (impl_ver) { > + key = scmi_protocol_key_calculate(protocol_id, vendor_id, > + sub_vendor_id, 0); > + if (key) > + proto = xa_load(&scmi_protocols, key); > + > + if (proto) > + return proto; > + } > + > + /* Any match on just the vendor ? */ > + if (sub_vendor_id) { > + key = scmi_protocol_key_calculate(protocol_id, vendor_id, > + NULL, 0); > + if (key) > + proto = xa_load(&scmi_protocols, key); > + } > + > + return proto; > +} > + > +static const struct scmi_protocol * > +scmi_protocol_get(int protocol_id, struct scmi_revision_info *version) > +{ > + const struct scmi_protocol *proto = NULL; > + > + if (protocol_id < SCMI_PROTOCOL_VENDOR) > + proto = xa_load(&scmi_protocols, protocol_id); > + else > + proto = scmi_vendor_protocol_lookup(protocol_id, > + version->vendor_id, > + version->sub_vendor_id, > + version->impl_ver); > if (!proto || !try_module_get(proto->owner)) { > pr_warn("SCMI Protocol 0x%x not found!\n", protocol_id); > return NULL; > @@ -206,21 +285,47 @@ static const struct scmi_protocol *scmi_protocol_get(int protocol_id) > > pr_debug("Found SCMI Protocol 0x%x\n", protocol_id); > > + if (protocol_id >= SCMI_PROTOCOL_VENDOR) > + pr_info("Loaded SCMI Vendor Protocol 0x%x - %s %s %X\n", > + protocol_id, proto->vendor_id ?: "", > + proto->sub_vendor_id ?: "", proto->impl_ver); > + > return proto; > } > > -static void scmi_protocol_put(int protocol_id) > +static void scmi_protocol_put(const struct scmi_protocol *proto) > { > - const struct scmi_protocol *proto; > - > - proto = idr_find(&scmi_protocols, protocol_id); > if (proto) > module_put(proto->owner); > } > > +static int scmi_vendor_protocol_check(const struct scmi_protocol *proto) > +{ > + if (!proto->vendor_id) { > + pr_err("missing vendor_id for protocol 0x%x\n", proto->id); > + return -EINVAL; > + } > + > + if (proto->vendor_id && You can drop ^^, since you've already checked for it. Reviewed-and-Tested-by: Sibi Sankar <quic_sibis@quicinc.com> > + strlen(proto->vendor_id) >= SCMI_SHORT_NAME_MAX_SIZE) { > + pr_err("malformed vendor_id for protocol 0x%x\n", proto->id); > + return -EINVAL; > + } > + > + if (proto->sub_vendor_id && > + strlen(proto->sub_vendor_id) >= SCMI_SHORT_NAME_MAX_SIZE) { > + pr_err("malformed sub_vendor_id for protocol 0x%x\n", > + proto->id); > + return -EINVAL; > + } > + > + return 0; > +} > + > int scmi_protocol_register(const struct scmi_protocol *proto) > { > int ret; > + unsigned long key; > > if (!proto) { > pr_err("invalid protocol\n"); > @@ -232,12 +337,23 @@ int scmi_protocol_register(const struct scmi_protocol *proto) > return -EINVAL; > } > > - spin_lock(&protocol_lock); > - ret = idr_alloc(&scmi_protocols, (void *)proto, > - proto->id, proto->id + 1, GFP_ATOMIC); > - spin_unlock(&protocol_lock); > - if (ret != proto->id) { > - pr_err("unable to allocate SCMI idr slot for 0x%x - err %d\n", > + if (proto->id >= SCMI_PROTOCOL_VENDOR && > + scmi_vendor_protocol_check(proto)) > + return -EINVAL; > + > + /* > + * Calculate a protocol key to register this protocol with the core; > + * key value 0 is considered invalid. > + */ > + key = scmi_protocol_key_calculate(proto->id, proto->vendor_id, > + proto->sub_vendor_id, > + proto->impl_ver); > + if (!key) > + return -EINVAL; > + > + ret = xa_insert(&scmi_protocols, key, (void *)proto, GFP_KERNEL); > + if (ret) { > + pr_err("unable to allocate SCMI protocol slot for 0x%x - err %d\n", > proto->id, ret); > return ret; > } > @@ -250,9 +366,15 @@ EXPORT_SYMBOL_GPL(scmi_protocol_register); > > void scmi_protocol_unregister(const struct scmi_protocol *proto) > { > - spin_lock(&protocol_lock); > - idr_remove(&scmi_protocols, proto->id); > - spin_unlock(&protocol_lock); > + unsigned long key; > + > + key = scmi_protocol_key_calculate(proto->id, proto->vendor_id, > + proto->sub_vendor_id, > + proto->impl_ver); > + if (!key) > + return; > + > + xa_erase(&scmi_protocols, key); > > pr_debug("Unregistered SCMI Protocol 0x%x\n", proto->id); > } > @@ -1888,7 +2010,7 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info, > /* Protocol specific devres group */ > gid = devres_open_group(handle->dev, NULL, GFP_KERNEL); > if (!gid) { > - scmi_protocol_put(proto->id); > + scmi_protocol_put(pi->proto); > goto out; > } > > @@ -1952,7 +2074,7 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info, > > clean: > /* Take care to put the protocol module's owner before releasing all */ > - scmi_protocol_put(proto->id); > + scmi_protocol_put(proto); > devres_release_group(handle->dev, gid); > out: > return ERR_PTR(ret); > @@ -1986,7 +2108,7 @@ scmi_get_protocol_instance(const struct scmi_handle *handle, u8 protocol_id) > const struct scmi_protocol *proto; > > /* Fails if protocol not registered on bus */ > - proto = scmi_protocol_get(protocol_id); > + proto = scmi_protocol_get(protocol_id, &info->version); > if (proto) > pi = scmi_alloc_init_protocol_instance(info, proto); > else > @@ -2041,7 +2163,7 @@ void scmi_protocol_release(const struct scmi_handle *handle, u8 protocol_id) > > idr_remove(&info->protocols, protocol_id); > > - scmi_protocol_put(protocol_id); > + scmi_protocol_put(pi->proto); > > devres_release_group(handle->dev, gid); > dev_dbg(handle->dev, "De-Initialized protocol: 0x%X\n", > diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h > index 693019fff0f6..cb8dbc93584b 100644 > --- a/drivers/firmware/arm_scmi/protocols.h > +++ b/drivers/firmware/arm_scmi/protocols.h > @@ -29,6 +29,8 @@ > #define PROTOCOL_REV_MAJOR(x) ((u16)(FIELD_GET(PROTOCOL_REV_MAJOR_MASK, (x)))) > #define PROTOCOL_REV_MINOR(x) ((u16)(FIELD_GET(PROTOCOL_REV_MINOR_MASK, (x)))) > > +#define SCMI_PROTOCOL_VENDOR 0x80 > + > enum scmi_common_cmd { > PROTOCOL_VERSION = 0x0, > PROTOCOL_ATTRIBUTES = 0x1, > @@ -330,6 +332,9 @@ struct scmi_protocol { > const void *ops; > const struct scmi_protocol_events *events; > unsigned int supported_version; > + u32 impl_ver; > + char *vendor_id; > + char *sub_vendor_id; > }; > > #define DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(name, proto) \
On Wed, Feb 28, 2024 at 10:36:43PM +0530, Sibi Sankar wrote: > > > On 2/22/24 03:34, Cristian Marussi wrote: > > Add a mechanism to be able to tag vendor protocol modules at compile-time > > with a vendor/sub_vendor string and an implementation version and then to > > choose to load, at run-time, only those vendor protocol modules matching > > as close as possible the vendor/subvendor identification advertised by > > the SCMI platform server. > > > > In this way, any in-tree existent vendor protocol module can be build and > > shipped by default in a single kernel image, even when using the same > > clashing protocol identification numbers, since the SCMI core will take > > care at run-time to load only the ones pertinent to the running system. > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > > Cristian, > > Thanks for the patch. > > > > --- > > Hi all, > > > > this is meant to address the possibility of having multiple vendors > > implementing distinct SCMI vendor protocols with possibly the same > > overlapping protocol number without the need of crafting the Kconfig > > at compile time to include only wanted protos > > (and without the need for a lottery :P) > > > > Basic idea is simple: > > > > - vendor protos HAS to be 'tagged' at build time with a vendor_id > > - vendor protos COULD also optionally be tagged at build time with > > sub_vendor_id and implemetation version > > > > - at init all the build vendor protos are registerd with the SCMI core > > using a key derived from the above tags > > > > - at SCMI probe time the platform is identified via Base protocol as > > usual and all the required vendor protos (i.e. IDs defined in the DT > > as usual) are loaded after a lookup process based on the following rules: > > > > + protocol DB is searched using the platform/Base runtime provided tags > > > > <vendor> / <sub_vendor> / <impl_ver> > > > > using the the following search logic (keys), first match: > > > > 1. proto_id / <vendor_id> / <sub_vendor_id> / <impl_ver> > > > > 2. proto_id / <vendor_id> / <sub_vendor_id> / 0 > > > > 3. proto_id / <vendor_id> / NULL / 0 > > > > IOW, closest match, depending on how much fine grained is your > > protocol tuned (tagged) for the platform. > > > > I am doubtful about the usage of <impl_ver>, and tempted to drop it > > since we have anyway protocol version and NEGOTIATE_PROTOCOL_VERSION > > to deal with slight difference in fw revision... > > > > Based on sudeep/for-linux-next on top of > > > > 1c2c88cfcb2b ("clk: scmi: Support get/set duty_cycle operations") > > > > Minimally tested .... > > > > Any feedback welcome > > > > Thanks, > > Cristian > > --- > > drivers/firmware/arm_scmi/driver.c | 166 ++++++++++++++++++++++---- > > drivers/firmware/arm_scmi/protocols.h | 5 + > > 2 files changed, 149 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c > > index 34d77802c990..8fb2903698c9 100644 > > --- a/drivers/firmware/arm_scmi/driver.c > > +++ b/drivers/firmware/arm_scmi/driver.c > > @@ -33,6 +33,7 @@ > > #include <linux/processor.h> > > #include <linux/refcount.h> > > #include <linux/slab.h> > > +#include <linux/xarray.h> > > #include "common.h" > > #include "notify.h" > > @@ -44,8 +45,7 @@ > > static DEFINE_IDA(scmi_id); > > -static DEFINE_IDR(scmi_protocols); > > -static DEFINE_SPINLOCK(protocol_lock); > > +static DEFINE_XARRAY(scmi_protocols); > > /* List of all SCMI devices active in system */ > > static LIST_HEAD(scmi_list); > > @@ -194,11 +194,90 @@ struct scmi_info { > > #define bus_nb_to_scmi_info(nb) container_of(nb, struct scmi_info, bus_nb) > > #define req_nb_to_scmi_info(nb) container_of(nb, struct scmi_info, dev_req_nb) > > -static const struct scmi_protocol *scmi_protocol_get(int protocol_id) > > +static unsigned long > > +scmi_vendor_protocol_signature(unsigned protocol_id, char *vendor_id, > > + char *sub_vendor_id, u32 impl_ver) > > { > > - const struct scmi_protocol *proto; > > + char *signature, *p; > > + unsigned long hash = 0; > > - proto = idr_find(&scmi_protocols, protocol_id); > > + /* vendor_id/sub_vendor_id guaranteed <= SCMI_SHORT_NAME_MAX_SIZE */ > > + signature = kasprintf(GFP_KERNEL, "%02X|%s|%s|0x%08X", protocol_id, > > + vendor_id ?: "", sub_vendor_id ?: "", impl_ver); > > + if (!signature) > > + return 0; > > + > > + p = signature; > > + while (*p) > > + hash = partial_name_hash(tolower(*p++), hash); > > + hash = end_name_hash(hash); > > + > > + kfree(signature); > > + > > + return hash; > > +} > > + > > +static unsigned long > > +scmi_protocol_key_calculate(int protocol_id, char *vendor_id, > > + char *sub_vendor_id, u32 impl_ver) > > +{ > > + if (protocol_id < SCMI_PROTOCOL_VENDOR) > > + return protocol_id; > > + else > > + return scmi_vendor_protocol_signature(protocol_id, vendor_id, > > + sub_vendor_id, impl_ver); > > +} > > + > > +static const struct scmi_protocol * > > +scmi_vendor_protocol_lookup(int protocol_id, char *vendor_id, > > + char *sub_vendor_id, u32 impl_ver) > > +{ > > + unsigned long key; > > + struct scmi_protocol *proto = NULL; > > + > > + /* Searching for closest match ...*/ > > + key = scmi_protocol_key_calculate(protocol_id, vendor_id, > > + sub_vendor_id, impl_ver); > > + if (key) > > + proto = xa_load(&scmi_protocols, key); > > + > > + if (proto) > > + return proto; > > + > > + /* Any match on vendor/sub_vendor ? */ > > + if (impl_ver) { > > + key = scmi_protocol_key_calculate(protocol_id, vendor_id, > > + sub_vendor_id, 0); > > + if (key) > > + proto = xa_load(&scmi_protocols, key); > > + > > + if (proto) > > + return proto; > > + } > > + > > + /* Any match on just the vendor ? */ > > + if (sub_vendor_id) { > > + key = scmi_protocol_key_calculate(protocol_id, vendor_id, > > + NULL, 0); > > + if (key) > > + proto = xa_load(&scmi_protocols, key); > > + } > > + > > + return proto; > > +} > > + > > +static const struct scmi_protocol * > > +scmi_protocol_get(int protocol_id, struct scmi_revision_info *version) > > +{ > > + const struct scmi_protocol *proto = NULL; > > + > > + if (protocol_id < SCMI_PROTOCOL_VENDOR) > > + proto = xa_load(&scmi_protocols, protocol_id); > > + else > > + proto = scmi_vendor_protocol_lookup(protocol_id, > > + version->vendor_id, > > + version->sub_vendor_id, > > + version->impl_ver); > > if (!proto || !try_module_get(proto->owner)) { > > pr_warn("SCMI Protocol 0x%x not found!\n", protocol_id); > > return NULL; > > @@ -206,21 +285,47 @@ static const struct scmi_protocol *scmi_protocol_get(int protocol_id) > > pr_debug("Found SCMI Protocol 0x%x\n", protocol_id); > > + if (protocol_id >= SCMI_PROTOCOL_VENDOR) > > + pr_info("Loaded SCMI Vendor Protocol 0x%x - %s %s %X\n", > > + protocol_id, proto->vendor_id ?: "", > > + proto->sub_vendor_id ?: "", proto->impl_ver); > > + > > return proto; > > } > > -static void scmi_protocol_put(int protocol_id) > > +static void scmi_protocol_put(const struct scmi_protocol *proto) > > { > > - const struct scmi_protocol *proto; > > - > > - proto = idr_find(&scmi_protocols, protocol_id); > > if (proto) > > module_put(proto->owner); > > } > > +static int scmi_vendor_protocol_check(const struct scmi_protocol *proto) > > +{ > > + if (!proto->vendor_id) { > > + pr_err("missing vendor_id for protocol 0x%x\n", proto->id); > > + return -EINVAL; > > + } > > + > > + if (proto->vendor_id && > > You can drop ^^, since you've already checked for it. > Ah, right it's just up there :P > Reviewed-and-Tested-by: Sibi Sankar <quic_sibis@quicinc.com> > Thanks for the review, I will wait for more possible feedback and reviews before reposting a V2 with this fix. Thanks, Cristian
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index 34d77802c990..8fb2903698c9 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -33,6 +33,7 @@ #include <linux/processor.h> #include <linux/refcount.h> #include <linux/slab.h> +#include <linux/xarray.h> #include "common.h" #include "notify.h" @@ -44,8 +45,7 @@ static DEFINE_IDA(scmi_id); -static DEFINE_IDR(scmi_protocols); -static DEFINE_SPINLOCK(protocol_lock); +static DEFINE_XARRAY(scmi_protocols); /* List of all SCMI devices active in system */ static LIST_HEAD(scmi_list); @@ -194,11 +194,90 @@ struct scmi_info { #define bus_nb_to_scmi_info(nb) container_of(nb, struct scmi_info, bus_nb) #define req_nb_to_scmi_info(nb) container_of(nb, struct scmi_info, dev_req_nb) -static const struct scmi_protocol *scmi_protocol_get(int protocol_id) +static unsigned long +scmi_vendor_protocol_signature(unsigned protocol_id, char *vendor_id, + char *sub_vendor_id, u32 impl_ver) { - const struct scmi_protocol *proto; + char *signature, *p; + unsigned long hash = 0; - proto = idr_find(&scmi_protocols, protocol_id); + /* vendor_id/sub_vendor_id guaranteed <= SCMI_SHORT_NAME_MAX_SIZE */ + signature = kasprintf(GFP_KERNEL, "%02X|%s|%s|0x%08X", protocol_id, + vendor_id ?: "", sub_vendor_id ?: "", impl_ver); + if (!signature) + return 0; + + p = signature; + while (*p) + hash = partial_name_hash(tolower(*p++), hash); + hash = end_name_hash(hash); + + kfree(signature); + + return hash; +} + +static unsigned long +scmi_protocol_key_calculate(int protocol_id, char *vendor_id, + char *sub_vendor_id, u32 impl_ver) +{ + if (protocol_id < SCMI_PROTOCOL_VENDOR) + return protocol_id; + else + return scmi_vendor_protocol_signature(protocol_id, vendor_id, + sub_vendor_id, impl_ver); +} + +static const struct scmi_protocol * +scmi_vendor_protocol_lookup(int protocol_id, char *vendor_id, + char *sub_vendor_id, u32 impl_ver) +{ + unsigned long key; + struct scmi_protocol *proto = NULL; + + /* Searching for closest match ...*/ + key = scmi_protocol_key_calculate(protocol_id, vendor_id, + sub_vendor_id, impl_ver); + if (key) + proto = xa_load(&scmi_protocols, key); + + if (proto) + return proto; + + /* Any match on vendor/sub_vendor ? */ + if (impl_ver) { + key = scmi_protocol_key_calculate(protocol_id, vendor_id, + sub_vendor_id, 0); + if (key) + proto = xa_load(&scmi_protocols, key); + + if (proto) + return proto; + } + + /* Any match on just the vendor ? */ + if (sub_vendor_id) { + key = scmi_protocol_key_calculate(protocol_id, vendor_id, + NULL, 0); + if (key) + proto = xa_load(&scmi_protocols, key); + } + + return proto; +} + +static const struct scmi_protocol * +scmi_protocol_get(int protocol_id, struct scmi_revision_info *version) +{ + const struct scmi_protocol *proto = NULL; + + if (protocol_id < SCMI_PROTOCOL_VENDOR) + proto = xa_load(&scmi_protocols, protocol_id); + else + proto = scmi_vendor_protocol_lookup(protocol_id, + version->vendor_id, + version->sub_vendor_id, + version->impl_ver); if (!proto || !try_module_get(proto->owner)) { pr_warn("SCMI Protocol 0x%x not found!\n", protocol_id); return NULL; @@ -206,21 +285,47 @@ static const struct scmi_protocol *scmi_protocol_get(int protocol_id) pr_debug("Found SCMI Protocol 0x%x\n", protocol_id); + if (protocol_id >= SCMI_PROTOCOL_VENDOR) + pr_info("Loaded SCMI Vendor Protocol 0x%x - %s %s %X\n", + protocol_id, proto->vendor_id ?: "", + proto->sub_vendor_id ?: "", proto->impl_ver); + return proto; } -static void scmi_protocol_put(int protocol_id) +static void scmi_protocol_put(const struct scmi_protocol *proto) { - const struct scmi_protocol *proto; - - proto = idr_find(&scmi_protocols, protocol_id); if (proto) module_put(proto->owner); } +static int scmi_vendor_protocol_check(const struct scmi_protocol *proto) +{ + if (!proto->vendor_id) { + pr_err("missing vendor_id for protocol 0x%x\n", proto->id); + return -EINVAL; + } + + if (proto->vendor_id && + strlen(proto->vendor_id) >= SCMI_SHORT_NAME_MAX_SIZE) { + pr_err("malformed vendor_id for protocol 0x%x\n", proto->id); + return -EINVAL; + } + + if (proto->sub_vendor_id && + strlen(proto->sub_vendor_id) >= SCMI_SHORT_NAME_MAX_SIZE) { + pr_err("malformed sub_vendor_id for protocol 0x%x\n", + proto->id); + return -EINVAL; + } + + return 0; +} + int scmi_protocol_register(const struct scmi_protocol *proto) { int ret; + unsigned long key; if (!proto) { pr_err("invalid protocol\n"); @@ -232,12 +337,23 @@ int scmi_protocol_register(const struct scmi_protocol *proto) return -EINVAL; } - spin_lock(&protocol_lock); - ret = idr_alloc(&scmi_protocols, (void *)proto, - proto->id, proto->id + 1, GFP_ATOMIC); - spin_unlock(&protocol_lock); - if (ret != proto->id) { - pr_err("unable to allocate SCMI idr slot for 0x%x - err %d\n", + if (proto->id >= SCMI_PROTOCOL_VENDOR && + scmi_vendor_protocol_check(proto)) + return -EINVAL; + + /* + * Calculate a protocol key to register this protocol with the core; + * key value 0 is considered invalid. + */ + key = scmi_protocol_key_calculate(proto->id, proto->vendor_id, + proto->sub_vendor_id, + proto->impl_ver); + if (!key) + return -EINVAL; + + ret = xa_insert(&scmi_protocols, key, (void *)proto, GFP_KERNEL); + if (ret) { + pr_err("unable to allocate SCMI protocol slot for 0x%x - err %d\n", proto->id, ret); return ret; } @@ -250,9 +366,15 @@ EXPORT_SYMBOL_GPL(scmi_protocol_register); void scmi_protocol_unregister(const struct scmi_protocol *proto) { - spin_lock(&protocol_lock); - idr_remove(&scmi_protocols, proto->id); - spin_unlock(&protocol_lock); + unsigned long key; + + key = scmi_protocol_key_calculate(proto->id, proto->vendor_id, + proto->sub_vendor_id, + proto->impl_ver); + if (!key) + return; + + xa_erase(&scmi_protocols, key); pr_debug("Unregistered SCMI Protocol 0x%x\n", proto->id); } @@ -1888,7 +2010,7 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info, /* Protocol specific devres group */ gid = devres_open_group(handle->dev, NULL, GFP_KERNEL); if (!gid) { - scmi_protocol_put(proto->id); + scmi_protocol_put(pi->proto); goto out; } @@ -1952,7 +2074,7 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info, clean: /* Take care to put the protocol module's owner before releasing all */ - scmi_protocol_put(proto->id); + scmi_protocol_put(proto); devres_release_group(handle->dev, gid); out: return ERR_PTR(ret); @@ -1986,7 +2108,7 @@ scmi_get_protocol_instance(const struct scmi_handle *handle, u8 protocol_id) const struct scmi_protocol *proto; /* Fails if protocol not registered on bus */ - proto = scmi_protocol_get(protocol_id); + proto = scmi_protocol_get(protocol_id, &info->version); if (proto) pi = scmi_alloc_init_protocol_instance(info, proto); else @@ -2041,7 +2163,7 @@ void scmi_protocol_release(const struct scmi_handle *handle, u8 protocol_id) idr_remove(&info->protocols, protocol_id); - scmi_protocol_put(protocol_id); + scmi_protocol_put(pi->proto); devres_release_group(handle->dev, gid); dev_dbg(handle->dev, "De-Initialized protocol: 0x%X\n", diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h index 693019fff0f6..cb8dbc93584b 100644 --- a/drivers/firmware/arm_scmi/protocols.h +++ b/drivers/firmware/arm_scmi/protocols.h @@ -29,6 +29,8 @@ #define PROTOCOL_REV_MAJOR(x) ((u16)(FIELD_GET(PROTOCOL_REV_MAJOR_MASK, (x)))) #define PROTOCOL_REV_MINOR(x) ((u16)(FIELD_GET(PROTOCOL_REV_MINOR_MASK, (x)))) +#define SCMI_PROTOCOL_VENDOR 0x80 + enum scmi_common_cmd { PROTOCOL_VERSION = 0x0, PROTOCOL_ATTRIBUTES = 0x1, @@ -330,6 +332,9 @@ struct scmi_protocol { const void *ops; const struct scmi_protocol_events *events; unsigned int supported_version; + u32 impl_ver; + char *vendor_id; + char *sub_vendor_id; }; #define DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(name, proto) \
Add a mechanism to be able to tag vendor protocol modules at compile-time with a vendor/sub_vendor string and an implementation version and then to choose to load, at run-time, only those vendor protocol modules matching as close as possible the vendor/subvendor identification advertised by the SCMI platform server. In this way, any in-tree existent vendor protocol module can be build and shipped by default in a single kernel image, even when using the same clashing protocol identification numbers, since the SCMI core will take care at run-time to load only the ones pertinent to the running system. Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> --- Hi all, this is meant to address the possibility of having multiple vendors implementing distinct SCMI vendor protocols with possibly the same overlapping protocol number without the need of crafting the Kconfig at compile time to include only wanted protos (and without the need for a lottery :P) Basic idea is simple: - vendor protos HAS to be 'tagged' at build time with a vendor_id - vendor protos COULD also optionally be tagged at build time with sub_vendor_id and implemetation version - at init all the build vendor protos are registerd with the SCMI core using a key derived from the above tags - at SCMI probe time the platform is identified via Base protocol as usual and all the required vendor protos (i.e. IDs defined in the DT as usual) are loaded after a lookup process based on the following rules: + protocol DB is searched using the platform/Base runtime provided tags <vendor> / <sub_vendor> / <impl_ver> using the the following search logic (keys), first match: 1. proto_id / <vendor_id> / <sub_vendor_id> / <impl_ver> 2. proto_id / <vendor_id> / <sub_vendor_id> / 0 3. proto_id / <vendor_id> / NULL / 0 IOW, closest match, depending on how much fine grained is your protocol tuned (tagged) for the platform. I am doubtful about the usage of <impl_ver>, and tempted to drop it since we have anyway protocol version and NEGOTIATE_PROTOCOL_VERSION to deal with slight difference in fw revision... Based on sudeep/for-linux-next on top of 1c2c88cfcb2b ("clk: scmi: Support get/set duty_cycle operations") Minimally tested .... Any feedback welcome Thanks, Cristian --- drivers/firmware/arm_scmi/driver.c | 166 ++++++++++++++++++++++---- drivers/firmware/arm_scmi/protocols.h | 5 + 2 files changed, 149 insertions(+), 22 deletions(-)