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

Reverting #12312 as it breaking current WiFI connect()->Disconnect() sequence #12513

Closed
wants to merge 0 commits into from
Closed

Reverting #12312 as it breaking current WiFI connect()->Disconnect() sequence #12513

wants to merge 0 commits into from

Conversation

cy-arsm
Copy link

@cy-arsm cy-arsm commented Feb 26, 2020

This reverts commit 18285e1.

Summary of changes

Issue:
In disconnect sequence, whd_emac_wifi_link_state_changed(FALSE) is called which is an asynchronous operation and execution is handled in tcp_ip thread(). As part of this api, it access the interface structure.
remove_ethernet_interface will remove the interface from the netif list, this is a synchronous execution and called after whd_emac_wifi_link_state_changed().
Hence there is synchronization issue, where whd_emac_wifi_link_state_changed() is trying to access the interface structure which is already freed by remove_ehternet_interface(). Hence crash is seen.

Hence reverting the remove interface in SoftAP->stop() and STA->disconnect() as it breaking the current WiFi connect()->disconnect() sequence.

Impact of changes

Migration actions required

Documentation


Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[x] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers


@ciarmcom ciarmcom requested review from a team February 26, 2020 10:00
@ciarmcom
Copy link
Member

@cy-arsm, thank you for your changes.
@ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 26, 2020

Would it be worth adding a reason for the revert (using --edit when reverting), same that is here in description ?

@cy-arsm cy-arsm closed this Feb 26, 2020
@cy-arsm cy-arsm reopened this Feb 26, 2020
@cy-arsm cy-arsm changed the title Revert "Pairing fails when IPv6 enabled in SoftAP intf" Reverting #12312 as it breaking current WiFI connect()->Disconnect() sequence Feb 26, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 26, 2020

Not only title, but also commit message. git revert provides an option --edit to do so, worth having in the message.

@cy-arsm
Copy link
Author

cy-arsm commented Feb 26, 2020

@morser499 can you review the changes. Have reverted back the remove_ethernet_interface logic

@0xc0170 0xc0170 requested a review from morser499 February 27, 2020 08:36
@mergify mergify bot added needs: CI and removed needs: review labels Feb 27, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 27, 2020

CI started meanwhile

@mbed-ci
Copy link

mbed-ci commented Feb 27, 2020

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-IAR

@mergify mergify bot added needs: work and removed needs: CI labels Feb 27, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 28, 2020

CI restarted, internal fault

@mbed-ci
Copy link

mbed-ci commented Feb 28, 2020

Test run: FAILED

Summary: 1 of 8 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 28, 2020

@ARMmbed/mbed-os-ipcore Is this fine to go in?

@cy-arsm
Copy link
Author

cy-arsm commented Mar 3, 2020

@0xc0170 i have updated the summary comments with reason for the failure.

@mergify
Copy link

mergify bot commented Mar 4, 2020

This PR cannot be merged due to conflicts. Please rebase to resolve them.

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 5, 2020

Once rebased ,will restart CI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants