[U-Boot] [PATCH 1/1] nvme: Fix PRP Offset Invalid

[Bin Meng]

Hi Aaron,

On Wed, Aug 21, 2019 at 8:34 AM Aaron Williams <awilliams at marvell.com> wrote:
>
> When large writes take place I saw a Samsung
> EVO 970+ return a status value of 0x13, PRP
> Offset Invalid.  I tracked this down to the
> improper handling of PRP entries.  The blocks
> the PRP entries are placed in cannot cross a
> page boundary and thus should be allocated on
> page boundaries.  This is how the Linux kernel
> driver works.
>
> With this patch, the PRP pool is allocated on
> a page boundary and other than the very first
> allocation, the pool size is a multiple of
> the page size.  Each page can hold (4096 / 8) - 1
> entries since the last entry must point to the
> next page in the pool.

Please write more words in a line, about 70 characters in a line.

>
> Signed-off-by: Aaron Williams <awilliams at marvell.com>
> ---
>  drivers/nvme/nvme.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c
> index 7008a54a6d..ae64459edf 100644
> --- a/drivers/nvme/nvme.c
> +++ b/drivers/nvme/nvme.c
> @@ -75,6 +75,8 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2,
>         int length = total_len;
>         int i, nprps;
>         length -= (page_size - offset);
> +       u32 prps_per_page = (page_size >> 3) - 1;
> +       u32 num_pages;

nits: please move these 2 above the line "length -= (page_size - offset);"

>
>         if (length <= 0) {
>                 *prp2 = 0;
> @@ -90,15 +92,16 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2,
>         }
>
>         nprps = DIV_ROUND_UP(length, page_size);
> +       num_pages = (nprps + prps_per_page - 1) / prps_per_page;

use DIV_ROUND_UP()

>
>         if (nprps > dev->prp_entry_num) {

I think we should adjust nprps before the comparison here.

nprps += num_pages - 1;

>                 free(dev->prp_pool);
> -               dev->prp_pool = malloc(nprps << 3);
> +               dev->prp_pool = memalign(page_size, num_pages * page_size);

Then we need only do: dev->prp_pool = memalign(page_size, nprps << 3)?

>                 if (!dev->prp_pool) {
>                         printf("Error: malloc prp_pool fail\n");
>                         return -ENOMEM;
>                 }
> -               dev->prp_entry_num = nprps;
> +               dev->prp_entry_num = ((page_size >> 3) - 1) * num_pages;

and no need to change this line, but we need change the while (nprps) {} loop.

>         }
>
>         prp_pool = dev->prp_pool;
> @@ -791,7 +794,7 @@ static int nvme_probe(struct udevice *udev)
>         }
>         memset(ndev->queues, 0, NVME_Q_NUM * sizeof(struct nvme_queue *));
>
> -       ndev->prp_pool = malloc(MAX_PRP_POOL);
> +       ndev->prp_pool = memalign(1 << 12, MAX_PRP_POOL);

I think we need use ndev->pagesize instead of (1 << 12), but
ndev->pagesize is initialized in nvme_configure_admin_queue(), so we
need move the initialization of ndev->pagesize out of that function
and do it before the memalign() here.

>         if (!ndev->prp_pool) {
>                 ret = -ENOMEM;
>                 printf("Error: %s: Out of memory!\n", udev->name);
> --

Regards,
Bin