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

Disable Writeback Done Head interrupt when processing to avoid corruption of the list #1907

Closed
wants to merge 2 commits into from

Conversation

wooyay
Copy link
Contributor

@wooyay wooyay commented Feb 14, 2023

Describe the PR
On some devices (this was found on the EA LPC4088 Developer's Kit) the done list can become corrupted unless the WDH interrupt is disabled during processing.

@Ryzee119
Copy link
Contributor

Ryzee119 commented Feb 14, 2023

I wouldnt have thought that should be require in IRQ context, but the OHCI spec does disable master interrupts during interrupt servicing

https://ia801603.us.archive.org/4/items/hcir1_0a/hcir1_0a.pdf From page 80.

//
// It is our interrupt, prevent HC from doing it to us again until we are finished
//
DeviceData->HC->HcInterruptDisable = MasterInterruptEnable;

Then at the end

DeviceData->HC->HcInterruptEnable = MasterInterruptEnable;

Perhaps we should be doing this? Hopefully it achieves the same outcome as the change in this PR but aligns with the spec and might prevent other similar issues we havent seen yet.

@wooyay
Copy link
Contributor Author

wooyay commented Feb 15, 2023

Thanks for the link to the OHCI spec - I'd misplaced my copy and couldn't easily find another at the time.

As expected, replacing the WDH disable/enable with MIE disable/enable over the entire interrupt handler results in the same fixed behaviour on my board.

Looking at the issue again it's possible to resolve it without disabling interrupts (MIE or WDH) by writing 0 to HccaDoneHead once the list has been reversed (the example in the spec on page 82 shows it after processing the done list but this can happen at anytime before the WDH interrupt is acknowledged).

@Ryzee119
Copy link
Contributor

Nice looks good to me

@Ryzee119
Copy link
Contributor

btw I have a open PR for a bunch of OHCI tweaks and fixes. Its getting a bit behind master and I should rebase it but it might be worth having a look.
#1491

@wooyay
Copy link
Contributor Author

wooyay commented Feb 17, 2023

btw I have a open PR for a bunch of OHCI tweaks and fixes. Its getting a bit behind master and I should rebase it but it might be worth having a look. #1491

I saw this PR and gave it go to see whether it resolved this issue or not, but it did make the second root hub port on the LPC4088 work as well as the first.

Do you want to include this PR in yours to keep the OHCI fixes together in a single PR?

@Ryzee119
Copy link
Contributor

Ryzee119 commented Feb 18, 2023

Do you want to include this PR in yours to keep the OHCI fixes together in a single PR

@wooyay Good idea. I have cherry picked your commit and merge it into my PR. I have maintained you as author of the commit ofcourse.

I also went ahead and rebased my PR against current master. If you wish to try it out let me know! a comment in #1491 would be good as it will improve confidence in the PR to help merge to master🤞

@wooyay
Copy link
Contributor Author

wooyay commented Feb 18, 2023

I also went ahead and rebased my PR against current master. If you wish to try it out let me know! a comment in #1491 would be good as it will improve confidence in the PR to help merge to master🤞

@Ryzee119 Great - I'll take a look and give it a go but it may not be until early next week. I can't see anything to be worried about but I'll leave a comment on #1491 either way and then close this PR.

@wooyay
Copy link
Contributor Author

wooyay commented Feb 19, 2023

Closing this PR as it's now included in #1491.

@wooyay wooyay closed this Feb 19, 2023
@hathach
Copy link
Owner

hathach commented Feb 22, 2023

superb !! thank you two for the PR, #1491 is merged now

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.

3 participants