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

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

[Aaron Williams]

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.

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;

 	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;

 	if (nprps > dev->prp_entry_num) {
 		free(dev->prp_pool);
-		dev->prp_pool = malloc(nprps << 3);
+		dev->prp_pool = memalign(page_size, num_pages * page_size);
 		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;
 	}

 	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);
 	if (!ndev->prp_pool) {
 		ret = -ENOMEM;
 		printf("Error: %s: Out of memory!\n", udev->name);
--
2.16.4

[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@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);"

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

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

[Aaron Williams]

Hopefully this addresses your concerns.  Note that nprbs can't be used
directly for the size since one must be removed from each page to point
to the next page.

-Aaron

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

[Aaron Williams]

From: Aaron Williams <aaron.williams@cavium.com>

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.

Change-Id: I8df66c87d6a6105da556d327d4cc5148e444d20e
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;
 
 	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;
 
 	if (nprps > dev->prp_entry_num) {
 		free(dev->prp_pool);
-		dev->prp_pool = malloc(nprps << 3);
+		dev->prp_pool = memalign(page_size, num_pages * page_size);
 		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;
 	}
 
 	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);
 	if (!ndev->prp_pool) {
 		ret = -ENOMEM;
 		printf("Error: %s: Out of memory!\n", udev->name);
-- 
2.16.4

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

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

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

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

[Aaron Williams]

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

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

[Bin Meng]

Hi Aaron,

On Thu, Aug 22, 2019 at 6:06 AM Aaron Williams <awilliams@marvell.com> wrote:
>
> 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.

OK, please add some comments there indicating why we are doing this. Thanks.

[snip]

Regards,
Bin

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

[Aaron Williams]

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.

Signed-off-by: Aaron Williams <awilliams@marvell.com>
---
 drivers/nvme/nvme.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c
index d4965e2ef6..bc4cf40b40 100644
--- a/drivers/nvme/nvme.c
+++ b/drivers/nvme/nvme.c
@@ -73,6 +73,9 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2,
 	u64 *prp_pool;
 	int length = total_len;
 	int i, nprps;
+	u32 prps_per_page = (page_size >> 3) - 1;
+	u32 num_pages;
+
 	length -= (page_size - offset);
 
 	if (length <= 0) {
@@ -89,15 +92,19 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2,
 	}
 
 	nprps = DIV_ROUND_UP(length, page_size);
+	num_pages = DIV_ROUND_UP(nprps, prps_per_page);
 
 	if (nprps > dev->prp_entry_num) {
 		free(dev->prp_pool);
-		dev->prp_pool = malloc(nprps << 3);
+		/* Always increase in increments of pages.  It doesn't waste
+		 * much memory and reduces the number of allocations.
+		 */
+		dev->prp_pool = memalign(page_size, num_pages * page_size);
 		if (!dev->prp_pool) {
 			printf("Error: malloc prp_pool fail\n");
 			return -ENOMEM;
 		}
-		dev->prp_entry_num = nprps;
+		dev->prp_entry_num = prps_per_page * num_pages;
 	}
 
 	prp_pool = dev->prp_pool;
@@ -788,14 +795,6 @@ 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);
-	if (!ndev->prp_pool) {
-		ret = -ENOMEM;
-		printf("Error: %s: Out of memory!\n", udev->name);
-		goto free_nvme;
-	}
-	ndev->prp_entry_num = MAX_PRP_POOL >> 3;
-
 	ndev->cap = nvme_readq(&ndev->bar->cap);
 	ndev->q_depth = min_t(int, NVME_CAP_MQES(ndev->cap) + 1, NVME_Q_DEPTH);
 	ndev->db_stride = 1 << NVME_CAP_STRIDE(ndev->cap);
@@ -805,6 +804,15 @@ static int nvme_probe(struct udevice *udev)
 	if (ret)
 		goto free_queue;
 
