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.

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

[sonic_platform_base/chassis_base]: Add firmware management interface #34

Merged
merged 5 commits into from
Jul 1, 2019

Conversation

mudsut4ke
Copy link
Contributor

@mudsut4ke mudsut4ke commented Jun 14, 2019

Add base class for firmware management API
(sonic-net/sonic-buildimage#3013)

Signed-off-by: Wirut Getbamrung wgetbumr@celestica.com

Copy link
Contributor

@jleveque jleveque left a comment

Choose a reason for hiding this comment

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

These new functions will need to be added to ChassisBase in addition to ModuleBase.

In SONiC platform API terminology, a "Module" is a removable device in a modular chassis, such as a line card, supervisor card, etc.).

Also, ChassisBase already has a function, get_component_versions() to retrieve all platform-specific firmware versions (the number of which could vary between platforms: BIOS, CPLD, FPGA, etc.). How do you see these functions working with a platform which has multiple components which require firmware?

@mudsut4ke
Copy link
Contributor Author

mudsut4ke commented Jun 17, 2019

Sorry, maybe I don't understand the purpose of your suggestion in #30 for ModuleBase

To move it to ChassisBase and working with a platform which has multiple components
Can I add update get_component_versions() to support component type ?


    def get_component_versions(self, type):
        """
        Retrieves platform-specific hardware/firmware versions for chassis
        componenets such as BIOS, CPLD, FPGA, etc.
        Args:
            type: A string, component type

        Returns:
            A string containing platform-specific component versions
        """
        raise NotImplementedError


    def install_component_firmware(self, type, image_path)
        """
        Install firmware to module
        Args:
            type: A string, component type.
            image_path: A string, path to firmware image.

        Returns:
            A boolean, True if install successfully, False if not
        """
        raise NotImplementedError

@jleveque
Copy link
Contributor

Another option is to replace get_component_versions() with get_firmware_version(), and the latter could take a component name as a parameter, e.g., get_firmware_version(self, component). Then the definition of install_component_firmware() could be updated to match, e.g., install_component_firmware(self, component, image_path). The component type strings should also be defined as constants.

@mudsut4ke
Copy link
Contributor Author

OK, I think so.

  1. Remove get_component_versions()
  2. Add get_firmware_version(self, component)
  3. Add install_component_firmware(self, component, image_path)

but for component type , is that necessary to defined as constants ?
I think just use component name is enough.

@jleveque
Copy link
Contributor

jleveque commented Jun 18, 2019

but for component type , is that necessary to defined as constants ?
I think just use component name is enough.

I guess they don't have to be defined as constants in the base class, as they will only be used by the vendor. This will allow for vendors to define their own component names, and allows for more flexibility for different hardware configurations.

@mudsut4ke mudsut4ke changed the title [sonic_platform_base/module_base]: Add firmware management interface [sonic_platform_base/chassis_base]: Add firmware management interface Jun 24, 2019
@mudsut4ke
Copy link
Contributor Author

@jleveque , Please review latest change and sonic-net/sonic-buildimage#2446

@jleveque
Copy link
Contributor

@mudsut4ke: I think the functions now look good, however, they should be present in both ChassisBase and ModuleBase. This will allow vendors to implement firmware updates for components on the chassis as well as components on a module (e.g., CPLD or FPGA on a line card).

@mudsut4ke
Copy link
Contributor Author

@jleveque , I will add these function to ModuleBase too

Copy link
Contributor

@jleveque jleveque left a comment

Choose a reason for hiding this comment

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

As comments. Note that my I made my comments in chassis_base.py, but they also apply to mondule_base.py, since the code is the same in both..

sonic_platform_base/chassis_base.py Outdated Show resolved Hide resolved
sonic_platform_base/chassis_base.py Outdated Show resolved Hide resolved
sonic_platform_base/chassis_base.py Outdated Show resolved Hide resolved
sonic_platform_base/chassis_base.py Outdated Show resolved Hide resolved
@mudsut4ke
Copy link
Contributor Author

OK

sonic_platform_base/chassis_base.py Outdated Show resolved Hide resolved
sonic_platform_base/chassis_base.py Outdated Show resolved Hide resolved
sonic_platform_base/chassis_base.py Outdated Show resolved Hide resolved
sonic_platform_base/module_base.py Outdated Show resolved Hide resolved
sonic_platform_base/module_base.py Outdated Show resolved Hide resolved
sonic_platform_base/chassis_base.py Outdated Show resolved Hide resolved
sonic_platform_base/module_base.py Outdated Show resolved Hide resolved
sonic_platform_base/chassis_base.py Outdated Show resolved Hide resolved
@mudsut4ke
Copy link
Contributor Author

@jleveque Comment updated as your request on 2754d94

@jleveque jleveque merged commit c021963 into sonic-net:master Jul 1, 2019
liat-grozovik pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Jul 4, 2019
* new platform api, chassis part

* Inject mlnx mlx libs to platform monitor

* address the review comments

* remove some confusing naming.

* Adjust the minor cause to a more human-readable way when rebooted by firmware

* address review comments

* expose host dir /host/reboot-cause to pmon docker so that the reboot causing by user command can be identified

* 1. Revert "expose host dir /host/reboot-cause to pmon docker so that the reboot causing by user command can be identified"
Since the only hardware-causing reboot should be handled by get_reboot_cause and the logic of handling reboot cause is about to move to the host side, no need to mount this dir to pmon docker.
This reverts commit 3feb968.
2. adjust log output by using sonic_daemon_base.daemon_base.Logger.
3. remove the logic of verifying /host/reboot-cause/ files.
4. fix typo.

* implement get_firmware_version and adjust the interfaces regarding components' version retrieving according to the sonic-net/sonic-platform-common#34
@mudsut4ke mudsut4ke deleted the add-fwmmgr-api branch August 6, 2019 03:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants