[PATCH 0/3] scsi: ensure writes are flushed to disk

[PATCH 0/3] scsi: ensure writes are flushed to disk

[Caleb Connolly]

SCSI devices like UFS may maintain their own cache to speed up writes,
however this is lost on board reset (and may be lost on device removal
or reset by OS drivers).

Currently this can be worked around by "waiting for a while" after
writing data to disk, but of course this is not an acceptable solution.

Ideally U-Boot would have a mechanism to flush caches during board
reset, but until that logic is hooked up let's be sure that all writes
are actually propagated to the storage device so that we don't lose data
on board reset.

This is particularly noticeable during capsule updates, since the update
file is deleted and the board is reset immediately afterwards which
resulted in the same capsule update being applied over and over again.

This specifically fixes Qualcomm SDM845 devices with UFS 2.1, but likely
all UFS devices that use a cache.

---
Caleb Connolly (3):
      scsi: fix typo in setup_read_ext()
      scsi: de-dup calls to scsi_setup_write_ext()
      scsi: sync cache on write

 drivers/scsi/scsi.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 40 insertions(+), 7 deletions(-)
---
base-commit: a383b9bd4d7e430fe7c254297540bae596649dba

Caleb Connolly <caleb.connolly@linaro.org>

[PATCH 1/3] scsi: fix typo in setup_read_ext()

[Caleb Connolly]

This clears the 6th byte of cmd twice rather than setting the 9th byte
to 0. Fix it.

