[PATCH 0/4] firmware: arm_scmi: Drop fake 'const' on scmi_handle

[PATCH 0/4] firmware: arm_scmi: Drop fake 'const' on scmi_handle

[Krzysztof Kozlowski]

Severale functions operating on the 'handle' pointer, like
scmi_handle_put() or scmi_xfer_raw_get(), are claiming it is a pointer
to const thus they should not modify the handle.  In fact that's a false
statement, because first thing these functions do is drop the cast to
const with container_of:

  struct scmi_info *info = handle_to_scmi_info(handle);

And with such cast the handle is easily writable with simple:

  info->handle.dev = NULL;

If the function really was not modifying the pointed handle, it would
use the container_of_const() call.

The code is not correct logically, either, because functions like
scmi_notification_instance_data_set() are meant to modify the data
behind the handle (in containing struct).

Best regards,
Krzysztof

---
Krzysztof Kozlowski (4):
      firmware: arm_scmi: Drop fake 'const' on scmi_handle
      firmware: arm_scmi: Drop fake 'const' on scmi_protocol_handle
      firmware: arm_scmi: Use container_of_const() on scmi_handle
      firmware: arm_scmi: Use container_of_const() on scmi_protocol_instance

 drivers/clk/clk-scmi.c                             |  2 +-
 drivers/firmware/arm_scmi/base.c                   |  2 +-
 drivers/firmware/arm_scmi/clock.c                  |  2 +-
 drivers/firmware/arm_scmi/common.h                 | 15 +++---
 drivers/firmware/arm_scmi/driver.c                 | 58 +++++++++++-----------
 drivers/firmware/arm_scmi/notify.c                 |  2 +-
 drivers/firmware/arm_scmi/perf.c                   |  2 +-
 drivers/firmware/arm_scmi/pinctrl.c                |  4 +-
 drivers/firmware/arm_scmi/power.c                  |  2 +-
 drivers/firmware/arm_scmi/powercap.c               |  2 +-
 drivers/firmware/arm_scmi/protocols.h              |  4 +-
 drivers/firmware/arm_scmi/raw_mode.c               |  4 +-
 drivers/firmware/arm_scmi/raw_mode.h               |  2 +-
 drivers/firmware/arm_scmi/reset.c                  |  2 +-
 drivers/firmware/arm_scmi/sensors.c                |  2 +-
 drivers/firmware/arm_scmi/system.c                 |  2 +-
 drivers/firmware/arm_scmi/vendors/imx/imx-sm-bbm.c |  2 +-
 drivers/firmware/arm_scmi/vendors/imx/imx-sm-cpu.c |  2 +-
 drivers/firmware/arm_scmi/vendors/imx/imx-sm-lmm.c |  2 +-
 .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c    |  2 +-
 drivers/firmware/arm_scmi/voltage.c                |  2 +-
 include/linux/scmi_protocol.h                      |  2 +-
 22 files changed, 60 insertions(+), 59 deletions(-)
---
base-commit: 5848db9e2caaa560a21ce692c4c32badef3c813f
change-id: 20260223-handle-not-const-9a6eb5529590

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

[PATCH 1/4] firmware: arm_scmi: Drop fake 'const' on scmi_handle

[Krzysztof Kozlowski]

Severale functions operating on the 'handle' pointer, like
scmi_handle_put() or scmi_xfer_raw_get(), are claiming it is a pointer
to const thus they should not modify the handle.  In fact that's a false
statement, because first thing these functions do is drop the cast to
const with container_of:

  struct scmi_info *info = handle_to_scmi_info(handle);

And with such cast the handle is easily writable with simple:

  info->handle.dev = NULL;

If the function really was not modifying the pointed handle, it would
use the container_of_const() call.

The code is not correct logically, either, because functions like
scmi_notification_instance_data_set() are meant to modify the data
behind the handle (in containing struct).

The code does not have actual visible bug, but incorrect 'const'
annotations could lead to incorrect compiler decisions.

Fixes: 3095a3e25d8f ("firmware: arm_scmi: Add xfer helpers to provide raw access")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
 drivers/clk/clk-scmi.c               |  2 +-
 drivers/firmware/arm_scmi/common.h   | 15 +++++++--------
 drivers/firmware/arm_scmi/driver.c   | 26 +++++++++++++-------------
 drivers/firmware/arm_scmi/notify.c   |  2 +-
 drivers/firmware/arm_scmi/raw_mode.c |  4 ++--
 drivers/firmware/arm_scmi/raw_mode.h |  2 +-
 include/linux/scmi_protocol.h        |  2 +-
 7 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index 6b286ea6f121..f9efe14a95ab 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -405,7 +405,7 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
 	struct clk_hw_onecell_data *clk_data;
 	struct device *dev = &sdev->dev;
 	struct device_node *np = dev->of_node;
