* [PATCH v2] firmware: exynos-acpm: Drop fake 'const' on handle pointer
@ 2026-02-24 10:42 Krzysztof Kozlowski
2026-02-24 12:57 ` Tudor Ambarus
2026-02-28 14:58 ` Krzysztof Kozlowski
0 siblings, 2 replies; 6+ messages in thread
From: Krzysztof Kozlowski @ 2026-02-24 10:42 UTC (permalink / raw)
To: Tudor Ambarus, Krzysztof Kozlowski, Sylwester Nawrocki,
Chanwoo Choi, Alim Akhtar, Michael Turquette, Stephen Boyd,
André Draszik, Lee Jones, linux-kernel, linux-samsung-soc,
linux-clk, linux-arm-kernel
Cc: Krzysztof Kozlowski, stable
All the functions operating on the 'handle' pointer 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 acpm_info *acpm = handle_to_acpm_info(handle);
And with such cast the handle is easily writable with simple:
acpm->handle.ops.pmic_ops.read_reg = NULL;
The code is not correct logically, either, because functions like
acpm_get_by_node() and acpm_handle_put() are meant to modify the handle
reference counting, thus they must modify the handle. Modification here
happens anyway, even if the reference counting is stored in the
container which the handle is part of.
The code does not have actual visible bug, but incorrect 'const'
annotations could lead to incorrect compiler decisions.
Fixes: a88927b534ba ("firmware: add Exynos ACPM protocol driver")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
I will have more patches for more drivers like TI, ARM SCMI...
Changes in v2:
1. Update also clk and mfd drivers, fixing build (do'h!) failure.
With Lee's blessing I can take the patch via Samsung.
---
drivers/clk/samsung/clk-acpm.c | 4 +-
drivers/firmware/samsung/exynos-acpm-dvfs.c | 4 +-
drivers/firmware/samsung/exynos-acpm-dvfs.h | 4 +-
drivers/firmware/samsung/exynos-acpm-pmic.c | 10 ++---
drivers/firmware/samsung/exynos-acpm-pmic.h | 10 ++---
drivers/firmware/samsung/exynos-acpm.c | 16 ++++----
drivers/firmware/samsung/exynos-acpm.h | 2 +-
drivers/mfd/sec-acpm.c | 10 ++---
.../firmware/samsung/exynos-acpm-protocol.h | 40 ++++++++-----------
9 files changed, 48 insertions(+), 52 deletions(-)
diff --git a/drivers/clk/samsung/clk-acpm.c b/drivers/clk/samsung/clk-acpm.c
index b90809ce3f88..d8944160793a 100644
--- a/drivers/clk/samsung/clk-acpm.c
+++ b/drivers/clk/samsung/clk-acpm.c
@@ -20,7 +20,7 @@ struct acpm_clk {
u32 id;
struct clk_hw hw;
unsigned int mbox_chan_id;
- const struct acpm_handle *handle;
+ struct acpm_handle *handle;
};
struct acpm_clk_variant {
@@ -113,7 +113,7 @@ static int acpm_clk_register(struct device *dev, struct acpm_clk *aclk,
static int acpm_clk_probe(struct platform_device *pdev)
{
- const struct acpm_handle *acpm_handle;
+ struct acpm_handle *acpm_handle;
struct clk_hw_onecell_data *clk_data;
struct clk_hw **hws;
struct device *dev = &pdev->dev;
diff --git a/drivers/firmware/samsung/exynos-acpm-dvfs.c b/drivers/firmware/samsung/exynos-acpm-dvfs.c
index 1c5b2b143bcc..66448c8037ac 100644
--- a/drivers/firmware/samsung/exynos-acpm-dvfs.c
+++ b/drivers/firmware/samsung/exynos-acpm-dvfs.c
@@ -42,7 +42,7 @@ static void acpm_dvfs_init_set_rate_cmd(u32 cmd[4], unsigned int clk_id,
cmd[3] = ktime_to_ms(ktime_get());
}
-int acpm_dvfs_set_rate(const struct acpm_handle *handle,
+int acpm_dvfs_set_rate(struct acpm_handle *handle,
unsigned int acpm_chan_id, unsigned int clk_id,
unsigned long rate)
{
@@ -62,7 +62,7 @@ static void acpm_dvfs_init_get_rate_cmd(u32 cmd[4], unsigned int clk_id)
cmd[3] = ktime_to_ms(ktime_get());
}
-unsigned long acpm_dvfs_get_rate(const struct acpm_handle *handle,
+unsigned long acpm_dvfs_get_rate(struct acpm_handle *handle,
unsigned int acpm_chan_id, unsigned int clk_id)
{
struct acpm_xfer xfer;
diff --git a/drivers/firmware/samsung/exynos-acpm-dvfs.h b/drivers/firmware/samsung/exynos-acpm-dvfs.h
index 9f2778e649c9..b37b15426102 100644
--- a/drivers/firmware/samsung/exynos-acpm-dvfs.h
+++ b/drivers/firmware/samsung/exynos-acpm-dvfs.h
@@ -11,10 +11,10 @@
struct acpm_handle;
-int acpm_dvfs_set_rate(const struct acpm_handle *handle,
+int acpm_dvfs_set_rate(struct acpm_handle *handle,
unsigned int acpm_chan_id, unsigned int id,
unsigned long rate);
-unsigned long acpm_dvfs_get_rate(const struct acpm_handle *handle,
+unsigned long acpm_dvfs_get_rate(struct acpm_handle *handle,
unsigned int acpm_chan_id,
unsigned int clk_id);
diff --git a/drivers/firmware/samsung/exynos-acpm-pmic.c b/drivers/firmware/samsung/exynos-acpm-pmic.c
index 961d7599e422..52e89d1b790f 100644
--- a/drivers/firmware/samsung/exynos-acpm-pmic.c
+++ b/drivers/firmware/samsung/exynos-acpm-pmic.c
@@ -77,7 +77,7 @@ static void acpm_pmic_init_read_cmd(u32 cmd[4], u8 type, u8 reg, u8 chan)
cmd[3] = ktime_to_ms(ktime_get());
}
-int acpm_pmic_read_reg(const struct acpm_handle *handle,
+int acpm_pmic_read_reg(struct acpm_handle *handle,
unsigned int acpm_chan_id, u8 type, u8 reg, u8 chan,
u8 *buf)
{
@@ -107,7 +107,7 @@ static void acpm_pmic_init_bulk_read_cmd(u32 cmd[4], u8 type, u8 reg, u8 chan,
FIELD_PREP(ACPM_PMIC_VALUE, count);
}
-int acpm_pmic_bulk_read(const struct acpm_handle *handle,
+int acpm_pmic_bulk_read(struct acpm_handle *handle,
unsigned int acpm_chan_id, u8 type, u8 reg, u8 chan,
u8 count, u8 *buf)
{
@@ -150,7 +150,7 @@ static void acpm_pmic_init_write_cmd(u32 cmd[4], u8 type, u8 reg, u8 chan,
cmd[3] = ktime_to_ms(ktime_get());
}
-int acpm_pmic_write_reg(const struct acpm_handle *handle,
+int acpm_pmic_write_reg(struct acpm_handle *handle,
unsigned int acpm_chan_id, u8 type, u8 reg, u8 chan,
u8 value)
{
@@ -187,7 +187,7 @@ static void acpm_pmic_init_bulk_write_cmd(u32 cmd[4], u8 type, u8 reg, u8 chan,
}
}
-int acpm_pmic_bulk_write(const struct acpm_handle *handle,
+int acpm_pmic_bulk_write(struct acpm_handle *handle,
unsigned int acpm_chan_id, u8 type, u8 reg, u8 chan,
u8 count, const u8 *buf)
{
@@ -220,7 +220,7 @@ static void acpm_pmic_init_update_cmd(u32 cmd[4], u8 type, u8 reg, u8 chan,
cmd[3] = ktime_to_ms(ktime_get());
}
-int acpm_pmic_update_reg(const struct acpm_handle *handle,
+int acpm_pmic_update_reg(struct acpm_handle *handle,
unsigned int acpm_chan_id, u8 type, u8 reg, u8 chan,
u8 value, u8 mask)
{
diff --git a/drivers/firmware/samsung/exynos-acpm-pmic.h b/drivers/firmware/samsung/exynos-acpm-pmic.h
index 078421888a14..88ae9aada2ae 100644
--- a/drivers/firmware/samsung/exynos-acpm-pmic.h
+++ b/drivers/firmware/samsung/exynos-acpm-pmic.h
@@ -11,19 +11,19 @@
struct acpm_handle;
-int acpm_pmic_read_reg(const struct acpm_handle *handle,
+int acpm_pmic_read_reg(struct acpm_handle *handle,
unsigned int acpm_chan_id, u8 type, u8 reg, u8 chan,
u8 *buf);
-int acpm_pmic_bulk_read(const struct acpm_handle *handle,
+int acpm_pmic_bulk_read(struct acpm_handle *handle,
unsigned int acpm_chan_id, u8 type, u8 reg, u8 chan,
u8 count, u8 *buf);
-int acpm_pmic_write_reg(const struct acpm_handle *handle,
+int acpm_pmic_write_reg(struct acpm_handle *handle,
unsigned int acpm_chan_id, u8 type, u8 reg, u8 chan,
u8 value);
-int acpm_pmic_bulk_write(const struct acpm_handle *handle,
+int acpm_pmic_bulk_write(struct acpm_handle *handle,
unsigned int acpm_chan_id, u8 type, u8 reg, u8 chan,
u8 count, const u8 *buf);
-int acpm_pmic_update_reg(const struct acpm_handle *handle,
+int acpm_pmic_update_reg(struct acpm_handle *handle,
unsigned int acpm_chan_id, u8 type, u8 reg, u8 chan,
u8 value, u8 mask);
#endif /* __EXYNOS_ACPM_PMIC_H__ */
diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c
index 0cb269c70460..987b59778ffc 100644
--- a/drivers/firmware/samsung/exynos-acpm.c
+++ b/drivers/firmware/samsung/exynos-acpm.c
@@ -412,7 +412,7 @@ static int acpm_wait_for_message_response(struct acpm_chan *achan,
*
* Return: 0 on success, -errno otherwise.
*/
-int acpm_do_xfer(const struct acpm_handle *handle, const struct acpm_xfer *xfer)
+int acpm_do_xfer(struct acpm_handle *handle, const struct acpm_xfer *xfer)
{
struct acpm_info *acpm = handle_to_acpm_info(handle);
struct exynos_mbox_msg msg;
@@ -674,7 +674,7 @@ static int acpm_probe(struct platform_device *pdev)
* acpm_handle_put() - release the handle acquired by acpm_get_by_phandle.
* @handle: Handle acquired by acpm_get_by_phandle.
*/
-static void acpm_handle_put(const struct acpm_handle *handle)
+static void acpm_handle_put(struct acpm_handle *handle)
{
struct acpm_info *acpm = handle_to_acpm_info(handle);
struct device *dev = acpm->dev;
@@ -700,9 +700,11 @@ static void devm_acpm_release(struct device *dev, void *res)
* @np: ACPM device tree node.
*
* Return: pointer to handle on success, ERR_PTR(-errno) otherwise.
+ *
+ * Note: handle CANNOT be pointer to const
*/
-static const struct acpm_handle *acpm_get_by_node(struct device *dev,
- struct device_node *np)
+static struct acpm_handle *acpm_get_by_node(struct device *dev,
+ struct device_node *np)
{
struct platform_device *pdev;
struct device_link *link;
@@ -743,10 +745,10 @@ static const struct acpm_handle *acpm_get_by_node(struct device *dev,
*
* Return: pointer to handle on success, ERR_PTR(-errno) otherwise.
*/
-const struct acpm_handle *devm_acpm_get_by_node(struct device *dev,
- struct device_node *np)
+struct acpm_handle *devm_acpm_get_by_node(struct device *dev,
+ struct device_node *np)
{
- const struct acpm_handle **ptr, *handle;
+ struct acpm_handle **ptr, *handle;
ptr = devres_alloc(devm_acpm_release, sizeof(*ptr), GFP_KERNEL);
if (!ptr)
diff --git a/drivers/firmware/samsung/exynos-acpm.h b/drivers/firmware/samsung/exynos-acpm.h
index 2d14cb58f98c..6417550f89aa 100644
--- a/drivers/firmware/samsung/exynos-acpm.h
+++ b/drivers/firmware/samsung/exynos-acpm.h
@@ -17,7 +17,7 @@ struct acpm_xfer {
struct acpm_handle;
-int acpm_do_xfer(const struct acpm_handle *handle,
+int acpm_do_xfer(struct acpm_handle *handle,
const struct acpm_xfer *xfer);
#endif /* __EXYNOS_ACPM_H__ */
diff --git a/drivers/mfd/sec-acpm.c b/drivers/mfd/sec-acpm.c
index 537ea65685bf..0e23b9d9f7ee 100644
--- a/drivers/mfd/sec-acpm.c
+++ b/drivers/mfd/sec-acpm.c
@@ -367,7 +367,7 @@ static const struct regmap_config s2mpg11_regmap_config_meter = {
};
struct sec_pmic_acpm_shared_bus_context {
- const struct acpm_handle *acpm;
+ struct acpm_handle *acpm;
unsigned int acpm_chan_id;
u8 speedy_channel;
};
@@ -390,7 +390,7 @@ static int sec_pmic_acpm_bus_write(void *context, const void *data,
size_t count)
{
struct sec_pmic_acpm_bus_context *ctx = context;
- const struct acpm_handle *acpm = ctx->shared->acpm;
+ struct acpm_handle *acpm = ctx->shared->acpm;
const struct acpm_pmic_ops *pmic_ops = &acpm->ops.pmic_ops;
size_t val_count = count - BITS_TO_BYTES(ACPM_ADDR_BITS);
const u8 *d = data;
@@ -410,7 +410,7 @@ static int sec_pmic_acpm_bus_read(void *context, const void *reg_buf, size_t reg
void *val_buf, size_t val_size)
{
struct sec_pmic_acpm_bus_context *ctx = context;
- const struct acpm_handle *acpm = ctx->shared->acpm;
+ struct acpm_handle *acpm = ctx->shared->acpm;
const struct acpm_pmic_ops *pmic_ops = &acpm->ops.pmic_ops;
const u8 *r = reg_buf;
u8 reg;
@@ -429,7 +429,7 @@ static int sec_pmic_acpm_bus_reg_update_bits(void *context, unsigned int reg, un
unsigned int val)
{
struct sec_pmic_acpm_bus_context *ctx = context;
- const struct acpm_handle *acpm = ctx->shared->acpm;
+ struct acpm_handle *acpm = ctx->shared->acpm;
const struct acpm_pmic_ops *pmic_ops = &acpm->ops.pmic_ops;
return pmic_ops->update_reg(acpm, ctx->shared->acpm_chan_id, ctx->type, reg & 0xff,
@@ -480,7 +480,7 @@ static int sec_pmic_acpm_probe(struct platform_device *pdev)
struct regmap *regmap_common, *regmap_pmic, *regmap;
const struct sec_pmic_acpm_platform_data *pdata;
struct sec_pmic_acpm_shared_bus_context *shared_ctx;
- const struct acpm_handle *acpm;
+ struct acpm_handle *acpm;
struct device *dev = &pdev->dev;
int ret, irq;
diff --git a/include/linux/firmware/samsung/exynos-acpm-protocol.h b/include/linux/firmware/samsung/exynos-acpm-protocol.h
index 2091da965a5a..13f17dc4443b 100644
--- a/include/linux/firmware/samsung/exynos-acpm-protocol.h
+++ b/include/linux/firmware/samsung/exynos-acpm-protocol.h
@@ -14,30 +14,24 @@ struct acpm_handle;
struct device_node;
struct acpm_dvfs_ops {
- int (*set_rate)(const struct acpm_handle *handle,
- unsigned int acpm_chan_id, unsigned int clk_id,
- unsigned long rate);
- unsigned long (*get_rate)(const struct acpm_handle *handle,
+ int (*set_rate)(struct acpm_handle *handle, unsigned int acpm_chan_id,
+ unsigned int clk_id, unsigned long rate);
+ unsigned long (*get_rate)(struct acpm_handle *handle,
unsigned int acpm_chan_id,
unsigned int clk_id);
};
struct acpm_pmic_ops {
- int (*read_reg)(const struct acpm_handle *handle,
- unsigned int acpm_chan_id, u8 type, u8 reg, u8 chan,
- u8 *buf);
- int (*bulk_read)(const struct acpm_handle *handle,
- unsigned int acpm_chan_id, u8 type, u8 reg, u8 chan,
- u8 count, u8 *buf);
- int (*write_reg)(const struct acpm_handle *handle,
- unsigned int acpm_chan_id, u8 type, u8 reg, u8 chan,
- u8 value);
- int (*bulk_write)(const struct acpm_handle *handle,
- unsigned int acpm_chan_id, u8 type, u8 reg, u8 chan,
- u8 count, const u8 *buf);
- int (*update_reg)(const struct acpm_handle *handle,
- unsigned int acpm_chan_id, u8 type, u8 reg, u8 chan,
- u8 value, u8 mask);
+ int (*read_reg)(struct acpm_handle *handle, unsigned int acpm_chan_id,
+ u8 type, u8 reg, u8 chan, u8 *buf);
+ int (*bulk_read)(struct acpm_handle *handle, unsigned int acpm_chan_id,
+ u8 type, u8 reg, u8 chan, u8 count, u8 *buf);
+ int (*write_reg)(struct acpm_handle *handle, unsigned int acpm_chan_id,
+ u8 type, u8 reg, u8 chan, u8 value);
+ int (*bulk_write)(struct acpm_handle *handle, unsigned int acpm_chan_id,
+ u8 type, u8 reg, u8 chan, u8 count, const u8 *buf);
+ int (*update_reg)(struct acpm_handle *handle, unsigned int acpm_chan_id,
+ u8 type, u8 reg, u8 chan, u8 value, u8 mask);
};
struct acpm_ops {
@@ -56,12 +50,12 @@ struct acpm_handle {
struct device;
#if IS_ENABLED(CONFIG_EXYNOS_ACPM_PROTOCOL)
-const struct acpm_handle *devm_acpm_get_by_node(struct device *dev,
- struct device_node *np);
+struct acpm_handle *devm_acpm_get_by_node(struct device *dev,
+ struct device_node *np);
#else
-static inline const struct acpm_handle *devm_acpm_get_by_node(struct device *dev,
- struct device_node *np)
+static inline struct acpm_handle *devm_acpm_get_by_node(struct device *dev,
+ struct device_node *np)
{
return NULL;
}
--
2.51.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] firmware: exynos-acpm: Drop fake 'const' on handle pointer
2026-02-24 10:42 [PATCH v2] firmware: exynos-acpm: Drop fake 'const' on handle pointer Krzysztof Kozlowski
@ 2026-02-24 12:57 ` Tudor Ambarus
2026-02-25 10:48 ` Krzysztof Kozlowski
2026-02-28 14:58 ` Krzysztof Kozlowski
1 sibling, 1 reply; 6+ messages in thread
From: Tudor Ambarus @ 2026-02-24 12:57 UTC (permalink / raw)
To: Krzysztof Kozlowski, Krzysztof Kozlowski, Sylwester Nawrocki,
Chanwoo Choi, Alim Akhtar, Michael Turquette, Stephen Boyd,
André Draszik, Lee Jones, linux-kernel, linux-samsung-soc,
linux-clk, linux-arm-kernel
Cc: stable
Hi Krzysztof,
On 2/24/26 12:42 PM, Krzysztof Kozlowski wrote:
> All the functions operating on the 'handle' pointer 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 acpm_info *acpm = handle_to_acpm_info(handle);
>
> And with such cast the handle is easily writable with simple:
>
> acpm->handle.ops.pmic_ops.read_reg = NULL;
>> The code is not correct logically, either, because functions like
> acpm_get_by_node() and acpm_handle_put() are meant to modify the handle
> reference counting, thus they must modify the handle. Modification here
You are right that casting away const via container_of to modify the
parent's reference count is incorrect, so dropping the const from the
handle argument makes sense.
However, to address the underlying issue of the operations being
writable (e.g., acpm->handle.ops.pmic_ops.read_reg = NULL), I think we
should also decouple the ops from the handle struct and keep them strictly
constant in .rodata.
How about we apply your fix for the signatures, and I follow up with
(or we include) a patch to do the following:
struct acpm_handle {
const struct acpm_ops *ops; // Changed from embedded struct to pointer
};
static const struct acpm_ops exynos_acpm_driver_ops = {
.dvfs_ops = {
.set_rate = acpm_dvfs_set_rate,
.get_rate = acpm_dvfs_get_rate,
},
.pmic_ops = {
.read_reg = acpm_pmic_read_reg,
.write_reg = acpm_pmic_write_reg,
// ... other ops
},
};
and in probe:
acpm->handle.ops = &exynos_acpm_driver_ops;
This way, the handle safely reflects the mutability of its container,
but our function pointers remain fully protected.
Cheers,
ta
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] firmware: exynos-acpm: Drop fake 'const' on handle pointer
2026-02-24 12:57 ` Tudor Ambarus
@ 2026-02-25 10:48 ` Krzysztof Kozlowski
2026-02-25 14:00 ` Tudor Ambarus
0 siblings, 1 reply; 6+ messages in thread
From: Krzysztof Kozlowski @ 2026-02-25 10:48 UTC (permalink / raw)
To: Tudor Ambarus, Krzysztof Kozlowski, Sylwester Nawrocki,
Chanwoo Choi, Alim Akhtar, Michael Turquette, Stephen Boyd,
André Draszik, Lee Jones, linux-kernel, linux-samsung-soc,
linux-clk, linux-arm-kernel
Cc: stable
On 24/02/2026 13:57, Tudor Ambarus wrote:
> Hi Krzysztof,
>
> On 2/24/26 12:42 PM, Krzysztof Kozlowski wrote:
>> All the functions operating on the 'handle' pointer 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 acpm_info *acpm = handle_to_acpm_info(handle);
>>
>> And with such cast the handle is easily writable with simple:
>>
>> acpm->handle.ops.pmic_ops.read_reg = NULL;
>>> The code is not correct logically, either, because functions like
>> acpm_get_by_node() and acpm_handle_put() are meant to modify the handle
>> reference counting, thus they must modify the handle. Modification here
>
> You are right that casting away const via container_of to modify the
> parent's reference count is incorrect, so dropping the const from the
> handle argument makes sense.
>
> However, to address the underlying issue of the operations being
> writable (e.g., acpm->handle.ops.pmic_ops.read_reg = NULL), I think we
> should also decouple the ops from the handle struct and keep them strictly
> constant in .rodata.
>
> How about we apply your fix for the signatures, and I follow up with
> (or we include) a patch to do the following:
>
> struct acpm_handle {
> const struct acpm_ops *ops; // Changed from embedded struct to pointer
> };
>
> static const struct acpm_ops exynos_acpm_driver_ops = {
> .dvfs_ops = {
> .set_rate = acpm_dvfs_set_rate,
> .get_rate = acpm_dvfs_get_rate,
> },
> .pmic_ops = {
> .read_reg = acpm_pmic_read_reg,
> .write_reg = acpm_pmic_write_reg,
> // ... other ops
> },
> };
>
> and in probe:
> acpm->handle.ops = &exynos_acpm_driver_ops;
>
> This way, the handle safely reflects the mutability of its container,
> but our function pointers remain fully protected.
Yes, this makes sense.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] firmware: exynos-acpm: Drop fake 'const' on handle pointer
2026-02-25 10:48 ` Krzysztof Kozlowski
@ 2026-02-25 14:00 ` Tudor Ambarus
2026-03-01 12:09 ` Krzysztof Kozlowski
0 siblings, 1 reply; 6+ messages in thread
From: Tudor Ambarus @ 2026-02-25 14:00 UTC (permalink / raw)
To: Krzysztof Kozlowski, Krzysztof Kozlowski, Sylwester Nawrocki,
Chanwoo Choi, Alim Akhtar, Michael Turquette, Stephen Boyd,
André Draszik, Lee Jones, linux-kernel, linux-samsung-soc,
linux-clk, linux-arm-kernel
Cc: stable
On 2/25/26 12:48 PM, Krzysztof Kozlowski wrote:
> On 24/02/2026 13:57, Tudor Ambarus wrote:
>> Hi Krzysztof,
>>
>> On 2/24/26 12:42 PM, Krzysztof Kozlowski wrote:
>>> All the functions operating on the 'handle' pointer 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 acpm_info *acpm = handle_to_acpm_info(handle);
>>>
>>> And with such cast the handle is easily writable with simple:
>>>
>>> acpm->handle.ops.pmic_ops.read_reg = NULL;
>>>> The code is not correct logically, either, because functions like
>>> acpm_get_by_node() and acpm_handle_put() are meant to modify the handle
>>> reference counting, thus they must modify the handle. Modification here
>>
>> You are right that casting away const via container_of to modify the
>> parent's reference count is incorrect, so dropping the const from the
>> handle argument makes sense.
>>
>> However, to address the underlying issue of the operations being
>> writable (e.g., acpm->handle.ops.pmic_ops.read_reg = NULL), I think we
>> should also decouple the ops from the handle struct and keep them strictly
>> constant in .rodata.
>>
>> How about we apply your fix for the signatures, and I follow up with
>> (or we include) a patch to do the following:
>>
>> struct acpm_handle {
>> const struct acpm_ops *ops; // Changed from embedded struct to pointer
>> };
>>
>> static const struct acpm_ops exynos_acpm_driver_ops = {
>> .dvfs_ops = {
>> .set_rate = acpm_dvfs_set_rate,
>> .get_rate = acpm_dvfs_get_rate,
>> },
>> .pmic_ops = {
>> .read_reg = acpm_pmic_read_reg,
>> .write_reg = acpm_pmic_write_reg,
>> // ... other ops
>> },
>> };
>>
>> and in probe:
>> acpm->handle.ops = &exynos_acpm_driver_ops;
>>
>> This way, the handle safely reflects the mutability of its container,
>> but our function pointers remain fully protected.
>
> Yes, this makes sense.
>
Will you come with a follow up patch or do you want me to ACK this and do
the follow up patch myself? Both options are fine by mine.
Thanks!
ta
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] firmware: exynos-acpm: Drop fake 'const' on handle pointer
2026-02-24 10:42 [PATCH v2] firmware: exynos-acpm: Drop fake 'const' on handle pointer Krzysztof Kozlowski
2026-02-24 12:57 ` Tudor Ambarus
@ 2026-02-28 14:58 ` Krzysztof Kozlowski
1 sibling, 0 replies; 6+ messages in thread
From: Krzysztof Kozlowski @ 2026-02-28 14:58 UTC (permalink / raw)
To: Tudor Ambarus, Sylwester Nawrocki, Chanwoo Choi, Alim Akhtar,
Michael Turquette, Stephen Boyd, André Draszik, Lee Jones,
linux-kernel, linux-samsung-soc, linux-clk, linux-arm-kernel,
Krzysztof Kozlowski
Cc: stable
On Tue, 24 Feb 2026 11:42:04 +0100, Krzysztof Kozlowski wrote:
> All the functions operating on the 'handle' pointer 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 acpm_info *acpm = handle_to_acpm_info(handle);
>
> [...]
Applied, thanks!
[1/1] firmware: exynos-acpm: Drop fake 'const' on handle pointer
https://git.kernel.org/krzk/linux/c/a2be37eedb52ea26938fa4cc9de1ff84963c57ad
Best regards,
--
Krzysztof Kozlowski <krzk@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] firmware: exynos-acpm: Drop fake 'const' on handle pointer
2026-02-25 14:00 ` Tudor Ambarus
@ 2026-03-01 12:09 ` Krzysztof Kozlowski
0 siblings, 0 replies; 6+ messages in thread
From: Krzysztof Kozlowski @ 2026-03-01 12:09 UTC (permalink / raw)
To: Tudor Ambarus, Krzysztof Kozlowski, Sylwester Nawrocki,
Chanwoo Choi, Alim Akhtar, Michael Turquette, Stephen Boyd,
André Draszik, Lee Jones, linux-kernel, linux-samsung-soc,
linux-clk, linux-arm-kernel
Cc: stable
On 25/02/2026 15:00, Tudor Ambarus wrote:
>>> static const struct acpm_ops exynos_acpm_driver_ops = {
>>> .dvfs_ops = {
>>> .set_rate = acpm_dvfs_set_rate,
>>> .get_rate = acpm_dvfs_get_rate,
>>> },
>>> .pmic_ops = {
>>> .read_reg = acpm_pmic_read_reg,
>>> .write_reg = acpm_pmic_write_reg,
>>> // ... other ops
>>> },
>>> };
>>>
>>> and in probe:
>>> acpm->handle.ops = &exynos_acpm_driver_ops;
>>>
>>> This way, the handle safely reflects the mutability of its container,
>>> but our function pointers remain fully protected.
>>
>> Yes, this makes sense.
>>
>
> Will you come with a follow up patch or do you want me to ACK this and do
> the follow up patch myself? Both options are fine by mine.
Go ahead with implementing this idea. I cannot test it anyway, so better
if you do it.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-03-01 12:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-02-24 10:42 [PATCH v2] firmware: exynos-acpm: Drop fake 'const' on handle pointer Krzysztof Kozlowski
2026-02-24 12:57 ` Tudor Ambarus
2026-02-25 10:48 ` Krzysztof Kozlowski
2026-02-25 14:00 ` Tudor Ambarus
2026-03-01 12:09 ` Krzysztof Kozlowski
2026-02-28 14:58 ` Krzysztof Kozlowski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox