[PATCH v3 1/2] nvme: Enable FUA

[PATCH v3 1/2] nvme: Enable FUA

[Jon Lin]

Most NVME devcies maintain data in internal cache for an uncertain
times, and u-boot has no method to force NVME to flush cache.
So this patch adds FUA to avoid data loss caused by power off after data
programming.

Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
Reviewed-by: Stefan Agner <stefan@agner.ch>
---

Changes in v3:
  Only enable FUA when vwc is enabled

 drivers/nvme/nvme.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c
index 3c529a2fce..9623c896a1 100644
--- a/drivers/nvme/nvme.c
+++ b/drivers/nvme/nvme.c
@@ -762,6 +762,10 @@ static ulong nvme_blk_rw(struct udevice *udev, lbaint_t blknr,
 	c.rw.appmask = 0;
 	c.rw.metadata = 0;
 
+	/* Enable FUA for data integrity if vwc is enabled */
+	if (dev->vwc)
+		c.rw.control |= NVME_RW_FUA;
+
 	while (total_lbas) {
 		if (total_lbas < lbas) {
 			lbas = (u16)total_lbas;
-- 
2.17.1

[PATCH v3 2/2] nvme: Fix error in nvme_setup_prps

[Jon Lin]

Consulting to "NVM Express® Base Specification, revision 2.0".

If more PRP List pages are required, then the last entry of
the PRP List contains the Page Base Address of the next PRP
List page. The next PRP List page shall be memory page aligned.

Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>
---

(no changes since v1)

 drivers/nvme/nvme.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c
index 9623c896a1..22ded626a5 100644
--- a/drivers/nvme/nvme.c
+++ b/drivers/nvme/nvme.c
@@ -100,7 +100,7 @@ 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);
+	num_pages = DIV_ROUND_UP(nprps + 1, prps_per_page);
 
 	if (nprps > dev->prp_entry_num) {
 		free(dev->prp_pool);
@@ -119,10 +119,11 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2,
 	prp_pool = dev->prp_pool;
 	i = 0;
 	while (nprps) {
-		if (i == ((page_size >> 3) - 1)) {
-			*(prp_pool + i) = cpu_to_le64((ulong)prp_pool +
+		if (i == prps_per_page) {
+			*(prp_pool + i) = *(prp_pool + i - 1);
+			*(prp_pool + i - 1) = cpu_to_le64((ulong)prp_pool +
 					page_size);
-			i = 0;
+			i = 1;
 			prp_pool += page_size;
 		}
 		*(prp_pool + i++) = cpu_to_le64(dma_addr);
-- 
2.17.1

Re: [PATCH v3 1/2] nvme: Enable FUA

[Bin Meng]

Hi Jon,

On Tue, Oct 19, 2021 at 10:41 AM Jon Lin <jon.lin@rock-chips.com> wrote:
>
> Most NVME devcies maintain data in internal cache for an uncertain
> times, and u-boot has no method to force NVME to flush cache.
> So this patch adds FUA to avoid data loss caused by power off after data
> programming.
>
> Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
> Reviewed-by: Stefan Agner <stefan@agner.ch>
> ---
>
> Changes in v3:
>   Only enable FUA when vwc is enabled
>
>  drivers/nvme/nvme.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c
> index 3c529a2fce..9623c896a1 100644
> --- a/drivers/nvme/nvme.c
> +++ b/drivers/nvme/nvme.c
> @@ -762,6 +762,10 @@ static ulong nvme_blk_rw(struct udevice *udev, lbaint_t blknr,
>         c.rw.appmask = 0;
>         c.rw.metadata = 0;
>
> +       /* Enable FUA for data integrity if vwc is enabled */
> +       if (dev->vwc)

Still this does not look correct to me.

The dev->vwc field only indicates the presence of the Volatile Write
Cache, but not the enable status of it.

> +               c.rw.control |= NVME_RW_FUA;
> +
>         while (total_lbas) {
>                 if (total_lbas < lbas) {
>                         lbas = (u16)total_lbas;

Regards,
Bin

Re: [PATCH v3 1/2] nvme: Enable FUA

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 436 bytes --]

On Tue, Oct 19, 2021 at 10:40:53AM +0800, Jon Lin wrote:

> Most NVME devcies maintain data in internal cache for an uncertain
> times, and u-boot has no method to force NVME to flush cache.
> So this patch adds FUA to avoid data loss caused by power off after data
> programming.
> 
> Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
> Reviewed-by: Stefan Agner <stefan@agner.ch>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 2/2] nvme: Fix error in nvme_setup_prps

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 478 bytes --]

On Tue, Oct 19, 2021 at 10:40:54AM +0800, Jon Lin wrote:

> Consulting to "NVM Express® Base Specification, revision 2.0".
> 
> If more PRP List pages are required, then the last entry of
> the PRP List contains the Page Base Address of the next PRP
> List page. The next PRP List page shall be memory page aligned.
> 
> Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
> Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 1/2] nvme: Enable FUA

[Bin Meng]

Hi Tom,

On Fri, Nov 19, 2021 at 3:14 AM Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Oct 19, 2021 at 10:40:53AM +0800, Jon Lin wrote:
>
> > Most NVME devcies maintain data in internal cache for an uncertain
> > times, and u-boot has no method to force NVME to flush cache.
> > So this patch adds FUA to avoid data loss caused by power off after data
> > programming.
> >
> > Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
> > Reviewed-by: Stefan Agner <stefan@agner.ch>
>
> Applied to u-boot/next, thanks!

I don't see my review comment being addressed. Please drop the patch
until all things are clear.

Regards,
Bin

Re: [PATCH v3 1/2] nvme: Enable FUA

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 767 bytes --]

On Fri, Nov 19, 2021 at 08:56:08AM +0800, Bin Meng wrote:
> Hi Tom,
> 
> On Fri, Nov 19, 2021 at 3:14 AM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Oct 19, 2021 at 10:40:53AM +0800, Jon Lin wrote:
> >
> > > Most NVME devcies maintain data in internal cache for an uncertain
> > > times, and u-boot has no method to force NVME to flush cache.
> > > So this patch adds FUA to avoid data loss caused by power off after data
> > > programming.
> > >
> > > Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
> > > Reviewed-by: Stefan Agner <stefan@agner.ch>
> >
> > Applied to u-boot/next, thanks!
> 
> I don't see my review comment being addressed. Please drop the patch
> until all things are clear.

Missed your comments, sorry.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 1/2] nvme: Enable FUA

[Jon Lin]

On 2021/11/19 9:14, Tom Rini wrote:
> On Fri, Nov 19, 2021 at 08:56:08AM +0800, Bin Meng wrote:
>> Hi Tom,
>>
>> On Fri, Nov 19, 2021 at 3:14 AM Tom Rini <trini@konsulko.com> wrote:
>>> On Tue, Oct 19, 2021 at 10:40:53AM +0800, Jon Lin wrote:
>>>
>>>> Most NVME devcies maintain data in internal cache for an uncertain
>>>> times, and u-boot has no method to force NVME to flush cache.
>>>> So this patch adds FUA to avoid data loss caused by power off after data
>>>> programming.
>>>>
>>>> Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
>>>> Reviewed-by: Stefan Agner <stefan@agner.ch>
>>> Applied to u-boot/next, thanks!
>> I don't see my review comment being addressed. Please drop the patch
>> until all things are clear.
> Missed your comments, sorry.
Thanks to bin Meng and Tom. I'm not familiar with nvme protocol. I 
expect people with nvme
experience to take over the patch and do the most reasonable treatment. 
Of course, I'll take
some time to seriously ponder the nvme process to see if it can meet 
your requirements