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

[Aaron Williams <awilliams@marvell.com>]

Hi Bin,

On Wednesday, August 21, 2019 8:23:50 AM PDT Bin Meng wrote:
> Hi Aaron,
> 
> On Wed, Aug 21, 2019 at 7:26 PM Aaron Williams <awilliams@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@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@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.
This would work.

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

This is true, but if we were forced to increase it, there may be a good chance 
that it may need to be increased again. I do not see an issue in allocating 
based on the number of pages required rather than the number of bytes. At 
most, 4K-8 will be wasted. On the other hand, this can prevent additional 
memalign calls from being called.

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