Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MdeModulePkg/DxeIpmiLibIpmiProtocol: Add depex #6538

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

MdeModulePkg/DxeIpmiLibIpmiProtocol: Add depex #6538

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion MdeModulePkg/Library/DxeIpmiLibIpmiProtocol/DxeIpmiLibIpmiProtocol.inf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,7 @@
DebugLib

[Protocols]
gIpmiProtocolGuid ## SOMETIMES_CONSUMES
gIpmiProtocolGuid ## CONSUMES

[Depex]
gIpmiProtocolGuid
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is made an unconditional dependency, then the lookup of the protocol should be done in a CONSTRUCTOR and IpmiSubmitCommand() can be simplified to assume mIpmiProtocol is always valid.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mdkinney, thanks for your reviewing.

The DxeIpmiLibIpmiProtocol always consumes the gIpmiProtocolGuid protocol, and declaring it in the [Depex] section ensures that the library consumers execute only after the protocol is available. Without this, there is no guarantee of proper driver execution order.

Regarding your suggestion to initialize mIpmiProtocol in a LIBRARY_CONSTRUCTOR, I agree this approach would simplify IpmiSubmitCommand() by removing the need for runtime protocol lookup. However, the current design expects the protocol to be available at the time of library use, and if it is not, calls to IpmiSubmitCommand() will fail as designed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you add to [Depex], then the protocol is guaranteed to be present when the entry point of this module is executed and CONSTRUCTOR services are executed. So looking up time of use is not required if it is added to [Depex]. If it is not added to [Depex], then the protocol will be checked on each use and can fail if the protocol is not present yet. If platforms depend on lazy evaluation of this protocol being present, then this change can not be accepted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mdkinney for the clarification.

The check for mIpmiProtocol == NULL in the IpmiSubmitCommand () function is intended to perform a one-time lookup of the gIpmiProtocolGuid protocol. Once the protocol is located, mIpmiProtocol is cached for subsequent use, ensuring minimal overhead after the initial lookup.

If it is not added to [Depex], then the protocol will be checked on each use and can fail if the protocol is not present yet.

The primary intent of this change is to ensure that the DxeIpmiLibIpmiProtocol library consumes the gIpmiProtocolGuid protocol reliably. Adding the protocol to the [Depex] section consistently enforces proper driver execution order and guarantees the protocol is available when the consumer's DXE Entry Point is executed.

If platforms depend on lazy evaluation of this protocol being present, then this change can not be accepted.

I'm unsure if I understand correctly. Adding the gIpmiProtocolGuid protocol to the [Depex] section of individual consumer drivers could address the issue. However, this change aims to simplify dependency management by moving the responsibility to the DxeIpmiLibIpmiProtocol library itself.

By declaring the protocol in the library’s [Depex] section:

  1. Existing and future consumers of the library are relieved of the need to explicitly handle the gIpmiProtocolGuid dependency.

  2. The library fully abstracts the protocol dependency, adhering to the principle of encapsulation and ensuring a cleaner, more maintainable design.

Additionally, this change ensures consistent behavior across all consumers without requiring them to have knowledge of the dependent protocol.

Loading