Re: [PATCH] ti: sci: Drop fake 'const' on handle pointer

[Andrew Davis <afd@ti.com>]

On 3/5/26 1:49 PM, Krzysztof Kozlowski wrote:
> On 05/03/2026 19:44, Andrew Davis wrote:
>> On 3/5/26 9:59 AM, Krzysztof Kozlowski wrote:
>>> On 05/03/2026 16:52, Andrew Davis wrote:
>>>>>>> The code is not correct logically, either, because functions like
>>>>>>> ti_sci_get_handle() and ti_sci_put_handle() are meant to modify the
>>>>>>> handle reference counting, thus they must modify the handle.
>>>>>>
>>>>>> The reference counting is handled outside of the ti_sci_handle struct,
>>>>>> the contents of the handle are never modified after it is created.
>>>>>>
>>>>>> The const is only added by functions return a handle to consumers.
>>>>>> We cannot return non-const to consumer drivers or then they would
>>>>>> be able to modify the content without a compiler warning, which would
>>>>>> be a real problem.
>>>>>
>>>>> This is the same argument as making pointer to const the pointer freed
>>>>> via kfree() (or free() in userspace). kfree() does not modify the
>>>>> contents of the pointer, right? The same as getting putting handle does
>>>>> not modify the handle...
>>>>>
>>>>
>>>> In that argument, if we wanted the consumer of the pointer to not free()
>>>> it we would return a const pointer, free()'ing that would result in the
>>>> warning we want (discards const qualifier).
>>>>
>>>> If you could somehow malloc() from a const area in memory then free()
>>>> doesn't modify the pointed to values, only the non-const record keeping
>>>> which would be stored outside of the const memory. So even in this analogy
>>>> there isn't a problem.
>>>
>>> I am not saying about malloc. I am saying about free() which does not
>>> modify the freed memory.
>>>
>>
>> And if you look, kfree() in Linux takes a const pointer. We also do not
> 
> The slub, but that's the only implementation being I believe frowned
> upon. The mistake made long time ago...
> 
>> modify the content of the pointer we are given either, so we should
>> be okay using const by the same reasoning.
> 
> That's a mistake so you cannot use the same reasoning. It's bogus and
> bugfree to take a pointer to const for any kfree(). Just poke MM folks...
> 

Don't act like I'm trying to trick you by picking some bad example, you
picked kfree() and it just happened to showcase my point.

> 
>>
>>>>
>>>>> The point is that storing the reference counter outside of handle does
>>>>> not make the argument correct. Logically when you get a reference, you
>>>>> increase the counter, so it is not a pointer to const. And the code
>>>>> agrees, because you must drop the const.
>>>>>
>>>>
>>>> The record keeping memory is not const and can be modified.
>>>>
>>>> And where do we drop the const? The outer "struct ti_sci_info" was never
>>>> const to begin with, so no dropped const.
>>>
>>> We discuss about different points. I did not say the outer memory is
>>> const. I said that you drop the const - EXPLICITLY - from the pointer to
>>> handle.
>>>
>>
>> Only because container_of() forces the const to be dropped, that is out
>> of our control. But we never modify handle though the non-const parent
>> struct.
> 
> That is not true. You could use container_of_const() if you wanted to
> have const. You explicitly drop the const, code would not work without
> dropping the const and this is the problem.
> 

There is no dropping the const, the parent struct was never const in
the first place. Only the handle inside the parent struct is const and
it can stay const and nothing would stop working in any API function.

>>
>>> And that API which gets a handle (increases reference count) via pointer
>>> to const is completely illogical, because increasing refcnt is already
>>> modifying it. Just because you store the refcnt outside, does not change
>>> the fact that API is simply confusing.
>>>
>>
>> If the refcnt is not inside the const struct, then the contents are not
>> changed, therefor const is still correct. Even if the content of handle
>> were in fixed ROM, nothing would break here.
> 
> I am talking about API and again you go into memory correctness. So
> again, very simple: any refcnt get taking const data is bogus.
> 

Modifying a refcnt stored *inside* a const struct is bogus. Keeping an
external refcnt about a bit of const data is perfectly sane. When the
refcnt goes to 0 nothing happens, it's not like we go an free() the
const data or anything. The refcnt exists just to print a warning if the
count goes negative, nothing else.

Andrew

> 
> Best regards,
> Krzysztof