+	/* Allocate after the page size is known */
+	ndev->prp_pool = memalign(ndev->page_size, MAX_PRP_POOL);
+	if (!ndev->prp_pool) {
+		ret = -ENOMEM;
+		printf("Error: %s: Out of memory!\n", udev->name);
+		goto free_nvme;
+	}
+	ndev->prp_entry_num = MAX_PRP_POOL >> 3;
+
 	ret = nvme_setup_io_queues(ndev);
 	if (ret)
 		goto free_queue;
-- 
2.16.4

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

[Aaron Williams]

I'm sorry about the messed up subject saying [PATCH]. For some reason git 
send-email is mangling the subject line. I'm new to trying to use this method 
to send out patches. This is version 2 of my patch.

-Aaron

On Thursday, August 22, 2019 2:12:32 AM PDT Aaron Williams 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.
> 
> Signed-off-by: Aaron Williams <awilliams@marvell.com>
> ---
>  drivers/nvme/nvme.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c
> index d4965e2ef6..bc4cf40b40 100644
> --- a/drivers/nvme/nvme.c
> +++ b/drivers/nvme/nvme.c
> @@ -73,6 +73,9 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64
> *prp2, u64 *prp_pool;
>  	int length = total_len;
>  	int i, nprps;
> +	u32 prps_per_page = (page_size >> 3) - 1;
> +	u32 num_pages;
> +
>  	length -= (page_size - offset);
> 
>  	if (length <= 0) {
> @@ -89,15 +92,19 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64
> *prp2, }
> 
>  	nprps = DIV_ROUND_UP(length, page_size);
> +	num_pages = DIV_ROUND_UP(nprps, prps_per_page);
> 
>  	if (nprps > dev->prp_entry_num) {
>  		free(dev->prp_pool);
> -		dev->prp_pool = malloc(nprps << 3);
> +		/* Always increase in increments of pages.  It doesn't 
waste
> +		 * much memory and reduces the number of allocations.
> +		 */
> +		dev->prp_pool = memalign(page_size, num_pages * 
page_size);
>  		if (!dev->prp_pool) {
>  			printf("Error: malloc prp_pool fail\n");
>  			return -ENOMEM;
>  		}
> -		dev->prp_entry_num = nprps;
> +		dev->prp_entry_num = prps_per_page * num_pages;
>  	}
> 
>  	prp_pool = dev->prp_pool;
> @@ -788,14 +795,6 @@ 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);
> -	if (!ndev->prp_pool) {
> -		ret = -ENOMEM;
> -		printf("Error: %s: Out of memory!\n", udev->name);
> -		goto free_nvme;
> -	}
> -	ndev->prp_entry_num = MAX_PRP_POOL >> 3;
> -
>  	ndev->cap = nvme_readq(&ndev->bar->cap);
>  	ndev->q_depth = min_t(int, NVME_CAP_MQES(ndev->cap) + 1, 
NVME_Q_DEPTH);
>  	ndev->db_stride = 1 << NVME_CAP_STRIDE(ndev->cap);
> @@ -805,6 +804,15 @@ static int nvme_probe(struct udevice *udev)
>  	if (ret)
>  		goto free_queue;
> 
> +	/* Allocate after the page size is known */
> +	ndev->prp_pool = memalign(ndev->page_size, MAX_PRP_POOL);
> +	if (!ndev->prp_pool) {
> +		ret = -ENOMEM;
> +		printf("Error: %s: Out of memory!\n", udev->name);
> +		goto free_nvme;
> +	}
> +	ndev->prp_entry_num = MAX_PRP_POOL >> 3;
> +
>  	ret = nvme_setup_io_queues(ndev);
>  	if (ret)
>  		goto free_queue;

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

[Bin Meng]

HI Aaron,

