On 7/10/2024 10:12 PM, Nickle Wang wrote:
Hi @Nhi Pham <mailto:n...@os.amperecomputing.com>,
I am sorry for taking so long to address your review comments. I updated
pull request here: https://github.com/tianocore/edk2-platforms/pull/76
<https://github.com/tianocore/edk2-platforms/pull/76>
Thanks, Nickle. I will continue reviewing (if any comments) on the PR
instead.
I addressed most of comments and I have questions about below review
comments. Please find my feedback inline below.
> Should we add a PCD for the OEN to be configured by platform specific
> BMC? Or this protocol is only to support OpenBMC.
Yes, per description in Readme.md, this is the protocol only to support
OpenBMC implementation.
> I'm thinking the caller sequence here. Typically, the caller might check
> the presence of a blob by calling GetCount () and Enumerate () before
> opening a blob session. This check here could waste time. Or, do we call
> open direction the blob session without pre-checking?
With this implementation, user can call open directly and it will check
the presence of blob for us. And yes, this is how we use it in NVIDIA
driver.
> There might be developer mistake when executing the transfer before
> opening the session. How do we handle this failure path? Do we need to
> maintain a state machine for that?
Caller can only get the session ID by calling open(). Session ID is
required when calling read, write, and commit. If caller make up a
session ID, caller is expected to see error return.
> How do callers know the data format of metadata for writing correctly?
I check the spec here:
https://github.com/openbmc/phosphor-ipmi-blobs?tab=readme-ov-file#bmcblobwritemeta-10 <https://github.com/openbmc/phosphor-ipmi-blobs?tab=readme-ov-file#bmcblobwritemeta-10> And there is no description about data format. Since we don't use this function in our driver, may I know if you have suggestion for this?
I don't have a strong opinion here (**That was just my curiosity**). So,
we can just leave it as your implementation or drop it as there is no
consumer.
> +IpmiBlobTransferDxeDriverEntryPoint (
> + IN EFI_HANDLE ImageHandle,
> + IN EFI_SYSTEM_TABLE *SystemTable
> + )
> +{
> + return gBS->InstallMultipleProtocolInterfaces (
> + &ImageHandle,
> Nit: Typically, we could also use gImageHandle instead.
Sorry I don't follow you here.
gImageHandle is used as global variable in driver. But here we just use
the ImageHandle from function parameter and we don't keep it as global
variable, right?
The pointers should be identical since they are cached by the
UefiBootServicesTableLib's constructor prior to the call to
IpmiBlobTransferDxeDriverEntryPoint.
Thanks,
Nhi
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119879): https://edk2.groups.io/g/devel/message/119879
Mute This Topic: https://groups.io/mt/106115743/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
