"Benno Lossin" <los...@kernel.org> writes:
> On Tue Jul 1, 2025 at 10:43 AM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <los...@kernel.org> writes:
>>> On Mon Jun 30, 2025 at 3:15 PM CEST, Andreas Hindborg wrote:
>>>> "Benno Lossin" <los...@kernel.org> writes:
>>>>> On Mon Jun 30, 2025 at 1:18 PM CEST, Andreas Hindborg wrote:
>>>>>> "Benno Lossin" <los...@kernel.org> writes:
>>>>>>> (no idea if the orderings are correct, I always have to think way to
>>>>>>> much about that... especially since our atomics seem to only take one
>>>>>>> ordering in compare_exchange?)
>>>>>>>
>>>>>>>> As far as I can tell, atomics may not land in v6.17, so this series
>>>>>>>> will probably not be ready for merge until v6.18 at the earliest.
>>>>>>>
>>>>>>> Yeah, sorry about that :(
>>>>>>
>>>>>> Actually, perhaps we could aim at merging this code without this
>>>>>> synchronization?
>>>>>
>>>>> I won't remember this issue in a few weeks and I fear that it will just
>>>>> get buried. In fact, I already had to re-read now what the actual issue
>>>>> was...
>>>>>
>>>>>> The lack of synchronization is only a problem if we
>>>>>> support custom parsing. This patch set does not allow custom parsing
>>>>>> code, so it does not suffer this issue.
>>>>>
>>>>> ... In doing that, I saw my original example of UB:
>>>>>
>>>>> module! {
>>>>> // ...
>>>>> params: {
>>>>> my_param: i64 {
>>>>> default: 0,
>>>>> description: "",
>>>>> },
>>>>> },
>>>>> }
>>>>>
>>>>> static BAD: &'static i64 = module_parameters::my_param.get();
>>>>>
>>>>> That can happen without custom parsing, so it's still a problem...
>>>>
>>>> Ah, got it. Thanks.
>>>
>>> On second thought, we *could* just make the accessor function `unsafe`.
>>> Of course with a pinky promise to make the implementation safe once
>>> atomics land. But I think if it helps you get your driver faster along,
>>> then we should do it.
>>
>> No, I am OK for now with configfs.
>>
>> But, progress is still great. How about if we add a copy accessor
>> instead for now, I think you proposed that a few million emails ago:
>>
>> pub fn get(&self) -> T;
>>
>> or maybe rename:
>>
>> pub fn copy(&self) -> T;
>>
>> Then we are fine safety wise for now, right? It is even sensible for
>> these `T: Copy` types.
>
> That is better than getting a reference, but still someone could read at
> the same time that a write is happening (though we need some new
> abstractions AFAIK?). But I fear that we forget about this issue,
> because it'll be some time until we land parameters that are `!Copy` (if
> at all...)
No, that could not happen when we are not allowing custom parsing or
sysfs access. Regarding forgetting, I already added a `NOTE` on `!Copy`,
and I would add one on this issue as well.
Best regards,
Andreas Hindborg