On Thu, Aug 22, 2019 at 5:12 PM 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.
>
> Signed-off-by: Aaron Williams <awilliams@marvell.com>
> ---
>  drivers/nvme/nvme.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c
> index d4965e2ef6..bc4cf40b40 100644
> --- a/drivers/nvme/nvme.c
> +++ b/drivers/nvme/nvme.c
> @@ -73,6 +73,9 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2,
>         u64 *prp_pool;
>         int length = total_len;
>         int i, nprps;
> +       u32 prps_per_page = (page_size >> 3) - 1;
> +       u32 num_pages;
> +
>         length -= (page_size - offset);
>
>         if (length <= 0) {
> @@ -89,15 +92,19 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2,
>         }
>
>         nprps = DIV_ROUND_UP(length, page_size);
> +       num_pages = DIV_ROUND_UP(nprps, prps_per_page);
>
>         if (nprps > dev->prp_entry_num) {
>                 free(dev->prp_pool);
> -               dev->prp_pool = malloc(nprps << 3);
> +               /* Always increase in increments of pages.  It doesn't waste

nits: please use the correct multi-line comment format.

> +                * much memory and reduces the number of allocations.
> +                */
> +               dev->prp_pool = memalign(page_size, num_pages * page_size);
>                 if (!dev->prp_pool) {
>                         printf("Error: malloc prp_pool fail\n");
>                         return -ENOMEM;
>                 }
> -               dev->prp_entry_num = nprps;
> +               dev->prp_entry_num = prps_per_page * num_pages;
>         }
>
>         prp_pool = dev->prp_pool;
> @@ -788,14 +795,6 @@ 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);
> -       if (!ndev->prp_pool) {
> -               ret = -ENOMEM;
> -               printf("Error: %s: Out of memory!\n", udev->name);
> -               goto free_nvme;
> -       }
> -       ndev->prp_entry_num = MAX_PRP_POOL >> 3;
> -
>         ndev->cap = nvme_readq(&ndev->bar->cap);
>         ndev->q_depth = min_t(int, NVME_CAP_MQES(ndev->cap) + 1, NVME_Q_DEPTH);
>         ndev->db_stride = 1 << NVME_CAP_STRIDE(ndev->cap);
> @@ -805,6 +804,15 @@ static int nvme_probe(struct udevice *udev)
>         if (ret)
>                 goto free_queue;
>
> +       /* Allocate after the page size is known */
> +       ndev->prp_pool = memalign(ndev->page_size, MAX_PRP_POOL);
> +       if (!ndev->prp_pool) {
> +               ret = -ENOMEM;
> +               printf("Error: %s: Out of memory!\n", udev->name);
> +               goto free_nvme;
> +       }
> +       ndev->prp_entry_num = MAX_PRP_POOL >> 3;
> +
>         ret = nvme_setup_io_queues(ndev);
>         if (ret)
>                 goto free_queue;
> --

Other than above nits, you can include my:
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

in your next version patch. Thanks!

Regards,
Bin

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

[Aaron Williams]

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.

Signed-off-by: Aaron Williams <awilliams@marvell.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
 drivers/nvme/nvme.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c
index d4965e2ef6..bc4cf40b40 100644
--- a/drivers/nvme/nvme.c
+++ b/drivers/nvme/nvme.c
@@ -73,6 +73,9 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2,
 	u64 *prp_pool;
 	int length = total_len;
 	int i, nprps;
+	u32 prps_per_page = (page_size >> 3) - 1;
+	u32 num_pages;
+
 	length -= (page_size - offset);

 	if (length <= 0) {
@@ -89,15 +92,19 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2,
 	}

 	nprps = DIV_ROUND_UP(length, page_size);
+	num_pages = DIV_ROUND_UP(nprps, prps_per_page);

 	if (nprps > dev->prp_entry_num) {
 		free(dev->prp_pool);
-		dev->prp_pool = malloc(nprps << 3);
+		/* Always increase in increments of pages.  It doesn't waste
+		 * much memory and reduces the number of allocations.
+		 */
+		dev->prp_pool = memalign(page_size, num_pages * page_size);
 		if (!dev->prp_pool) {
 			printf("Error: malloc prp_pool fail\n");
 			return -ENOMEM;
 		}
-		dev->prp_entry_num = nprps;
+		dev->prp_entry_num = prps_per_page * num_pages;
 	}

 	prp_pool = dev->prp_pool;
