Re: [RFT PATCH 2/2] mtd: cfi-flash: call ctrlc() during CFI reads

[Christian Melki <christian.melki@t2data.com>]

On 5/22/23 10:38 AM, Ahmad Fatoum wrote:
> On 22.05.23 07:25, Ahmad Fatoum wrote:
>> Memory mapped flash access can be quite slow on older processors. For
>> writing and erasing, we already call resched() indirectly to feed the
>> watchdog, so let's do this when reading as well. This fixes an issue
>> of short running watchdogs triggering on PowerPC, because kernel boot
>> takes too long.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>   drivers/mtd/nor/cfi_flash.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nor/cfi_flash.c b/drivers/mtd/nor/cfi_flash.c
>> index f1555a72a42e..10542c710118 100644
>> --- a/drivers/mtd/nor/cfi_flash.c
>> +++ b/drivers/mtd/nor/cfi_flash.c
>> @@ -25,7 +25,9 @@
>>   #include <io.h>
>>   #include <errno.h>
>>   #include <progress.h>
>> +#include <string.h>
>>   #include <linux/err.h>
>> +#include <linux/sizes.h>
>>   #include <asm/unaligned.h>
>>   #include <linux/mtd/concat.h>
>>   #include "cfi_flash.h"
>> @@ -891,10 +893,16 @@ static int cfi_mtd_read(struct mtd_info *mtd, loff_t from, size_t len,
>>   		size_t *retlen, u8 *buf)
>>   {
>>   	struct flash_info *info = container_of(mtd, struct flash_info, mtd);
>> +	const void *src = info->base + from;
>> +	size_t i;
>> +
>> +	for (i = 0; i < len; i = size_add(i, SZ_1M)) {
>> +		buf = mempcpy(buf, src + i, min_t(size_t, len, SZ_1M));
>> +		if (ctrlc())
>> +			return -EINTR;
> 
> Christian mentioned in IRC that it may be surprising to users that a boot could be
> aborted with ctrlc(). Thoughts? Also, the POSIXy thing to do is to return a short
> read count, but I didn't do that, because I have not vetted that everything handles
> that gracefully.. (read_file e.g. does, but I don't know about others).
> 

I think short read handling is the way to go.
Did that in my local version, but yeah. I didn't check all callers either. :)

> Cheers,
> Ahmad
> 

I have a solution here where I have a somewhat constrained boot flow and the console is still available for some initial choices.
So looking at this, I think that from a developer or just bench style perspective, a boot that can be aborted by default is a nice feature.
But from a immutable BareBox or a constrained/schematic boot flow perspective, this might come as a surprise.

Imho, I don't think ctrl-c checks belong in any lower layer.
Given the current BB design, I think they should make sure they don't hold the CPU for to long(reading, writing, erasing, crypto, hashing, compression etc).
Then the caller (some resonable high level) can then decide if they want to check whatever.
But even a resched is a pretty large take on "not holding the CPU" as my primary focus here was the WD.

Maybe there is no pretty way around this.

Regards,
Christian

>> +	}
>>   
>> -	memcpy(buf, info->base + from, len);
>>   	*retlen = len;
>> -
>>   	return 0;
>>   }
>>   
>