Re: [PATCH] rust: io: use const generics for read/write offsets

[Joel Fernandes]

Hello, Danilo,

> On Sep 19, 2025, at 1:26 AM, Danilo Krummrich <dakr@xxxxxxxxxx> wrote:
> 
> On Thu Sep 18, 2025 at 8:13 PM CEST, Joel Fernandes wrote:
>>> On Thu, Sep 18, 2025 at 03:02:11PM +0000, Alice Ryhl wrote:
>>> Using build_assert! to assert that offsets are in bounds is really
>>> fragile and likely to result in spurious and hard-to-debug build
>>> failures. Therefore, build_assert! should be avoided for this case.
>>> Thus, update the code to perform the check in const evaluation instead.
>> 
>> I really don't think this patch is a good idea (and nobody I spoke to thinks
>> so). Not only does it mess up the user's caller syntax completely, it is also
> 
> I appreacite you raising the concern,
> but I rather have other people speak up
> themselves.

I did not mean to speak for others, sorry it came across like that (and that is certainly not what I normally do). But I discussed the patch in person since we are at a conference and discussing it in person, and I did not get a lot of consensus on this. That is what I was trying to say. If it was a brilliant or great idea, I would have hoped for at least one person to tell me that this is exactly how we should do it.

> 
>> super confusing to pass both a generic and a function argument separately.
> 
> Why? We assert that the offset has to be const, whereas the value does not
> have this requirement, so this makes perfect sense to me.
> 
> (I agree that it can look unfamiliar at the first glance though.)

Yes the familiarity is the issue. I still do not feel using a generic for this looks ok to me and I think we can fix it differently, I will try to come up with an alternative fix unless we have already decided to use generics for this.

> 
>> Sorry if I knew this would be the syntax, I would have objected even when we
>> spoke :)
>> 
>> I think the best fix (from any I've seen so far), is to move the bindings
>> calls of offending code into a closure and call the closure directly, as I
>> posted in the other thread. I also passed the closure idea by Gary and he
>> confirmed the compiler should behave correctly (I will check the code gen
>> with/without later). Gary also provided a brilliant suggestion that we can
>> call the closure directly instead of assigning it to a variable first. That
>> fix is also smaller, and does not screw up the users. APIs should fix issues
>> within them instead of relying on user to work around them.
> 
> This is not a workaround, this is an idiomatic solution (which I probably should
> have been doing already when I introduced the I/O code).
> 
> We do exactly the same thing for DmaMask::new() [1] and we agreed on doing the
> same thing for Alignment as well [2].
> 
> [1] https://rust.docs.kernel.org/kernel/dma/struct.DmaMask.html#method.new
> [2] https://lore.kernel.org/rust-for-linux/20250908-num-v5-1-c0f2f681ea96@xxxxxxxxxx/

Ah ok. Since there is precedent, I am ok with it, especially since you feel so strongly about it and since you are the rust IO code maintainer.

But passing one parameter as a constant generic and another parameter as a function parameter, just seems weird to me even if there is precedent.

And one can argue that the value is also a constant right? It is confusing and makes callers unreadable IMHO.

Cheers,

 - Joel