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

Some comments on the NVME implementation #62

Open
freewilll opened this issue Jan 2, 2023 · 1 comment
Open

Some comments on the NVME implementation #62

freewilll opened this issue Jan 2, 2023 · 1 comment
Assignees

Comments

@freewilll
Copy link

First of all, the NVME code has been a great help for me to develop my own driver for my toy OS. It's well documented and well written. I did however find a few issues. I'll post them all here for brevity, but they do deserve to be their own issues if taken seriously.

Missing wait loop when disabling the controller during initialization

When disabling the controller, a loop needs to wait for it to be actually disabled before continuing the initialization. Without this, the next initialization steps (creating the admin queues) can fail. I can reproduce this issue on my test laptop (an Asus Vivobook)

Missing error checking when waiting for command completion

In both the admin and I/O completion checks, DW2 is checked to be non zero. In fact, the comments say "Add 8 for DW3", but yet 8 is added, which is the offset for DW2. This check will correctly identify that the command completed. However, it would be better if the status code in DW3 was checked to be zero.

LBA size determination is incorrect

The code finds the highest possible LBA size and assumes the drive was formatted with it. I believe the correct thing to do is read the FLBAS byte, then use that in the LBA format table LBAF table to look up the LBA size of the formatted drive.

@IanSeyler IanSeyler self-assigned this Jan 3, 2023
@IanSeyler
Copy link
Member

Thanks for this! I will review and make the needed changes.

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

No branches or pull requests

2 participants