Skip to content

[Y-Cable][Credo] add theading locker to support thread-safe calling, add SKU check for download_firmware API. #222

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

Jump to bottom
Merged
merged 3 commits into from
Feb 15, 2022
Merged

[Y-Cable][Credo] add theading locker to support thread-safe calling, add SKU check for download_firmware API. #222

merged 3 commits into from
Feb 15, 2022

Conversation

xinyulin
Copy link
Contributor

@xinyulin xinyulin commented Oct 22, 2021 •
edited by vdahiya12
Loading

Description

Add threading locker to protect VSC protocol
This PR also adds SKU check for firmware download in the API.
so 50G or 100G cables wont be allowed to the alternate firmware versions, meaning 50G wont be allowed to download 100G and 100G wont be allowed 50G

Motivation and Context

To solve the racing condition when multiple threads access the same port at the same time

How Has This Been Tested?

Tested on Arista 7050

Additional Information (Optional)

@prgeor prgeor requested a review from vdahiya12 December 2, 2021 10:53
@vdahiya12
Copy link
Contributor

@xinyulin Can we add more methods which require locking during firmware update to be added inside this.

@xinyulin
[Y-Cable][Credo] fix racing issue of VSC releated APIs
562dbd4 
    The VSC protocol didn't allow user to send two or more vsc command to the module simultaneously,
    otherwise unexpected error might occurred.

    To avoid this issue, we can simply to update the download_firmware_statue in the the following functions.
    This could help the helper to know vsc is in progress or not.

        * get_firmware_version()
        * download_firmware()
        * activate_firmware()
        * rollback_firmware()

Signed-off-by: xinyu <[email protected]>
@xinyulin
Copy link
Contributor Author

xinyulin commented Dec 8, 2021

@vdahiya12 Is it acceptable to get ack_error=1 during firmware_activate()? If not, I think I'd need to add thread lock on most of APIs.
As I explained in the mail thread, the module is unable to response any i2c transaction during firmware activation.
If the y_cable_helper call any eeprom read/write related API (ex. get_read_side()) during firmware activation, it will get nothing but a NACK error.

@vdahiya12
Copy link
Contributor

@vdahiya12 Is it acceptable to get ack_error=1 during firmware_activate()? If not, I think I'd need to add thread lock on most of APIs. As I explained in the mail thread, the module is unable to response any i2c transaction during firmware activation. If the y_cable_helper call any eeprom read/write related API (ex. get_read_side()) during firmware activation, it will get nothing but a NACK error.

Okay lets add the thread locker on API's that we can get rid of the NACK errors

