Re: [PATCH v3 1/7] rust: firmware: add request_into_buf()

On Tue Jul 7, 2026 at 7:10 AM CEST, Alexandre Courbot wrote:
> On Tue Jul 7, 2026 at 11:54 AM JST, Timur Tabi wrote:
>> On Mon, 2026-07-06 at 12:50 +0200, Danilo Krummrich wrote:
>>> On Mon Jul 6, 2026 at 8:30 AM CEST, Alexandre Courbot wrote:
>>> > Sashiko is correct to point out that this doesn't return the number of
>>> > bytes actually written into `buf`. We might not use that information in
>>> > nova-core, but this is a kernel-wide API.
>>> 
>>> Returning the size only doesn't carry a lot of value, please see [1].
>>> 
>>> > Returning a `Result<&[u8]>` would take care of this and cover the
>>> > general use-case nicely.
>>> 
>>> I'm fine with either this or just Result, as we don't have a user for the
>>> former.
>>> 
>>> [1] https://lore.kernel.org/nova-gpu/[email protected]/
>>
>> Guys, please come to a consensus.  I had it return a size originally, but then Danilo said don't
>> bother, and now Alex says I should return a size.

I said that both are fine for now, just Result or the slice, but only the size
doesn't serve a purpose (see [1] above or the reasoning below).

> Incorrect; I said you should return a slice. :)
>
> What makes me a bit nervous with this API is that it is not sound
> against incorrect size information or race conditions (e.g. if the file
> on disk changes after we determined its size). But that's true of my
> proposal as well anyway.

There's nothing we can do about this, the file could also be corrupted, etc. In
the end there are only two cases, the hardware can't deal with garbage, then the
size-sanity check is not sufficient and we need proper validation (e.g. through
the returned slice), or the hardware can deal with garbage, then the size-sanity
check isn't necessary.

> Another inefficiency is that the buffer must be initialized when passed,
> only for its content to be immediately overwritten. I'd like to make
> this function accept a `MaybeUninit`, but that will require more generic
> code.

This is true, with the current approach we need something like

	let buf = VBox::<[u8; SIZE]>::zeroed(GFP_KERNEL)?;

which isn't the end of the world, especially in the firmware loading path, but I
agree it is a bit unfortunate.

We could indeed get rid of the initialization requirement entirely, but in this
case just returning a slice isn't sufficient, as it leaves the caller with e.g.
a VBox<MaybeUninit<[u8; SIZE]>> for storage, which is annoying if you want to
access the buffer subsequently, e.g. to parse headers, etc.

So, for this to properly work, you'd need to take the container (Vec, Box,
CoherentBox) by value and return the initialized one on success and the
uninitialized one wrapped into a custom error type on failure. But this is
probably a bit overengineered.

A simpler option would be to just use an initializer, such that
request_into_buf() becomes something like this:

	pub fn request_firmware_init<'a>(
	    name: &'a CStr,
	    dev: &'a Device,
	) -> impl Init<[u8], Error> + 'a {
	    unsafe {
	        init_from_closure(move |slot: *mut [u8]| {
	            // call request_firmware_into_buf()
	            Ok(())
	        })
	    }
	}

And then allocate the buffer with

	let init = request_firmware_init(...);
	let fw = VVec::init_with(len, init, GFP_KERNEL)?;

In any case, the current approach is fine for this series, this can be a
follow-up if needed.

The only thing that'd be nice to replace would be

	VVec::from_elem(0u8, size, GFP_KERNEL)

with

	VVec::<u8>::zeroed(size, GFP_KERNEL)

analogous to Box::zeroed().