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

[Bin Meng]

Hi Aaron,

On Wed, Aug 21, 2019 at 7:26 PM Aaron Williams <awilliams at marvell.com> wrote:
>
> Hi Bin,
>
> I submitted another patch via git. Hopefully it went through. I'm new to
> trying to get email to work with GIT since until now nobody in my group has
> had access to a working SMTP server so I'm still learning how to use git send-
> email.
>
> -Aaron
>
> On Wednesday, August 21, 2019 12:55:59 AM PDT Bin Meng wrote:
> > External Email
> >
> > ----------------------------------------------------------------------
> > 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);"
> Done.
> >
> > >         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()
> Done
> >
> > >         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)?
> We can't use nprps << 3 because if the prps span more than a single page then
> we lose a prp per page.

Looks you missed my comment above.

If we adjust nprps by "nprps += num_pages - 1", then we don't lose the
last prp per page.

Mallocing num_pages * page_size exceeds the real needs. We should only
allocate the exact prp size we need.

> >
> > >                 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