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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
> + 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 <[email protected]>
>
> 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 <[email protected]>
>
Thanks for the review, I will wait for more possible feedback and
reviews before reposting a V2 with this fix.
Thanks,
Cristian