-	const struct scmi_handle *handle = sdev->handle;
+	struct scmi_handle *handle = sdev->handle;
 	struct scmi_protocol_handle *ph;
 	const struct clk_ops *scmi_clk_ops_db[SCMI_MAX_CLK_OPS] = {};
 	struct scmi_clk *sclks;
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 7c35c95fddba..a18babacebf2 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -154,8 +154,8 @@ struct scmi_device *scmi_device_create(struct device_node *np,
 				       const char *name);
 void scmi_device_destroy(struct device *parent, int protocol, const char *name);
 
-int scmi_protocol_acquire(const struct scmi_handle *handle, u8 protocol_id);
-void scmi_protocol_release(const struct scmi_handle *handle, u8 protocol_id);
+int scmi_protocol_acquire(struct scmi_handle *handle, u8 protocol_id);
+void scmi_protocol_release(struct scmi_handle *handle, u8 protocol_id);
 
 /* SCMI Transport */
 /**
@@ -277,13 +277,12 @@ static inline bool is_polling_enabled(struct scmi_chan_info *cinfo,
 		is_transport_polling_capable(desc);
 }
 
-void scmi_xfer_raw_put(const struct scmi_handle *handle,
-		       struct scmi_xfer *xfer);
-struct scmi_xfer *scmi_xfer_raw_get(const struct scmi_handle *handle);
+void scmi_xfer_raw_put(struct scmi_handle *handle, struct scmi_xfer *xfer);
+struct scmi_xfer *scmi_xfer_raw_get(struct scmi_handle *handle);
 struct scmi_chan_info *
-scmi_xfer_raw_channel_get(const struct scmi_handle *handle, u8 protocol_id);
+scmi_xfer_raw_channel_get(struct scmi_handle *handle, u8 protocol_id);
 
-int scmi_xfer_raw_inflight_register(const struct scmi_handle *handle,
+int scmi_xfer_raw_inflight_register(struct scmi_handle *handle,
 				    struct scmi_xfer *xfer);
 
 int scmi_xfer_raw_wait_for_message_response(struct scmi_chan_info *cinfo,
@@ -522,7 +521,7 @@ static struct platform_driver __drv = {					       \
 	.probe = __tag##_probe,						       \
 }
 
-void scmi_notification_instance_data_set(const struct scmi_handle *handle,
+void scmi_notification_instance_data_set(struct scmi_handle *handle,
 					 void *priv);
 void *scmi_notification_instance_data_get(const struct scmi_handle *handle);
 int scmi_inflight_count(const struct scmi_handle *handle);
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 3e76a3204ba4..8b27e74d8a19 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -438,7 +438,7 @@ static void scmi_destroy_protocol_devices(struct scmi_info *info,
 	mutex_unlock(&info->devreq_mtx);
 }
 
-void scmi_notification_instance_data_set(const struct scmi_handle *handle,
+void scmi_notification_instance_data_set(struct scmi_handle *handle,
 					 void *priv)
 {
 	struct scmi_info *info = handle_to_scmi_info(handle);
@@ -638,7 +638,7 @@ static int scmi_xfer_inflight_register(struct scmi_xfer *xfer,
  *
  * Return: 0 on Success, error otherwise
  */
