Re: [PATCH v4 19/20] gpu: nova-core: extract FWSEC from BIOS and patch it to run FWSEC-FRTS

["Alexandre Courbot" <acourbot@nvidia.com>]

On Wed Jun 4, 2025 at 7:42 PM JST, Danilo Krummrich wrote:
> On Wed, May 21, 2025 at 03:45:14PM +0900, Alexandre Courbot wrote:
>> +impl FirmwareDmaObject<FwsecFirmware> {
>> +    /// Patch the Fwsec firmware image in `fw` to run the command `cmd`.
>> +    fn patch_command(&mut self, v3_desc: &FalconUCodeDescV3, cmd: FwsecCommand) -> Result<()> {
>
> Same comment as on the previous patch regarding patch_signature().

This one can be easily fixed, contrary to the previous one. The
constructor now takes the command to patch and does it here (as it makes
no sense to run FWSEC without the command patched in).

>
> <snip>
>
>> +    fn dmem_load_params(&self) -> FalconLoadTarget {
>> +        FalconLoadTarget {
>> +            src_start: self.desc.imem_load_size,
>> +            dst_start: self.desc.dmem_phys_base,
>> +            len: Layout::from_size_align(self.desc.dmem_load_size as usize, 256)
>> +                // Cannot panic, as 256 is non-zero and a power of 2.
>> +                .unwrap()
>
> There is also Layout::from_size_align_unchecked(), which I prefer over unwrap().
> I think we should never use unwrap() and rather the unsafe variant, which at least
> forces us to document things properly, if there's no other option.
>
> In this case, however, I don't see why we can't just propage the error? This
> method is used from Falcon::dma_load(), which returns a Result anyways, so let's
> just propagate it.
>
> In general, we should *never* potentially panic the whole kernel just because
> of a wrong size calculation in a driver.

Good point. In this case we can do something even simpler, which is
use the `align_up` method introduced in the `num` module.