@@ -788,14 +795,6 @@ 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);
-	if (!ndev->prp_pool) {
-		ret = -ENOMEM;
-		printf("Error: %s: Out of memory!\n", udev->name);
-		goto free_nvme;
-	}
-	ndev->prp_entry_num = MAX_PRP_POOL >> 3;
-
 	ndev->cap = nvme_readq(&ndev->bar->cap);
 	ndev->q_depth = min_t(int, NVME_CAP_MQES(ndev->cap) + 1, NVME_Q_DEPTH);
 	ndev->db_stride = 1 << NVME_CAP_STRIDE(ndev->cap);
@@ -805,6 +804,15 @@ static int nvme_probe(struct udevice *udev)
 	if (ret)
 		goto free_queue;

+	/* Allocate after the page size is known */
+	ndev->prp_pool = memalign(ndev->page_size, MAX_PRP_POOL);
+	if (!ndev->prp_pool) {
+		ret = -ENOMEM;
+		printf("Error: %s: Out of memory!\n", udev->name);
+		goto free_nvme;
+	}
+	ndev->prp_entry_num = MAX_PRP_POOL >> 3;
+
 	ret = nvme_setup_io_queues(ndev);
 	if (ret)
 		goto free_queue;
--
2.16.4

[U-Boot] [PATCH v3 0/1] nvme: Fix invalid PRP Offset

[Aaron Williams]

Hopefully this addresses all of the issues.

I ran into problems with the Samsung EVO 970+ NVME drive where I was
getting a status code of 0x2013 which is Invalid PRP Offset.  The
PRP data structure cannot span page boundaries and it should be a
multiple of the page size, where the last entry points to the next
page.  This patch fixes this and duplicates the way this works in
the Linux kernel.

Personally I would like to move away from using PRP since I think
it may be possible to just use a simple scatter-gather entry instead.

If I do that it will be in a later patch.

Aaron Williams (1):
  nvme: Fix PRP Offset Invalid

 drivers/nvme/nvme.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

--
2.16.4

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

[Aaron Williams]

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.

Signed-off-by: Aaron Williams <awilliams@marvell.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
 drivers/nvme/nvme.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c
index d4965e2ef6..bc4cf40b40 100644
--- a/drivers/nvme/nvme.c
+++ b/drivers/nvme/nvme.c
@@ -73,6 +73,9 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2,
 	u64 *prp_pool;
 	int length = total_len;
 	int i, nprps;
+	u32 prps_per_page = (page_size >> 3) - 1;
+	u32 num_pages;
+
 	length -= (page_size - offset);
 
 	if (length <= 0) {
@@ -89,15 +92,19 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2,
 	}
 
 	nprps = DIV_ROUND_UP(length, page_size);
+	num_pages = DIV_ROUND_UP(nprps, prps_per_page);
 
 	if (nprps > dev->prp_entry_num) {
 		free(dev->prp_pool);
-		dev->prp_pool = malloc(nprps << 3);
+		/* Always increase in increments of pages.  It doesn't waste
+		 * much memory and reduces the number of allocations.
+		 */
+		dev->prp_pool = memalign(page_size, num_pages * page_size);
 		if (!dev->prp_pool) {
 			printf("Error: malloc prp_pool fail\n");
 			return -ENOMEM;
 		}
-		dev->prp_entry_num = nprps;
+		dev->prp_entry_num = prps_per_page * num_pages;
 	}
 
 	prp_pool = dev->prp_pool;
@@ -788,14 +795,6 @@ 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);
-	if (!ndev->prp_pool) {
-		ret = -ENOMEM;
-		printf("Error: %s: Out of memory!\n", udev->name);
-		goto free_nvme;
-	}
-	ndev->prp_entry_num = MAX_PRP_POOL >> 3;
-
 	ndev->cap = nvme_readq(&ndev->bar->cap);
 	ndev->q_depth = min_t(int, NVME_CAP_MQES(ndev->cap) + 1, NVME_Q_DEPTH);
 	ndev->db_stride = 1 << NVME_CAP_STRIDE(ndev->cap);
