Re: [edk2-devel] [edk2-platforms][PATCH v2] ManageabilityPkg: add support for the phosphor ipmi blob transfer protocol

["Nhi Pham via groups.io" <nhi=os.amperecomputing.com@groups.io>]

On 7/10/2024 10:12 PM, Nickle Wang wrote:
> Hi @Nhi Pham <mailto:nhi@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-