On 3/25/25 16:18, neil.armstr...@linaro.org wrote:
On 25/03/2025 14:49, Caleb Connolly wrote:
On 3/25/25 14:33, Neil Armstrong wrote:
On 25/03/2025 14:02, Caleb Connolly wrote:
We don't have a mechanism to safely shutdown block devices prior to a
baord reset or driver removal. Prevent data loss by synchronizing the
SCSI cache after every write.
In particular this solves the issue of capsule updates looping on some
devices because the board resets immediately after deleting the capsule
file and this write wouldn't be flushed in time.
This may impact NAND wear, but should be negligible given the usecases
for disk write in U-Boot.
Signed-off-by: Caleb Connolly <caleb.conno...@linaro.org>
---
drivers/scsi/scsi.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index
1aa7fbdbb5278e387de72a3c3e73d19ea0342fff..1b71e580b89b100395ea132a4976366a713cba8a 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -77,8 +77,25 @@ static void scsi_setup_inquiry(struct scsi_cmd
*pccb)
pccb->cmdlen = 6;
pccb->msgout[0] = SCSI_IDENTIFY; /* NOT USED */
}
+static void scsi_setup_sync_cache(struct scsi_cmd *pccb, lbaint_t
start,
+ unsigned short blocks)
+{
+ pccb->cmd[0] = SCSI_SYNC_CACHE;
+ pccb->cmd[1] = 0;
+ pccb->cmd[2] = (unsigned char)(start >> 24) & 0xff;
+ pccb->cmd[3] = (unsigned char)(start >> 16) & 0xff;
+ pccb->cmd[4] = (unsigned char)(start >> 8) & 0xff;
+ pccb->cmd[5] = (unsigned char)start & 0xff;
+ pccb->cmd[6] = 0;
+ pccb->cmd[7] = (unsigned char)(blocks >> 8) & 0xff;
+ pccb->cmd[8] = (unsigned char)blocks & 0xff;
+ pccb->cmd[9] = 0;
+ pccb->cmdlen = 10;
+ pccb->msgout[0] = SCSI_IDENTIFY; /* NOT USED */
+}
+
static void scsi_setup_read_ext(struct scsi_cmd *pccb, lbaint_t
start,
unsigned short blocks)
{
pccb->cmd[0] = SCSI_READ10;
@@ -235,8 +252,19 @@ static ulong scsi_write(struct udevice *dev,
lbaint_t blknr, lbaint_t blkcnt,
blkcnt -= blks;
break;
}
+ /*
+ * Ensure the writes are flushed to disk so we don't lose data
+ * on board reset.
+ * FIXME: this ought to be in a SCSI shutdown path instead
+ */
+ scsi_setup_sync_cache(pccb, start, smallblks);
Why can't this be done at the end of scsi_write() ?
We have the same limitation that we can only flush SCSI_MAX_BLK blocks
at once. It could be done in a second loop at the end but I figured
it's more sensible to just interleave them.
Indeed you're right, another solution is to leave count to 0, and it
will sync all data after start:
````
If the LBA and COUNT arguments are both zero (their defaults) then all
blocks in the cache are synchronized
If LBA is greater than zero while COUNT is zero then blocks in the cache
whose addresses are from and including LBA to the highest lba on the
device are synchronized.
```
And it seems Linux only passes 0 parameters, and just flushes all data
in cache:
https://elixir.bootlin.com/linux/v6.13.7/source/drivers/scsi/sd.c#L1145
So either you leave is as is so all data is written as soon as possible,
or we wait until the end of the write and flush everything.
Ahh, damn I should have checked for that!
I think the last one is just simpler and safer.
You're totally right XD
Thanks
Neil
+ if (scsi_exec(bdev, pccb)) {
+ scsi_print_error(pccb);
+ break;
+ }
+
if (blks > max_blks) {
start += max_blks;
blks -= max_blks;
} else {
And perhaps a sync subcommand of the scsi command would be nice to
have !
Good idea, that would tie in nicely if/when we get a .sync() op for
block devices and call it prior to board reset.
Neil
--
Caleb (they/them)