@@ -805,6 +804,15 @@ static int nvme_probe(struct udevice *udev)
 	if (ret)
 		goto free_queue;
 
+	/* Allocate after the page size is known */
+	ndev->prp_pool = memalign(ndev->page_size, MAX_PRP_POOL);
+	if (!ndev->prp_pool) {
+		ret = -ENOMEM;
+		printf("Error: %s: Out of memory!\n", udev->name);
+		goto free_nvme;
+	}
+	ndev->prp_entry_num = MAX_PRP_POOL >> 3;
+
 	ret = nvme_setup_io_queues(ndev);
 	if (ret)
 		goto free_queue;
-- 
2.16.4

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

[Bin Meng]

Hi Aaron,

On Fri, Aug 23, 2019 at 2:05 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.
>
> Signed-off-by: Aaron Williams <awilliams@marvell.com>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>  drivers/nvme/nvme.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c
> index d4965e2ef6..bc4cf40b40 100644
> --- a/drivers/nvme/nvme.c
> +++ b/drivers/nvme/nvme.c
> @@ -73,6 +73,9 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2,
>         u64 *prp_pool;
>         int length = total_len;
>         int i, nprps;
> +       u32 prps_per_page = (page_size >> 3) - 1;
> +       u32 num_pages;
> +
>         length -= (page_size - offset);
>
>         if (length <= 0) {
> @@ -89,15 +92,19 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2,
>         }
>
>         nprps = DIV_ROUND_UP(length, page_size);
> +       num_pages = DIV_ROUND_UP(nprps, prps_per_page);
>
>         if (nprps > dev->prp_entry_num) {
>                 free(dev->prp_pool);
> -               dev->prp_pool = malloc(nprps << 3);
> +               /* Always increase in increments of pages.  It doesn't waste

It seems you forgot the address this multi-line comment format issue.

Please resend the patch. thanks!

> +                * much memory and reduces the number of allocations.
> +                */

[snip]

Regards,
Bin

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

[Aaron Williams]

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.

Signed-off-by: Aaron Williams <awilliams@marvell.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
 drivers/nvme/nvme.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c
index d4965e2ef6..47f101e280 100644
--- a/drivers/nvme/nvme.c
+++ b/drivers/nvme/nvme.c
@@ -73,6 +73,9 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2,
 	u64 *prp_pool;
 	int length = total_len;
 	int i, nprps;
+	u32 prps_per_page = (page_size >> 3) - 1;
+	u32 num_pages;
+
 	length -= (page_size - offset);

 	if (length <= 0) {
@@ -89,15 +92,20 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2,
 	}

 	nprps = DIV_ROUND_UP(length, page_size);
+	num_pages = DIV_ROUND_UP(nprps, prps_per_page);

 	if (nprps > dev->prp_entry_num) {
 		free(dev->prp_pool);
-		dev->prp_pool = malloc(nprps << 3);
+		/*
+		 * Always increase in increments of pages.  It doesn't waste
+		 * much memory and reduces the number of allocations.
+		 */
+		dev->prp_pool = memalign(page_size, num_pages * page_size);
 		if (!dev->prp_pool) {
 			printf("Error: malloc prp_pool fail\n");
 			return -ENOMEM;
 		}
-		dev->prp_entry_num = nprps;
+		dev->prp_entry_num = prps_per_page * num_pages;
 	}

 	prp_pool = dev->prp_pool;
@@ -788,14 +796,6 @@ 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);
-	if (!ndev->prp_pool) {
-		ret = -ENOMEM;
-		printf("Error: %s: Out of memory!\n", udev->name);
-		goto free_nvme;
-	}
-	ndev->prp_entry_num = MAX_PRP_POOL >> 3;
-
 	ndev->cap = nvme_readq(&ndev->bar->cap);
 	ndev->q_depth = min_t(int, NVME_CAP_MQES(ndev->cap) + 1, NVME_Q_DEPTH);
 	ndev->db_stride = 1 << NVME_CAP_STRIDE(ndev->cap);
