search

ReturnInfinity / BareMetal

A very minimal, resource efficient exo-kernel
http://www.returninfinity.com
MIT License
620 stars 89 forks source link

Some comments on the NVME implementation #62

Open freewilll opened 1 year ago

freewilll commented 1 year ago

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 commented 1 year ago

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