-int scmi_xfer_raw_inflight_register(const struct scmi_handle *handle,
+int scmi_xfer_raw_inflight_register(struct scmi_handle *handle,
 				    struct scmi_xfer *xfer)
 {
 	struct scmi_info *info = handle_to_scmi_info(handle);
@@ -730,7 +730,7 @@ static struct scmi_xfer *scmi_xfer_get(const struct scmi_handle *handle,
  *
  * Return: A valid xfer on Success, or an error-pointer otherwise
  */
-struct scmi_xfer *scmi_xfer_raw_get(const struct scmi_handle *handle)
+struct scmi_xfer *scmi_xfer_raw_get(struct scmi_handle *handle)
 {
 	struct scmi_xfer *xfer;
 	struct scmi_info *info = handle_to_scmi_info(handle);
@@ -757,7 +757,7 @@ struct scmi_xfer *scmi_xfer_raw_get(const struct scmi_handle *handle)
  * Return: A reference to the channel to use, or an ERR_PTR
  */
 struct scmi_chan_info *
-scmi_xfer_raw_channel_get(const struct scmi_handle *handle, u8 protocol_id)
+scmi_xfer_raw_channel_get(struct scmi_handle *handle, u8 protocol_id)
 {
 	struct scmi_chan_info *cinfo;
 	struct scmi_info *info = handle_to_scmi_info(handle);
@@ -820,7 +820,7 @@ __scmi_xfer_put(struct scmi_xfers_info *minfo, struct scmi_xfer *xfer)
  * Note that as with other xfer_put() handlers the xfer is really effectively
  * released only if there are no more users on the system.
  */
-void scmi_xfer_raw_put(const struct scmi_handle *handle, struct scmi_xfer *xfer)
+void scmi_xfer_raw_put(struct scmi_handle *handle, struct scmi_xfer *xfer)
 {
 	struct scmi_info *info = handle_to_scmi_info(handle);
 
@@ -2202,7 +2202,7 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info,
 	int ret = -ENOMEM;
 	void *gid;
 	struct scmi_protocol_instance *pi;
-	const struct scmi_handle *handle = &info->handle;
+	struct scmi_handle *handle = &info->handle;
 
 	/* Protocol specific devres group */
 	gid = devres_open_group(handle->dev, NULL, GFP_KERNEL);
@@ -2282,7 +2282,7 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info,
  *	   NOT be found.
  */
 static struct scmi_protocol_instance * __must_check
-scmi_get_protocol_instance(const struct scmi_handle *handle, u8 protocol_id)
+scmi_get_protocol_instance(struct scmi_handle *handle, u8 protocol_id)
 {
 	struct scmi_protocol_instance *pi;
 	struct scmi_info *info = handle_to_scmi_info(handle);
@@ -2317,7 +2317,7 @@ scmi_get_protocol_instance(const struct scmi_handle *handle, u8 protocol_id)
  *
  * Return: 0 if protocol was acquired successfully.
  */
-int scmi_protocol_acquire(const struct scmi_handle *handle, u8 protocol_id)
+int scmi_protocol_acquire(struct scmi_handle *handle, u8 protocol_id)
 {
 	return PTR_ERR_OR_ZERO(scmi_get_protocol_instance(handle, protocol_id));
 }
@@ -2330,7 +2330,7 @@ int scmi_protocol_acquire(const struct scmi_handle *handle, u8 protocol_id)
  * Remove one user for the specified protocol and triggers de-initialization
  * and resources de-allocation once the last user has gone.
  */
-void scmi_protocol_release(const struct scmi_handle *handle, u8 protocol_id)
+void scmi_protocol_release(struct scmi_handle *handle, u8 protocol_id)
 {
 	struct scmi_info *info = handle_to_scmi_info(handle);
 	struct scmi_protocol_instance *pi;
@@ -2372,7 +2372,7 @@ void scmi_setup_protocol_implemented(const struct scmi_protocol_handle *ph,
 }
 
 static bool
-scmi_is_protocol_implemented(const struct scmi_handle *handle, u8 prot_id)
+scmi_is_protocol_implemented(struct scmi_handle *handle, u8 prot_id)
 {
 	int i;
 	struct scmi_info *info = handle_to_scmi_info(handle);
@@ -2388,7 +2388,7 @@ scmi_is_protocol_implemented(const struct scmi_handle *handle, u8 prot_id)
 }
 
 struct scmi_protocol_devres {
-	const struct scmi_handle *handle;
+	struct scmi_handle *handle;
 	u8 protocol_id;
 };
 
@@ -2525,7 +2525,7 @@ static void scmi_devm_protocol_put(struct scmi_device *sdev, u8 protocol_id)
  *
  * Return: True if transport is configured as atomic
  */
-static bool scmi_is_transport_atomic(const struct scmi_handle *handle,
+static bool scmi_is_transport_atomic(struct scmi_handle *handle,
 				     unsigned int *atomic_threshold)
 {
 	bool ret;
@@ -2582,7 +2582,7 @@ static struct scmi_handle *scmi_handle_get(struct device *dev)
  * Return: 0 is successfully released
  *	if null was passed, it returns -EINVAL;
  */
-static int scmi_handle_put(const struct scmi_handle *handle)
+static int scmi_handle_put(struct scmi_handle *handle)
 {
 	struct scmi_info *info;
 
diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
index dee9f238f6fd..bfc1d57ce052 100644
--- a/drivers/firmware/arm_scmi/notify.c
+++ b/drivers/firmware/arm_scmi/notify.c
@@ -1464,7 +1464,7 @@ static int scmi_notifier_unregister(const struct scmi_handle *handle,
 }
 
 struct scmi_notifier_devres {
-	const struct scmi_handle *handle;
+	struct scmi_handle *handle;
 	u8 proto_id;
 	u8 evt_id;
 	u32 __src_id;
diff --git a/drivers/firmware/arm_scmi/raw_mode.c b/drivers/firmware/arm_scmi/raw_mode.c
index 73db5492ab44..efae99febdde 100644
--- a/drivers/firmware/arm_scmi/raw_mode.c
+++ b/drivers/firmware/arm_scmi/raw_mode.c
@@ -172,7 +172,7 @@ struct scmi_raw_queue {
  */
 struct scmi_raw_mode_info {
 	unsigned int id;
-	const struct scmi_handle *handle;
+	struct scmi_handle *handle;
 	const struct scmi_desc *desc;
 	int tx_max_msg;
 	struct scmi_raw_queue *q[SCMI_RAW_MAX_QUEUE];
@@ -1210,7 +1210,7 @@ static int scmi_raw_mode_setup(struct scmi_raw_mode_info *raw,
  *
  * Return: An opaque handle to the Raw instance on Success, an ERR_PTR otherwise
  */
-void *scmi_raw_mode_init(const struct scmi_handle *handle,
+void *scmi_raw_mode_init(struct scmi_handle *handle,
 			 struct dentry *top_dentry, int instance_id,
 			 u8 *channels, int num_chans,
 			 const struct scmi_desc *desc, int tx_max_msg)
diff --git a/drivers/firmware/arm_scmi/raw_mode.h b/drivers/firmware/arm_scmi/raw_mode.h
index 8af756a83fd1..49895b81bc3b 100644
--- a/drivers/firmware/arm_scmi/raw_mode.h
+++ b/drivers/firmware/arm_scmi/raw_mode.h
@@ -17,7 +17,7 @@ enum {
 	SCMI_RAW_MAX_QUEUE
 };
 
-void *scmi_raw_mode_init(const struct scmi_handle *handle,
+void *scmi_raw_mode_init(struct scmi_handle *handle,
 			 struct dentry *top_dentry, int instance_id,
 			 u8 *channels, int num_chans,
 			 const struct scmi_desc *desc, int tx_max_msg);
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index aafaac1496b0..17095e41f5c6 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -909,7 +909,7 @@ struct scmi_handle {
 		(*devm_protocol_get)(struct scmi_device *sdev, u8 proto,
 				     struct scmi_protocol_handle **ph);
 	void (*devm_protocol_put)(struct scmi_device *sdev, u8 proto);
-	bool (*is_transport_atomic)(const struct scmi_handle *handle,
+	bool (*is_transport_atomic)(struct scmi_handle *handle,
 				    unsigned int *atomic_threshold);
 
 	const struct scmi_notify_ops *notify_ops;

-- 
2.51.0

[PATCH 2/4] firmware: arm_scmi: Drop fake 'const' on scmi_protocol_handle

[Krzysztof Kozlowski]

Severale functions operating on the 'handle' pointer, like
scmi_set_protocol_priv(), are claiming it is a pointer to const thus
they should not modify the handle.  In fact that's a false statement,
because first thing these functions do is drop the cast to const with
container_of:

  struct scmi_protocol_instance *pi = ph_to_pi(ph);

And with such cast the handle is easily writable with simple:

  pi->ph.dev = NULL;

If the function really was not modifying the pointed handle, it would
use the container_of_const() call.

The code is not correct logically, either, because functions like
scmi_set_protocol_priv() are meant to modify the data behind the handle
(in containing struct).

The code does not have actual visible bug, but incorrect 'const'
annotations could lead to incorrect compiler decisions.

Fixes: d7b6cc563a60 ("firmware: arm_scmi: Introduce protocol handle definitions")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
 drivers/firmware/arm_scmi/base.c                    | 2 +-
 drivers/firmware/arm_scmi/clock.c                   | 2 +-
 drivers/firmware/arm_scmi/driver.c                  | 2 +-
 drivers/firmware/arm_scmi/perf.c                    | 2 +-
 drivers/firmware/arm_scmi/pinctrl.c                 | 4 ++--
 drivers/firmware/arm_scmi/power.c                   | 2 +-
 drivers/firmware/arm_scmi/powercap.c                | 2 +-
 drivers/firmware/arm_scmi/protocols.h               | 4 ++--
 drivers/firmware/arm_scmi/reset.c                   | 2 +-
 drivers/firmware/arm_scmi/sensors.c                 | 2 +-
 drivers/firmware/arm_scmi/system.c                  | 2 +-
 drivers/firmware/arm_scmi/vendors/imx/imx-sm-bbm.c  | 2 +-
 drivers/firmware/arm_scmi/vendors/imx/imx-sm-cpu.c  | 2 +-
 drivers/firmware/arm_scmi/vendors/imx/imx-sm-lmm.c  | 2 +-
 drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c | 2 +-
 drivers/firmware/arm_scmi/voltage.c                 | 2 +-
 16 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
index 22267bbd0f4d..8302aaacdf43 100644
--- a/drivers/firmware/arm_scmi/base.c
+++ b/drivers/firmware/arm_scmi/base.c
@@ -371,7 +371,7 @@ static const struct scmi_protocol_events base_protocol_events = {
 	.num_sources = SCMI_BASE_NUM_SOURCES,
 };
 
-static int scmi_base_protocol_init(const struct scmi_protocol_handle *ph)
+static int scmi_base_protocol_init(struct scmi_protocol_handle *ph)
 {
 	int id, ret;
 	u8 *prot_imp;
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index ab36871650a1..58cc392c1c80 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -1064,7 +1064,7 @@ static const struct scmi_protocol_events clk_protocol_events = {
 	.num_events = ARRAY_SIZE(clk_events),
 };
 
-static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
+static int scmi_clock_protocol_init(struct scmi_protocol_handle *ph)
 {
 	int clkid, ret;
 	struct clock_info *cinfo;
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 8b27e74d8a19..951711aa7d33 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1630,7 +1630,7 @@ static int version_get(const struct scmi_protocol_handle *ph, u32 *version)
  *
  * Return: 0 on Success
  */
-static int scmi_set_protocol_priv(const struct scmi_protocol_handle *ph,
+static int scmi_set_protocol_priv(struct scmi_protocol_handle *ph,
 				  void *priv)
 {
 	struct scmi_protocol_instance *pi = ph_to_pi(ph);
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 4583d02bee1c..1c0a150612a8 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -1266,7 +1266,7 @@ static const struct scmi_protocol_events perf_protocol_events = {
 	.num_events = ARRAY_SIZE(perf_events),
 };
 
-static int scmi_perf_protocol_init(const struct scmi_protocol_handle *ph)
+static int scmi_perf_protocol_init(struct scmi_protocol_handle *ph)
 {
 	int domain, ret;
 	struct scmi_perf_info *pinfo;
diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
index a020e23d7c49..99f98eb6808d 100644
--- a/drivers/firmware/arm_scmi/pinctrl.c
+++ b/drivers/firmware/arm_scmi/pinctrl.c
@@ -827,7 +827,7 @@ static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {
 	.pin_free = scmi_pinctrl_pin_free,
 };
 
-static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
+static int scmi_pinctrl_protocol_init(struct scmi_protocol_handle *ph)
 {
 	int ret;
 	struct scmi_pinctrl_info *pinfo;
@@ -861,7 +861,7 @@ static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
 	return ph->set_priv(ph, pinfo);
 }
 
-static int scmi_pinctrl_protocol_deinit(const struct scmi_protocol_handle *ph)
+static int scmi_pinctrl_protocol_deinit(struct scmi_protocol_handle *ph)
 {
 	int i;
 	struct scmi_pinctrl_info *pi = ph->get_priv(ph);
diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
index bb5062ab8280..00a9f53295f6 100644
--- a/drivers/firmware/arm_scmi/power.c
+++ b/drivers/firmware/arm_scmi/power.c
@@ -319,7 +319,7 @@ static const struct scmi_protocol_events power_protocol_events = {
 	.num_events = ARRAY_SIZE(power_events),
 };
 
-static int scmi_power_protocol_init(const struct scmi_protocol_handle *ph)
+static int scmi_power_protocol_init(struct scmi_protocol_handle *ph)
 {
 	int domain, ret;
 	struct scmi_power_info *pinfo;
diff --git a/drivers/firmware/arm_scmi/powercap.c b/drivers/firmware/arm_scmi/powercap.c
index ab9733f4458b..ac527f59bc1e 100644
--- a/drivers/firmware/arm_scmi/powercap.c
+++ b/drivers/firmware/arm_scmi/powercap.c
@@ -957,7 +957,7 @@ static const struct scmi_protocol_events powercap_protocol_events = {
 };
 
 static int
-scmi_powercap_protocol_init(const struct scmi_protocol_handle *ph)
+scmi_powercap_protocol_init(struct scmi_protocol_handle *ph)
 {
 	int domain, ret;
 	struct powercap_info *pinfo;
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index 4c75970326e6..309e834e5392 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -183,7 +183,7 @@ struct scmi_protocol_handle {
 	unsigned int version;
 	const struct scmi_xfer_ops *xops;
 	const struct scmi_proto_helpers_ops *hops;
-	int (*set_priv)(const struct scmi_protocol_handle *ph, void *priv);
+	int (*set_priv)(struct scmi_protocol_handle *ph, void *priv);
 	void *(*get_priv)(const struct scmi_protocol_handle *ph);
 };
 
@@ -315,7 +315,7 @@ struct scmi_xfer_ops {
 			 struct scmi_xfer *xfer);
 };
 
-typedef int (*scmi_prot_init_ph_fn_t)(const struct scmi_protocol_handle *);
+typedef int (*scmi_prot_init_ph_fn_t)(struct scmi_protocol_handle *);
 
 /**
  * struct scmi_protocol  - Protocol descriptor
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index 4bc5c24c2d72..532ebac3286a 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -351,7 +351,7 @@ static const struct scmi_protocol_events reset_protocol_events = {
 	.num_events = ARRAY_SIZE(reset_events),
 };
 
-static int scmi_reset_protocol_init(const struct scmi_protocol_handle *ph)
+static int scmi_reset_protocol_init(struct scmi_protocol_handle *ph)
 {
 	int domain, ret;
 	struct scmi_reset_info *pinfo;
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 882d55f987d2..0a2b3bb83cc9 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -1144,7 +1144,7 @@ static const struct scmi_protocol_events sensor_protocol_events = {
 	.num_events = ARRAY_SIZE(sensor_events),
 };
 
-static int scmi_sensors_protocol_init(const struct scmi_protocol_handle *ph)
+static int scmi_sensors_protocol_init(struct scmi_protocol_handle *ph)
 {
 	int ret;
 	struct sensors_info *sinfo;
diff --git a/drivers/firmware/arm_scmi/system.c b/drivers/firmware/arm_scmi/system.c
index 0f51c36f6a9d..dfb7183d1f14 100644
--- a/drivers/firmware/arm_scmi/system.c
+++ b/drivers/firmware/arm_scmi/system.c
@@ -138,7 +138,7 @@ static const struct scmi_protocol_events system_protocol_events = {
 	.num_sources = SCMI_SYSTEM_NUM_SOURCES,
 };
 
-static int scmi_system_protocol_init(const struct scmi_protocol_handle *ph)
+static int scmi_system_protocol_init(struct scmi_protocol_handle *ph)
 {
 	struct scmi_system_info *pinfo;
 
diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-bbm.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-bbm.c
index 33f9ebf6092b..1f569951bb31 100644
--- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-bbm.c
+++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-bbm.c
@@ -342,7 +342,7 @@ static const struct scmi_imx_bbm_proto_ops scmi_imx_bbm_proto_ops = {
 	.button_get = scmi_imx_bbm_button_get,
 };
 
-static int scmi_imx_bbm_protocol_init(const struct scmi_protocol_handle *ph)
+static int scmi_imx_bbm_protocol_init(struct scmi_protocol_handle *ph)
 {
 	int ret;
 	struct scmi_imx_bbm_info *binfo;
diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-cpu.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-cpu.c
index 753274af11d2..928d93a8603f 100644
--- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-cpu.c
+++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-cpu.c
@@ -230,7 +230,7 @@ static int scmi_imx_cpu_attributes_get(const struct scmi_protocol_handle *ph,
 	return ret;
 }
 
-static int scmi_imx_cpu_protocol_init(const struct scmi_protocol_handle *ph)
+static int scmi_imx_cpu_protocol_init(struct scmi_protocol_handle *ph)
 {
 	struct scmi_imx_cpu_info *info;
 	int ret, i;
diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-lmm.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-lmm.c
index c56ae247774d..0a9af7ed1981 100644
--- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-lmm.c
+++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-lmm.c
@@ -223,7 +223,7 @@ static int scmi_imx_lmm_protocol_attributes_get(const struct scmi_protocol_handl
 	return ret;
 }
 
-static int scmi_imx_lmm_protocol_init(const struct scmi_protocol_handle *ph)
+static int scmi_imx_lmm_protocol_init(struct scmi_protocol_handle *ph)
 {
 	struct scmi_imx_lmm_priv *info;
 	int ret;
diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
index 0ada753367ef..79c7966888be 100644
--- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
+++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
@@ -459,7 +459,7 @@ static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
 	.misc_syslog = scmi_imx_misc_syslog_get,
 };
 
-static int scmi_imx_misc_protocol_init(const struct scmi_protocol_handle *ph)
+static int scmi_imx_misc_protocol_init(struct scmi_protocol_handle *ph)
 {
 	struct scmi_imx_misc_info *minfo;
 	int ret;
diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
index b9391c1ee8a0..60c3405b4999 100644
--- a/drivers/firmware/arm_scmi/voltage.c
+++ b/drivers/firmware/arm_scmi/voltage.c
@@ -401,7 +401,7 @@ static const struct scmi_voltage_proto_ops voltage_proto_ops = {
 	.level_get = scmi_voltage_level_get,
 };
 
-static int scmi_voltage_protocol_init(const struct scmi_protocol_handle *ph)
+static int scmi_voltage_protocol_init(struct scmi_protocol_handle *ph)
 {
 	int ret;
 	struct voltage_info *vinfo;

-- 
2.51.0

Re: [PATCH 0/4] firmware: arm_scmi: Drop fake 'const' on scmi_handle

[Cristian Marussi]

On Tue, Feb 24, 2026 at 11:43:38AM +0100, Krzysztof Kozlowski wrote:
> Severale functions operating on the 'handle' pointer, like

Hi Krzysztof,

> scmi_handle_put() or scmi_xfer_raw_get(), are claiming it is a pointer
> to const thus they should not modify the handle.  In fact that's a false
> statement, because first thing these functions do is drop the cast to
> const with container_of:

Thanks for this first of all...

...but :D

... the SCMI stack attempts to follow a sort of layered design, so roughly
we have transport, core, protocols and finally SCMI Drivers.

Each of these layers has its own responsabilities and the aim was to
enforce some sort of isolation between these layers, OR even between
different disjoint features within the same layer, if it made sense,
like with the notification subsystem within the core...

Now, given that all of the above must be attained using our beloved C
language, such attempt to enforce isolation between such 'islands' with
different responsibilities is based on:

 - fully opaque pointers/handles...like the ph protocol handels within
   SCMI drivers...best way to do it..if you cannot even peek into an
   object you certainly cannot mess with it...

 - some 'constification' when passing around some nonm-opaque references
   across such boundaries

So, when you say that some of these functions sports a fake const
reference, is certainly true to some extent, BUT you miss the fact that
usually the const is meant to stop the CALLER from messing freely with
the handle and instead enforce the usage of a dedicated helper that sits
in another layer...

As an example, when you say that the scmi_protocol_handle *ph is indeed
manipulated by scmi_protocol_set_priv() and so it is NOT const, you are
certainly right, BUT the above function and the protocol handle itself
lives in the core, a different layer from the protocols, and indeed the
protocol_init function cannot change directly the protocol priv value
but instead has to pass through the helper ph->set_priv() which is the
only helper that can touch the ph content...
...IOW you are forced to respect the isolation boundary (as much as
possible) by the constification of ph...if you drop the const in the
protocol_init protoypes you end opening the stack to all sort of
cross-boundary manipulations annd hacks: helpers like set_priv were
added to be able to manipulate the bits that needed to be modifiable
while maintaining separation of resposibilities between latyers.

Similarly for notifications, they are kept isolated as much as possible
from the core.

So, I still have definitely to properly go through all of your
series, but while the usage of container_of_const() is certainly a
welcome addition, because it raises the isolation factor, dropping the
'fake' const seems to me a step back in the enforcement of isolation
boundaries between different layers or different subsystems within the
same layer.

IOW, the last that we want is to be able to freely change the content
of such const handles from outside the 'island' they live in...

Any improvement on such isolation with more modern C tricks, if
possible, is pretty much welcome, i.e. I am not saying that the current
system is perfect and not improvable...but just dropping all of this in
name of some better possible compilation optimization seems not worth in
terms of maintainability...

Does any of the above blabbing of mine makes sense :P ?

Thanks,
Cristian

Re: [PATCH 0/4] firmware: arm_scmi: Drop fake 'const' on scmi_handle

[Krzysztof Kozlowski]

On 24/02/2026 13:31, Cristian Marussi wrote:
> On Tue, Feb 24, 2026 at 11:43:38AM +0100, Krzysztof Kozlowski wrote:
>> Severale functions operating on the 'handle' pointer, like
> 
> Hi Krzysztof,
> 
>> scmi_handle_put() or scmi_xfer_raw_get(), are claiming it is a pointer
>> to const thus they should not modify the handle.  In fact that's a false
>> statement, because first thing these functions do is drop the cast to
>> const with container_of:
> 
> Thanks for this first of all...
> 
> ...but :D
> 
> ... the SCMI stack attempts to follow a sort of layered design, so roughly
> we have transport, core, protocols and finally SCMI Drivers.
> 
> Each of these layers has its own responsabilities and the aim was to
> enforce some sort of isolation between these layers, OR even between
> different disjoint features within the same layer, if it made sense,
> like with the notification subsystem within the core...
> 
> Now, given that all of the above must be attained using our beloved C
> language, such attempt to enforce isolation between such 'islands' with
> different responsibilities is based on:
> 
>  - fully opaque pointers/handles...like the ph protocol handels within
>    SCMI drivers...best way to do it..if you cannot even peek into an
>    object you certainly cannot mess with it...
> 
>  - some 'constification' when passing around some nonm-opaque references
>    across such boundaries
> 
> So, when you say that some of these functions sports a fake const
> reference, is certainly true to some extent, BUT you miss the fact that
> usually the const is meant to stop the CALLER from messing freely with
> the handle and instead enforce the usage of a dedicated helper that sits
> in another layer...

The caller can mess with the handle, because of container_of() cast, so
there is nothing stopping it. I understand you want to express that
handle is somehow unchangeable but then as you mentioned - it should be
opaque pointer.

> 
> As an example, when you say that the scmi_protocol_handle *ph is indeed
> manipulated by scmi_protocol_set_priv() and so it is NOT const, you are
> certainly right, BUT the above function and the protocol handle itself
> lives in the core, a different layer from the protocols, and indeed the
> protocol_init function cannot change directly the protocol priv value
> but instead has to pass through the helper ph->set_priv() which is the
> only helper that can touch the ph content...
> ...IOW you are forced to respect the isolation boundary (as much as
> possible) by the constification of ph...if you drop the const in the
> protocol_init protoypes you end opening the stack to all sort of
> cross-boundary manipulations annd hacks: helpers like set_priv were
> added to be able to manipulate the bits that needed to be modifiable
> while maintaining separation of resposibilities between latyers.
> 
> Similarly for notifications, they are kept isolated as much as possible
> from the core.

I understand the goal this code tried to achieve. And it did achieve
it... plus another goal of having "const" like functions modifying
memory. This is not a readable code. Function which receives only
arguments as values and pointers to const is expected to not modify
state of received pointed data. But all the functions here, because of
the cast, can or even do modify.

I understand we do not write here C++ const methods, obviously. But all
these functions look re-entrant from the interface point of view but in
fact are not re-entrant.


> 
> So, I still have definitely to properly go through all of your
> series, but while the usage of container_of_const() is certainly a
> welcome addition, because it raises the isolation factor, dropping the
> 'fake' const seems to me a step back in the enforcement of isolation
> boundaries between different layers or different subsystems within the
> same layer.
> 
> IOW, the last that we want is to be able to freely change the content
> of such const handles from outside the 'island' they live in...

If I understood correctly the composition here:

The handle should be in such case be not contained in the upper
structure, but be a pointer to const like:

struct scmi_protocol_instance {
	...
-	struct scmi_protocol_handle     ph;
+	const struct scmi_protocol_handle     *ph;
}

And since this is 1-to-1 relationship, the scmi_protocol_handle should
have a pointer to scmi_protocol_instance. This way you keep passing
pointer to const handle without ever needing to cast it.

The current solution would be much nicer than above, if the code was not
dropping const, IMO.

> 
> Any improvement on such isolation with more modern C tricks, if
> possible, is pretty much welcome, i.e. I am not saying that the current
> system is perfect and not improvable...but just dropping all of this in
> name of some better possible compilation optimization seems not worth in
> terms of maintainability...
> 
> Does any of the above blabbing of mine makes sense :P ?
> 
Best regards,
Krzysztof

Re: [PATCH 0/4] firmware: arm_scmi: Drop fake 'const' on scmi_handle

[Cristian Marussi]

On Wed, Feb 25, 2026 at 12:42:08PM +0100, Krzysztof Kozlowski wrote:
> On 24/02/2026 13:31, Cristian Marussi wrote:
> > On Tue, Feb 24, 2026 at 11:43:38AM +0100, Krzysztof Kozlowski wrote:
> >> Severale functions operating on the 'handle' pointer, like
> > 
> > Hi Krzysztof,

Hi,

> > 
> >> scmi_handle_put() or scmi_xfer_raw_get(), are claiming it is a pointer
> >> to const thus they should not modify the handle.  In fact that's a false
> >> statement, because first thing these functions do is drop the cast to
> >> const with container_of:
> > 
> > Thanks for this first of all...
> > 
> > ...but :D
> > 
> > ... the SCMI stack attempts to follow a sort of layered design, so roughly
> > we have transport, core, protocols and finally SCMI Drivers.
> > 
> > Each of these layers has its own responsabilities and the aim was to
> > enforce some sort of isolation between these layers, OR even between
> > different disjoint features within the same layer, if it made sense,
> > like with the notification subsystem within the core...
> > 
> > Now, given that all of the above must be attained using our beloved C
> > language, such attempt to enforce isolation between such 'islands' with
> > different responsibilities is based on:
> > 
> >  - fully opaque pointers/handles...like the ph protocol handels within
> >    SCMI drivers...best way to do it..if you cannot even peek into an
> >    object you certainly cannot mess with it...
> > 
> >  - some 'constification' when passing around some nonm-opaque references
> >    across such boundaries
> > 
> > So, when you say that some of these functions sports a fake const
> > reference, is certainly true to some extent, BUT you miss the fact that
> > usually the const is meant to stop the CALLER from messing freely with
> > the handle and instead enforce the usage of a dedicated helper that sits
> > in another layer...
> 
> The caller can mess with the handle, because of container_of() cast, so
> there is nothing stopping it. I understand you want to express that
> handle is somehow unchangeable but then as you mentioned - it should be
> opaque pointer.
> 

Well no, the caller who calls from within the _protocol_init code can only
do what the available function (set_priv) allows him to do when called....
...like setting the priv field....and scmi_set_protocol_priv, which live in
another 'logical island', embeds that logic....from within protocol_init
you cannot get to the protocol instance directly simply because you dont
have the definition of struct nor the ph_to_pi() macros...

Also scmi_set_protocol_priv() uses the const ph to access the container
pi and change that...NOT the ph object...even though clearly it could once
it has pi (as you pointed out)....but that would be a bug...no ?

... on the other side dropping the const from the protocol_init funcs as
you suggest would mean that immediately you could do from within the protocol
layer stuff like:

	ph->version = 0x12345;

...while now you can only read ph->version, since it is discovered
negoatiated and set by the core SCMI stack and so it is NOT meant to be
touched by the protocol layer, while it has to be freely modificable by
the core...(sp it cannot be real const in the core)

Yes, ideally ph would be better as an opaque object of course, like it
appears to the SCMI drivers when it is used to call proto_ops, BUT the
attempt here was to maximize isolation while also keeping the thing
practically usable AND without having to export zillion symbols...

...that is the reason also for ph-embeeded xops/hops...if you make ph fully
opaque also to the protocol layer you will have to expose and EXPORT
xops/hops since vendor protocols can be loadable modules, and in general
all across the SCMI stack we generally always chose to export the minimum
possible number of symbols and a few handles with attached ops to let the
stack work, since if not, given the nature of the SCMI protocol that is
highly extensible, we would have ended up with a constantly increasing
number of exported symbols to maintain...
(imagine to export all of scmi_protocol.h)

I agree that the current const usage patterns in the stack are semantically
slightly off at time, but what is the real benefit of dropping such fake
const ?

...because on one side they do stop any attempt to mess with the internals
of the handles IF such attempts comes from a context in which the objects
are NOT supposed to be touched, and so, in some way enforce and constraint
the developer writing these protocols (standard or vendor) to 'behave'

...BUT, on the other side,  what would be the gain in having such consts
insteaD more pedantically applied (dropepd) ? What are the compilation time
benefits that you mention ?

Thanks,
Cristian