On 5/20/2025 5:30 AM, Danilo Krummrich wrote:
> On Tue, May 20, 2025 at 03:55:06AM -0400, Joel Fernandes wrote:
>> On 5/13/2025 1:19 PM, Danilo Krummrich wrote:
>>> On Wed, May 07, 2025 at 10:52:43PM +0900, Alexandre Courbot wrote:
>>>> @@ -238,6 +239,8 @@ pub(crate) fn new(
>>>>
>>>> let _sec2_falcon = Falcon::<Sec2>::new(pdev.as_ref(),
>>>> spec.chipset, bar, true)?;
>>>>
>>>> + let _bios = Vbios::new(pdev, bar)?;
>>>
>>> Please add a comment why, even though unused, it is important to create this
>>> instance.
>>>
>>> Also, please use `_` if it's not intended to ever be used.
>>
>> If I add a comment, it will simply be removed by the next patch. I can add
>> that
>> though so it makes it more clear.
>
> I recommend to add such comments, because then reviewers don't stumble over
> it.
> :-)
Point taken and fixed! ;-)
>>>
>>>> + pdev.as_ref(),
>>>> + "Found BIOS image: size: {:#x}, type: {}, last: {}\n",
>>>> + full_image.image_size_bytes()?,
>>>> + full_image.image_type_str(),
>>>> + full_image.is_last()
>>>> + );
>>>> +
>>>> + // Get references to images we will need after the loop, in
>>>> order to
>>>> + // setup the falcon data offset.
>>>> + match full_image {
>>>> + BiosImage::PciAt(image) => {
>>>> + pci_at_image = Some(image);
>>>> + }
>>>> + BiosImage::FwSec(image) => {
>>>> + if first_fwsec_image.is_none() {
>>>> + first_fwsec_image = Some(image);
>>>> + } else {
>>>> + second_fwsec_image = Some(image);
>>>> + }
>>>> + }
>>>> + // For now we don't need to handle these
>>>> + BiosImage::Efi(_image) => {}
>>>> + BiosImage::Nbsi(_image) => {}
>>>> + }
>>>> + }
>>>> +
>>>> + // Using all the images, setup the falcon data pointer in Fwsec.
>>>> + // We need mutable access here, so we handle the Option manually.
>>>> + let final_fwsec_image = {
>>>> + let mut second = second_fwsec_image; // Take ownership of the
>>>> option
>>>> +
>>>> + if let (Some(second), Some(first), Some(pci_at)) =
>>>> + (second.as_mut(), first_fwsec_image, pci_at_image)
>>>> + {
>>>> + second
>>>> + .setup_falcon_data(pdev, &pci_at, &first)
>>>> + .inspect_err(|e| {
>>>> + dev_err!(pdev.as_ref(), "Falcon data setup
>>>> failed: {:?}\n", e)
>>>> + })?;
>>>> + } else {
>>>> + dev_err!(
>>>> + pdev.as_ref(),
>>>> + "Missing required images for falcon data setup,
>>>> skipping\n"
>>>> + );
>>>> + return Err(EINVAL);
>>>
>>> This means that if second == None we fail, which makes sense, so why store
>>> an
>>> Option in Vbios? All methods of Vbios fail if fwsec_image == None.
>>>
>>
>> Well, if first and pci_at are None, we will fail as well. Not just second.
>> But
>> we don't know until we finish parsing all the images in the prior loop, if we
>> found all the images. So we store it as Option during the prior loop, and
>> check
>> it later. Right?
>
> My point is not that second is an option within this function -- that's fine.
> I
> don't want the Vbios type to store an Option, because that doesn't make sense.
> I.e. it should be
>
> struct Vbios {
> fwsec_image: FwSecBiosImage,
> }
>
> or just
>
> struct Vbios(FwSecBiosImage);
>
> which is the same, rather than
>
> struct Vbios {
> fwsec_image: Option<FwSecBiosImage>,
> }
>
> because Vbios::new() fails anyways if any of the images is None, i.e.
> vbios.fwsec_image can't ever be None.
>
> The code below does that for you, i.e. it returns an instance of Vbios without
> the inner Option.
But your code below does Vbios(second) where Vbios is an option..
>
>>>> + }
>>>> + second
>>>> + };
>>>
>>> I think this should be:
>>>
>>> let mut second = second_fwsec_image;
>>>
>>> if let (Some(second), Some(first), Some(pci_at)) =
>>> (second.as_mut(), first_fwsec_image, pci_at_image)
>>> {
>>> second
>>> .setup_falcon_data(pdev, &pci_at, &first)
>>> .inspect_err(|e| {
>>> dev_err!(pdev.as_ref(), "Falcon data setup failed: {:?}\n",
>>> e)
>>> })?;
>>>
>>> Ok(Vbios(second)
I can't do that become second is a mutable reference in the above snippet.
But this works:
Ok(Vbios { fwsec_image: second.take().ok_or(EINVAL)? })
(This did require changing 'Some(second)' to 'Some(second_ref)', see below.)
>>> } else {
>>> dev_err!(
>>> pdev.as_ref(),
>>> "Missing required images for falcon data setup, skipping\n"
>>> );
>>>
>>> Err(EINVAL)
>>> }
>>>
>>> where Vbios can just be
>>>
>>> pub(crate) struct Vbios(FwSecBiosImage);
>>
>> But your suggestion here still considers second as an Option? That's why you
>> wrote 'Some(second)' ?
>
> Yes, that's fine, see above. The difference is that the code returns you an
> instance of
>
> struct Vbios(FwSecBiosImage);
>
> rather than
>
> struct Vbios {
> fwsec_image: Option<FwSecBiosImage>,
> }
>
> which is unnecessary.
Sure, ok, yeah I made this change in another thread we are discussing so we are
good.
So the code here now looks like the below, definitely better, thanks! :
if let (Some(second_ref), Some(first), Some(pci_at)) =
(second.as_mut(), first_fwsec_image, pci_at_image)
{
second_ref
.setup_falcon_data(pdev, &pci_at, &first)
.inspect_err(|e| {
dev_err!(..)
})?;
Ok(Vbios { fwsec_image: second.take().ok_or(EINVAL)? })
} else {
dev_err!(
pdev.as_ref(),
"Missing required images for falcon data setup, skipping\n"
);
Err(EINVAL)
}
>>>> +
>>>> + Ok(Vbios {
>>>> + fwsec_image: final_fwsec_image,
>>>> + })
>>>> + }
>>>> +
>>>> + pub(crate) fn fwsec_header(&self, pdev: &device::Device) ->
>>>> Result<&FalconUCodeDescV3> {
>>>> + let image = self.fwsec_image.as_ref().ok_or(EINVAL)?;
>>>> + image.fwsec_header(pdev)
>>>> + }
>>>> +
>>>> + pub(crate) fn fwsec_ucode(&self, pdev: &device::Device) ->
>>>> Result<&[u8]> {
>>>> + let image = self.fwsec_image.as_ref().ok_or(EINVAL)?;
>>>> + image.fwsec_ucode(pdev, image.fwsec_header(pdev)?)
>>>> + }
>>>> +
>>>> + pub(crate) fn fwsec_sigs(&self, pdev: &device::Device) ->
>>>> Result<&[u8]> {
>>>> + let image = self.fwsec_image.as_ref().ok_or(EINVAL)?;
>>>> + image.fwsec_sigs(pdev, image.fwsec_header(pdev)?)
>>>> + }
>>>
>>> Those then become infallible, e.g.
>>>
>>> pub(crate) fn fwsec_sigs(&self, pdev: &device::Device) -> &[u8] {
>>> self.0.fwsec_sigs(pdev, self.fwsec_header(pdev))
>>> }
>>>
>>
>> Nope, I think you are wrong there. fwsec_sigs() of the underlying .0 returns
>> a
>> Result.
>
> That's true, I confused self.fwsec_sigs() with self.0.fwsec_sigs(). It seems
> that you may want to implement Deref for Vbios.
>
> Also, can you please double check the Options in FwSecBiosImage (in case we
> didn't talk about them yet)? They look quite suspicious too.
> In general, I feel like a lot of those Option come from a programming pattern
> that is very common in C, i.e. allocate a structure (stack or heap) and then
> initialize its fields.
>
> In Rust you should aim to initialize all the fields of a structure when you
> create the instance. Option as a return type of a function is common, but it's
> always a bit suspicious when there is an Option field in a struct.
I looked into it, I could not git rid of those ones because we need to
initialize in the "impl TryFrom<BiosImageBase> for BiosImage {"
0xE0 => Ok(BiosImage::FwSec(FwSecBiosImage {
base,
falcon_data_offset: None,
pmu_lookup_table: None,
falcon_ucode_offset: None,
})),
And these fields will not be determined until much later, because as is the case
with the earlier example, these fields cannot be determined until all the images
are parsed.
> I understand that there are cases where we can't omit it, and for obvious
> reasons the Vbios code is probably a perfect example for that.
>
> However, I recommend looking at this from top to bottom: Do the "final"
> structures that we expose to the driver from the Vbios module have fields that
> are *really* optional? Or is the Option type just a result from the parsing
> process?
>
> If it's the latter, we should get rid of it and work with a different type
> during the parsing process and then create the final instance that is exposed
> to
> the driver at the end.
>
> For instance FwSecBiosImage is defined as:
>
> pub(crate) struct FwSecBiosImage {
> base: BiosImageBase,
> falcon_data_offset: Option<usize>,
> pmu_lookup_table: Option<PmuLookupTable>,
> falcon_ucode_offset: Option<usize>,
> }
>
> Do only *some* FwSecBiosImage instances have a falcon_ucode_offset?
>
> If the answer is 'no' then it shouldn't be an Option. If the answer is 'yes',
> then this indicates that FwSecBiosImage is probably too generic and should be
> split into more specific types of a FwSecBiosImage which instead share a
> common
> trait in order to treat the different types generically.
Understood, thanks.
>> Also in Vbios::new(), I extract the Option when returning:
>>
>> Ok(Vbios {
>> fwsec_image: final_fwsec_image.ok_or(EINVAL)?,
>> })
>
> Maybe you do so in your tree? v3 of the patch series has:
>
> pub(crate) struct Vbios {
> pub fwsec_image: Option<FwSecBiosImage>,
> }
>
> and
>
> Ok(Vbios {
> fwsec_image: final_fwsec_image,
> })
Yes, I made the change during our review on the other thread and will be posted
in the next posting. Sorry for any confusion.
thanks,
- Joel