@@ -805,6 +805,15 @@ static int nvme_probe(struct udevice *udev)
 	if (ret)
 		goto free_queue;

+	/* Allocate after the page size is known */
+	ndev->prp_pool = memalign(ndev->page_size, MAX_PRP_POOL);
+	if (!ndev->prp_pool) {
+		ret = -ENOMEM;
+		printf("Error: %s: Out of memory!\n", udev->name);
+		goto free_nvme;
+	}
+	ndev->prp_entry_num = MAX_PRP_POOL >> 3;
+
 	ret = nvme_setup_io_queues(ndev);
 	if (ret)
 		goto free_queue;
--
2.16.4

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

[Aaron Williams]

Doh!  I forgot to check that in.  This should fix it.  I'm still
trying to get used to git send-email.

-Aaron

Aaron Williams (1):
  nvme: Fix PRP Offset Invalid

 drivers/nvme/nvme.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

--
2.16.4

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

[Aaron Williams]

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.

Signed-off-by: Aaron Williams <awilliams@marvell.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
 drivers/nvme/nvme.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c
index d4965e2ef6..47f101e280 100644
--- a/drivers/nvme/nvme.c
+++ b/drivers/nvme/nvme.c
@@ -73,6 +73,9 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2,
 	u64 *prp_pool;
 	int length = total_len;
 	int i, nprps;
+	u32 prps_per_page = (page_size >> 3) - 1;
+	u32 num_pages;
+
 	length -= (page_size - offset);
 
 	if (length <= 0) {
@@ -89,15 +92,20 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2,
 	}
 
 	nprps = DIV_ROUND_UP(length, page_size);
+	num_pages = DIV_ROUND_UP(nprps, prps_per_page);
 
 	if (nprps > dev->prp_entry_num) {
 		free(dev->prp_pool);
-		dev->prp_pool = malloc(nprps << 3);
+		/*
+		 * Always increase in increments of pages.  It doesn't waste
+		 * much memory and reduces the number of allocations.
+		 */
+		dev->prp_pool = memalign(page_size, num_pages * page_size);
 		if (!dev->prp_pool) {
 			printf("Error: malloc prp_pool fail\n");
 			return -ENOMEM;
 		}
-		dev->prp_entry_num = nprps;
+		dev->prp_entry_num = prps_per_page * num_pages;
 	}
 
 	prp_pool = dev->prp_pool;
@@ -788,14 +796,6 @@ 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);
-	if (!ndev->prp_pool) {
-		ret = -ENOMEM;
-		printf("Error: %s: Out of memory!\n", udev->name);
-		goto free_nvme;
-	}
-	ndev->prp_entry_num = MAX_PRP_POOL >> 3;
-
 	ndev->cap = nvme_readq(&ndev->bar->cap);
 	ndev->q_depth = min_t(int, NVME_CAP_MQES(ndev->cap) + 1, NVME_Q_DEPTH);
 	ndev->db_stride = 1 << NVME_CAP_STRIDE(ndev->cap);
@@ -805,6 +805,15 @@ static int nvme_probe(struct udevice *udev)
 	if (ret)
 		goto free_queue;
 
+	/* Allocate after the page size is known */
+	ndev->prp_pool = memalign(ndev->page_size, MAX_PRP_POOL);
+	if (!ndev->prp_pool) {
+		ret = -ENOMEM;
+		printf("Error: %s: Out of memory!\n", udev->name);
+		goto free_nvme;
+	}
+	ndev->prp_entry_num = MAX_PRP_POOL >> 3;
+
 	ret = nvme_setup_io_queues(ndev);
 	if (ret)
 		goto free_queue;
-- 
2.16.4

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

[Tom Rini]

On Thu, Aug 22, 2019 at 08:37:26PM -0700, Aaron Williams 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.
> 
> Signed-off-by: Aaron Williams <awilliams@marvell.com>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190826/9b3da7bf/attachment.sig>