Hi Denis, On 2026-07-16T22:33:54, None <[email protected]> wrote: > drivers: nvme: Enable Force Unit Access (FUA) > > Enable FUA (if present) on NVMe devices to prevent any potential data > integrity issues caused by accidental power loss or malformed device > handoff sequence to the OS. > > Original FUA effort has been started in [1] > > [1] > https://lore.kernel.org/u-boot/20211019104049.v3.1.Ic581ec99f46b6dfa2e0b1922e670a333ac859e82@changeid/
Oddly that link doesn't work for me. > Original-patch-by: Jon Lin <[email protected]> > Signed-off-by: Denis Mukhin <[email protected]> > > drivers/nvme/nvme.c | 17 +++++++++++++++++ > drivers/nvme/nvme.h | 5 +++++ > 2 files changed, 22 insertions(+) > diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c > @@ -916,6 +923,16 @@ int nvme_init(struct udevice *udev) > + /* Check whether volatile write cache is enabled. */ > + if (ndev->vwc & NVME_CTRL_VWC_PRESENT) { > + u32 features; > + > + ret = nvme_get_features(ndev, NVME_FEAT_VOLATILE_WC, 0, > 0, > + &features); > + if (!ret && (features & 0x1)) > + ndev->flags |= NVME_VWC_ENABLED; > + } The volatile write cache is a controller-level property, not per-namespace, but this sits inside the namespace loop, so the same Get Features command is issued for every active namespace. Please can you move it out of the loop, e.g. into nvme_get_info_from_identify() next to where dev->vwc is read from the identify data? Also, 0x1 is the WCE bit from the spec, so please add a named constant in nvme.h rather than a magic number. BTW this adds the first caller of nvme_get_features(), so please drop the stale 'At the moment there is no user of this function' sentence from the TODO comment there. > diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c > @@ -788,6 +788,13 @@ static ulong nvme_blk_rw(struct udevice *udev, lbaint_t > blknr, > + /* > + * Enable force unit access (FUA) for data integrity if volatile write > + * cache (VWC) is enabled. > + */ > + if (!read && (dev->flags & NVME_VWC_ENABLED)) > + c.rw.control |= cpu_to_le16(NVME_RW_FUA); Just to check the design here: patch 3 implements the flush command and patch 5 exports nvme_shutdown(), which already flushes the cache before hand-off. With those in place, per-write FUA makes each write synchronous to media, which defeats the write cache and can slow down writing large images considerably. My understanding is that flushing at the appropriate points (explicit flush command, shutdown) gives the same integrity guarantee without the per-write cost. Could this patch be dropped once the rest of the series lands? What do you think? Regards, Simon