The only other command that sets the 9th byte is the 64-bit read, so
this likely never caused issues in practise.

Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 drivers/scsi/scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index cd0b84c062252118d250b9305728e03f61736600..34ac47c03ab397ca999abf130d84ccbd3be4c419 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -89,9 +89,9 @@ static void scsi_setup_read_ext(struct scsi_cmd *pccb, lbaint_t start,
 	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[6] = 0;
+	pccb->cmd[9] = 0;
 	pccb->cmdlen = 10;
 	pccb->msgout[0] = SCSI_IDENTIFY; /* NOT USED */
 	debug("scsi_setup_read_ext: cmd: %02X %02X startblk %02X%02X%02X%02X blccnt %02X%02X\n",
 	      pccb->cmd[0], pccb->cmd[1],

-- 
2.49.0

[PATCH 2/3] scsi: de-dup calls to scsi_setup_write_ext()

[Caleb Connolly]

This gets called with the same parameter in two paths in scsi_write(),
de-dup them to prepare for additional logic after the write.

Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 drivers/scsi/scsi.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 34ac47c03ab397ca999abf130d84ccbd3be4c419..1aa7fbdbb5278e387de72a3c3e73d19ea0342fff 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -220,25 +220,30 @@ static ulong scsi_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
 		pccb->dma_dir = DMA_TO_DEVICE;
 		if (blks > max_blks) {
 			pccb->datalen = block_dev->blksz * max_blks;
 			smallblks = max_blks;
-			scsi_setup_write_ext(pccb, start, smallblks);
-			start += max_blks;
-			blks -= max_blks;
 		} else {
 			pccb->datalen = block_dev->blksz * blks;
 			smallblks = (unsigned short)blks;
-			scsi_setup_write_ext(pccb, start, smallblks);
-			start += blks;
-			blks = 0;
 		}
+
 		debug("%s: startblk " LBAF ", blccnt %x buffer %lx\n",
 		      __func__, start, smallblks, buf_addr);
+		scsi_setup_write_ext(pccb, start, smallblks);
+
 		if (scsi_exec(bdev, pccb)) {
 			scsi_print_error(pccb);
 			blkcnt -= blks;
 			break;
 		}
+
+		if (blks > max_blks) {
+			start += max_blks;
+			blks -= max_blks;
+		} else {
+			start += blks;
+			blks = 0;
+		}
 		buf_addr += pccb->datalen;
 	} while (blks != 0);
 	debug("%s: end startblk " LBAF ", blccnt %x buffer %lX\n",
 	      __func__, start, smallblks, buf_addr);

-- 
2.49.0

[PATCH 3/3] scsi: sync cache on write

[Caleb Connolly]

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.connolly@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);
+		if (scsi_exec(bdev, pccb)) {
+			scsi_print_error(pccb);
+			break;
+		}
+
 		if (blks > max_blks) {
 			start += max_blks;
 			blks -= max_blks;
 		} else {

-- 
2.49.0

Re: [PATCH 1/3] scsi: fix typo in setup_read_ext()

[Neil Armstrong]

On 25/03/2025 14:02, Caleb Connolly wrote:
> This clears the 6th byte of cmd twice rather than setting the 9th byte
> to 0. Fix it.
> 
> The only other command that sets the 9th byte is the 64-bit read, so
> this likely never caused issues in practise.
> 
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>   drivers/scsi/scsi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index cd0b84c062252118d250b9305728e03f61736600..34ac47c03ab397ca999abf130d84ccbd3be4c419 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -89,9 +89,9 @@ static void scsi_setup_read_ext(struct scsi_cmd *pccb, lbaint_t start,
>   	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[6] = 0;
> +	pccb->cmd[9] = 0;
>   	pccb->cmdlen = 10;
>   	pccb->msgout[0] = SCSI_IDENTIFY; /* NOT USED */
>   	debug("scsi_setup_read_ext: cmd: %02X %02X startblk %02X%02X%02X%02X blccnt %02X%02X\n",
>   	      pccb->cmd[0], pccb->cmd[1],
> 

Good catch


Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH 3/3] scsi: sync cache on write

[Neil Armstrong]

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.connolly@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() ?

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

Neil

Re: [PATCH 2/3] scsi: de-dup calls to scsi_setup_write_ext()

[Neil Armstrong]

On 25/03/2025 14:02, Caleb Connolly wrote:
> This gets called with the same parameter in two paths in scsi_write(),
> de-dup them to prepare for additional logic after the write.
> 
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>   drivers/scsi/scsi.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 34ac47c03ab397ca999abf130d84ccbd3be4c419..1aa7fbdbb5278e387de72a3c3e73d19ea0342fff 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -220,25 +220,30 @@ static ulong scsi_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
>   		pccb->dma_dir = DMA_TO_DEVICE;
>   		if (blks > max_blks) {
>   			pccb->datalen = block_dev->blksz * max_blks;
>   			smallblks = max_blks;
> -			scsi_setup_write_ext(pccb, start, smallblks);
> -			start += max_blks;
> -			blks -= max_blks;
>   		} else {
>   			pccb->datalen = block_dev->blksz * blks;
>   			smallblks = (unsigned short)blks;
> -			scsi_setup_write_ext(pccb, start, smallblks);
> -			start += blks;
> -			blks = 0;
>   		}
> +
>   		debug("%s: startblk " LBAF ", blccnt %x buffer %lx\n",
>   		      __func__, start, smallblks, buf_addr);
> +		scsi_setup_write_ext(pccb, start, smallblks);
> +
>   		if (scsi_exec(bdev, pccb)) {
>   			scsi_print_error(pccb);
>   			blkcnt -= blks;
>   			break;
>   		}
> +
> +		if (blks > max_blks) {
> +			start += max_blks;
> +			blks -= max_blks;
> +		} else {
> +			start += blks;
> +			blks = 0;
> +		}

Ok this changes the logic and the result of blkcnt on error, before
is accounted the current command in the blkcnt but now it doesn't.

Weird we do not return any errors

>   		buf_addr += pccb->datalen;
>   	} while (blks != 0);
>   	debug("%s: end startblk " LBAF ", blccnt %x buffer %lX\n",
>   	      __func__, start, smallblks, buf_addr);
> 

Re: [PATCH 2/3] scsi: de-dup calls to scsi_setup_write_ext()

[Caleb Connolly]

On 3/25/25 14:38, Neil Armstrong wrote:
> On 25/03/2025 14:02, Caleb Connolly wrote:
>> This gets called with the same parameter in two paths in scsi_write(),
>> de-dup them to prepare for additional logic after the write.
>>
>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>> ---
>>   drivers/scsi/scsi.c | 17 +++++++++++------
>>   1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>> index 
>> 34ac47c03ab397ca999abf130d84ccbd3be4c419..1aa7fbdbb5278e387de72a3c3e73d19ea0342fff 100644
>> --- a/drivers/scsi/scsi.c
>> +++ b/drivers/scsi/scsi.c
>> @@ -220,25 +220,30 @@ static ulong scsi_write(struct udevice *dev, 
>> lbaint_t blknr, lbaint_t blkcnt,
>>           pccb->dma_dir = DMA_TO_DEVICE;
>>           if (blks > max_blks) {
>>               pccb->datalen = block_dev->blksz * max_blks;
>>               smallblks = max_blks;
>> -            scsi_setup_write_ext(pccb, start, smallblks);
>> -            start += max_blks;
>> -            blks -= max_blks;
>>           } else {
>>               pccb->datalen = block_dev->blksz * blks;
>>               smallblks = (unsigned short)blks;
>> -            scsi_setup_write_ext(pccb, start, smallblks);
>> -            start += blks;
>> -            blks = 0;
>>           }
>> +
>>           debug("%s: startblk " LBAF ", blccnt %x buffer %lx\n",
>>                 __func__, start, smallblks, buf_addr);
>> +        scsi_setup_write_ext(pccb, start, smallblks);
>> +
>>           if (scsi_exec(bdev, pccb)) {
>>               scsi_print_error(pccb);
>>               blkcnt -= blks;
>>               break;
>>           }
>> +
>> +        if (blks > max_blks) {
>> +            start += max_blks;
>> +            blks -= max_blks;
>> +        } else {
>> +            start += blks;
>> +            blks = 0;
>> +        }
> 
> Ok this changes the logic and the result of blkcnt on error, before
> is accounted the current command in the blkcnt but now it doesn't.

Ooh good point, will fix that in v2.
> 
> Weird we do not return any errors

Yeahh and scsi_print_error() is just a stub... This is probably 
something worth looking at in the future.
> 
>>           buf_addr += pccb->datalen;
>>       } while (blks != 0);
>>       debug("%s: end startblk " LBAF ", blccnt %x buffer %lX\n",
>>             __func__, start, smallblks, buf_addr);
>>
> 

-- 
Caleb (they/them)

Re: [PATCH 3/3] scsi: sync cache on write

[Caleb Connolly]

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.connolly@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.
> 
>> +        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)

Re: [PATCH 3/3] scsi: sync cache on write

[neil.armstrong]

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

I think the last one is just simpler and safer.

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

Re: [PATCH 3/3] scsi: sync cache on write

[Caleb Connolly]

On 3/25/25 16:18, neil.armstrong@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.connolly@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)