From: Christian Melki <christian.melki@t2data.com>
To: Ahmad Fatoum <a.fatoum@pengutronix.de>, barebox@lists.infradead.org
Cc: jbe@pengutronix.de
Subject: Re: [RFT PATCH 2/2] mtd: cfi-flash: call ctrlc() during CFI reads
Date: Mon, 22 May 2023 11:21:38 +0200 [thread overview]
Message-ID: <4bf40dcc-5cab-ef34-5624-9d09bf08638f@t2data.com> (raw)
In-Reply-To: <a67a1e6e-6f82-0f0d-ac06-b25d42d11a1f@pengutronix.de>
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;
>> }
>>
>
prev parent reply other threads:[~2023-05-22 9:24 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-22 5:25 [RFT PATCH 1/2] include: <linux/overflow.h>: sync with kernel Ahmad Fatoum
2023-05-22 5:25 ` [RFT PATCH 2/2] mtd: cfi-flash: call ctrlc() during CFI reads Ahmad Fatoum
2023-05-22 8:38 ` Ahmad Fatoum
2023-05-22 9:21 ` Christian Melki [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4bf40dcc-5cab-ef34-5624-9d09bf08638f@t2data.com \
--to=christian.melki@t2data.com \
--cc=a.fatoum@pengutronix.de \
--cc=barebox@lists.infradead.org \
--cc=jbe@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox