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>

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

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

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