@vdahiya12 vdahiya12 changed the title [Y-Cable][Credo] add theading locker to support thread-safe calling [Y-Cable][Credo] add theading locker to support thread-safe calling, add SKU check for download_firmware API. Feb 15, 2022
@vdahiya12 vdahiya12 merged commit 931c6ea into sonic-net:master Feb 15, 2022
qiluo-msft pushed a commit that referenced this pull request Feb 15, 2022
@xinyulin @qiluo-msft
[Y-Cable][Credo] add theading locker to support thread-safe calling, …
c249681 
…add SKU check for download_firmware API. (#222)

* [Y-Cable][Credo] fix racing issue of VSC releated APIs

    The VSC protocol didn't allow user to send two or more vsc command to the module simultaneously,
    otherwise unexpected error might occurred.

    To avoid this issue, we can simply to update the download_firmware_status in the the following functions.
    This could help the helper to know vsc is in progress or not.

        * get_firmware_version()
        * download_firmware()
        * activate_firmware()
        * rollback_firmware()
This PR also does add SKU check for download_firmware API.
* [Y-Cable][Credo] add firmware ID checker to avoid update the wrong firmware

Signed-off-by: xinyu <[email protected]>
@vdahiya12 vdahiya12 mentioned this pull request Mar 16, 2022
Merged
vdahiya12 added a commit that referenced this pull request Mar 16, 2022
@vdahiya12
[Credo][Ycable] Fix Credo firmware download API download_firmware flag (
573717a 
#269)

Signed-off-by: vaibhav-dahiya <[email protected]>

Due to PR #222
the download_firmware_status variable of download_firmware API was not cleaned up, due to which higher layer
could sometimes see wrong values of firmware version.
This PR addresses this issue.

After calling a firmware download and not changing this variable


admin@sonic:~$ show mux firmware version Ethernet0
{
    "version_nic_active": "N/A",
    "version_nic_inactive": "N/A",
    "version_nic_next": "N/A",
    "version_peer_active": "N/A",
    "version_peer_inactive": "N/A",
    "version_peer_next": "N/A",
    "version_self_active": "N/A",
    "version_self_inactive": "N/A",
    "version_self_next": "N/A"
}
after the change

admin@sonic:~$ show mux firmware version Ethernet0
<versionX>
{
    "version_nic_active": "1.0MS",
    "version_nic_inactive": "1.1MS",
    "version_nic_next": "1.0MS",
    "version_peer_active": "1.0MS",
    "version_peer_inactive": "1.1MS",
    "version_peer_next": "1.0MS",
    "version_self_active": "1.0MS",
    "version_self_inactive": "1.1MS",
    "version_self_next": "1.0MS"
}
Description
Motivation and Context
How Has This Been Tested?
Tested on an Arista Device.
qiluo-msft pushed a commit that referenced this pull request Mar 16, 2022
@vdahiya12 @qiluo-msft
[Credo][Ycable] Fix Credo firmware download API download_firmware flag (
eb35c85 
#269)

Signed-off-by: vaibhav-dahiya <[email protected]>

Due to PR #222
the download_firmware_status variable of download_firmware API was not cleaned up, due to which higher layer
could sometimes see wrong values of firmware version.
This PR addresses this issue.

After calling a firmware download and not changing this variable


admin@sonic:~$ show mux firmware version Ethernet0
{
    "version_nic_active": "N/A",
    "version_nic_inactive": "N/A",
    "version_nic_next": "N/A",
    "version_peer_active": "N/A",
    "version_peer_inactive": "N/A",
    "version_peer_next": "N/A",
    "version_self_active": "N/A",
    "version_self_inactive": "N/A",
    "version_self_next": "N/A"
}
after the change

admin@sonic:~$ show mux firmware version Ethernet0
<versionX>
{
    "version_nic_active": "1.0MS",
    "version_nic_inactive": "1.1MS",
    "version_nic_next": "1.0MS",
    "version_peer_active": "1.0MS",
    "version_peer_inactive": "1.1MS",
    "version_peer_next": "1.0MS",
    "version_self_active": "1.0MS",
    "version_self_inactive": "1.1MS",
    "version_self_next": "1.0MS"
}
Description
Motivation and Context
How Has This Been Tested?
Tested on an Arista Device.
@keboliu keboliu mentioned this pull request Apr 5, 2022
Merged
6 tasks
liat-grozovik pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Apr 7, 2022
@keboliu
[submodule] Advance sonic-platform-common pointer (#10469)
07fad26 
Update sonic-platform-common submodule to pickup new commits:
01512ec [SSD]Enhance ssd_generic with more error handling to avoid python crash sonic-net/sonic-platform-common#271
ac3e7f1 [y_cable][Broadcom] update the BRCM y_cable driver to release 2.0 sonic-net/sonic-platform-common#263
573717a [Credo][Ycable] Fix Credo firmware download API download_firmware flag sonic-net/sonic-platform-common#269
a844f18 [xcvr] Add get_module_fw_info method to XcvrApi class. sonic-net/sonic-platform-common#267
35bad16 [sfputil]Refactoring read_porttab_mappings sonic-net/sonic-platform-common#264
83c4345 [SSD Generic] Add support for parsing nvme ssd model, health and temperature sonic-net/sonic-platform-common#265
5da31e1 [ycable][credo] Fix the is_link_active API for Credo Ycable sonic-net/sonic-platform-common#260
931c6ea [Y-Cable][Credo] add theading locker to support thread-safe calling, add SKU check for download_firmware API. sonic-net/sonic-platform-common#222
ff3aa75 Fix SFF8472 Enhanced Options sonic-net/sonic-platform-common#259
a8a83e9 [ssd] Allow individual vendor parsers to handle errors sonic-net/sonic-platform-common#252

Signed-off-by: Kebo Liu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vdahiya12 vdahiya12 vdahiya12 approved these changes

Assignees

@vdahiya12 vdahiya12

Labels
Included in 202012 Branch Request for 202012 Branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xinyulin @vdahiya12 @qiluo-msft