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

[Aaron Williams]

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

-- 
Aaron Williams
Senior Software Engineer
Marvell Semiconductor, Inc.
(408) 943-7198	(510) 789-8988 (cell)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190821/1e3098b2/attachment.sig>