Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33582 )
Change subject: libpayload/storage: Add NVMe driver ......................................................................
Patch Set 15: Code-Review+1
(5 comments)
https://review.coreboot.org/c/coreboot/+/33582/15/payloads/libpayload/driver... File payloads/libpayload/drivers/storage/nvme.c:
https://review.coreboot.org/c/coreboot/+/33582/15/payloads/libpayload/driver... PS15, Line 60: : static storage_poll_t nvme_poll(struct storage_dev *dev); : static void nvme_detach_device(struct storage_dev *dev); : static ssize_t nvme_read_blocks512( : struct storage_dev *dev, lba_t start, size_t count, unsigned char *buf); : : static int create_admin_queues(struct nvme_dev *nvme); : static int create_io_submission_queue(struct nvme_dev *nvme); : static int create_io_completion_queue(struct nvme_dev *nvme); : static int delete_admin_queues(struct nvme_dev *nvme); : static int delete_io_submission_queue(struct nvme_dev *nvme); : static int delete_io_completion_queue(struct nvme_dev *nvme); : static int nvme_cmd( : struct nvme_dev *nvme, enum nvme_queue q, const struct nvme_s_queue_entry *cmd); : static int read(struct nvme_dev *nvme, unsigned char *buffer, uint64_t base, uint16_t count); : It would contribute to the readability of the code to reorder the functions so that this is not needed.
https://review.coreboot.org/c/coreboot/+/33582/15/payloads/libpayload/driver... PS15, Line 130: static int read even though this is static, but please don't use posix/bsd/sysv reserved function names (unless you are implementing standard functions)
https://review.coreboot.org/c/coreboot/+/33582/15/payloads/libpayload/driver... PS15, Line 390: FATAL ERROR How are tgese two functions more fatal than the other ones goto-ing to abort:?
https://review.coreboot.org/c/coreboot/+/33582/15/payloads/libpayload/driver... PS15, Line 394: NVMe ERROR: Failed to disable controller. Timeout.\n It would be beneficial to make the error message unique, so it is possible to determine where in the code we die, when something goes wrong. (Although this is mostly a copy of the above code, apart from the abort condition)
https://review.coreboot.org/c/coreboot/+/33582/15/payloads/libpayload/driver... File payloads/libpayload/drivers/storage/storage.c:
https://review.coreboot.org/c/coreboot/+/33582/15/payloads/libpayload/driver... PS15, Line 31: #if CONFIG(LP_STORAGE_AHCI) : # include <storage/ahci.h> : #endif : #if CONFIG(LP_STORAGE_NVME) : #include <storage/nvme.h> : #endif With these only containing one prototype each, are we sure that wrapping the includes in CONFIG() is warranted?