All of lore.kernel.org
 help / color / mirror / Atom feed
From: Frieder Schrempf <frieder.schrempf@kontron.de>
To: Michael Walle <michael@walle.cc>, Frieder Schrempf <frieder@fris.de>
Cc: u-boot@lists.denx.de, Heiko Thiery <heiko.thiery@gmail.com>,
	Bin Meng <bmeng.cn@gmail.com>,
	Jagan Teki <jagan@amarulasolutions.com>,
	Pratyush Yadav <p.yadav@ti.com>,
	Sean Anderson <seanga2@gmail.com>, Simon Glass <sjg@chromium.org>
Subject: Re: [PATCH] cmd: sf: Support unaligned flash updates with 'sf update'
Date: Thu, 30 Sep 2021 19:17:46 +0200	[thread overview]
Message-ID: <0b725bbb-0bc6-2ce3-d861-18bbd8f34059@kontron.de> (raw)
In-Reply-To: <92ca13e960f4e862c0fdd83373255301@walle.cc>

On 30.09.21 18:35, Michael Walle wrote:
> Am 2021-09-30 18:19, schrieb Frieder Schrempf:
>> In order to support unaligned updates, we simply read the first full
>> block and check only the requested part of the block for changes. If
>> necessary, the block is erased, the first (unchanged) part of the block
>> is written back together with the second part of the block (updated
>> data).
> 
> I'm not sure what I should think of this. You might loose that unchanged
> part if there is an power loss in the middle, even worse, you likely don't
> have this part anymore because it isn't part of the data you want to write
> to the flash.
> 
> Maybe add an parameter for allow (unsafe) unaligned updates?
> 
> Now you might argue, that with "sf erase, sf write" you can do the same
> harm, to which i reply: but then it is intentional ;) Because "sf erase"
> just works on sector boundaries (which isn't really checked in the command,
> i just realized, but at least my driver returns EINVAL) and then the
> user has to include the "unchanged part" for the arguments on the
> commandline.

True, but "sf update" already is "unsafe" even without supporting
unaligned start offsets. The code already handles partial sector writes
for the last sector in the same way (read, erase, write), which is also
prone to data loss in case of power loss.

So this patch in fact just adds support for partial sector updates for
the first sector. It is slightly more probable to loose data, but it
doesn't introduce new "unsafe" behavior.

Maybe we could cover this by adding a warning to the documentation, or
even printing a warning?

  reply	other threads:[~2021-09-30 17:17 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-30 16:19 [PATCH] cmd: sf: Support unaligned flash updates with 'sf update' Frieder Schrempf
2021-09-30 16:35 ` Michael Walle
2021-09-30 17:17   ` Frieder Schrempf [this message]
2021-09-30 21:08     ` Michael Walle
2021-10-04 10:31       ` Frieder Schrempf
2021-10-04 11:21         ` Michael Walle
2021-09-30 21:21 ` Michael Walle
2021-10-04 10:41   ` Frieder Schrempf
2021-10-01  5:03 ` Simon Glass

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=0b725bbb-0bc6-2ce3-d861-18bbd8f34059@kontron.de \
    --to=frieder.schrempf@kontron.de \
    --cc=bmeng.cn@gmail.com \
    --cc=frieder@fris.de \
    --cc=heiko.thiery@gmail.com \
    --cc=jagan@amarulasolutions.com \
    --cc=michael@walle.cc \
    --cc=p.yadav@ti.com \
    --cc=seanga2@gmail.com \
    --cc=sjg@chromium.org \
    --cc=u-boot@lists.denx.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.