Patchwork [03/10] nandwrite: consolidate buffer usage

login
register
mail settings
Submitter Brian Norris
Date Aug. 31, 2011, 8 p.m.
Message ID <1314820839-7107-4-git-send-email-computersforpeace@gmail.com>
Download mbox | patch
Permalink /patch/112671/
State Accepted
Commit bf01f2960ba82468b1b25f00e044fd0c3ee0770a
Headers show

Comments

Brian Norris - Aug. 31, 2011, 8 p.m.
Instead of using two different output buffers for OOB data, let's just
use the same one for all output. This adds an extra memcpy, but it
simplifies some future work, so it's worth it.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 nandwrite.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
Artem Bityutskiy - Sept. 11, 2011, 1:07 p.m.
On Wed, 2011-08-31 at 13:00 -0700, Brian Norris wrote:
> Instead of using two different output buffers for OOB data, let's just
> use the same one for all output. This adds an extra memcpy, but it
> simplifies some future work, so it's worth it.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Also pushed this one, thanks!
Mike Frysinger - Sept. 14, 2011, 5:15 a.m.
On Wed, Aug 31, 2011 at 16:00, Brian Norris wrote:
> Instead of using two different output buffers for OOB data, let's just
> use the same one for all output. This adds an extra memcpy, but it
> simplifies some future work, so it's worth it.

could it be done by pulling out the pointer ?  make oobbuf a "char *",
rename existing oobbuf to like "char _oobbuf[]", and then assign
oobbuf to the relevant buffer and assume it's always set ...
-mike
Brian Norris - Sept. 14, 2011, 6:22 p.m.
On Wed, Sep 14, 2011 at 1:15 AM, Mike Frysinger <vapier.adi@gmail.com> wrote:
> On Wed, Aug 31, 2011 at 16:00, Brian Norris wrote:
>> Instead of using two different output buffers for OOB data, let's just
>> use the same one for all output. This adds an extra memcpy, but it
>> simplifies some future work, so it's worth it.
>
> could it be done by pulling out the pointer ?  make oobbuf a "char *",
> rename existing oobbuf to like "char _oobbuf[]", and then assign
> oobbuf to the relevant buffer and assume it's always set ...

It could be done this way, but actually, this patch was not intended
to stand alone; it was a precursor to removing one of the buffers from
nandwrite.c, as done in patch 07/10:
   mtdutils: move OOB auto-layout into libmtd's mtd_write
From the comment in patch 07:
   "This patch also cleans up a need for an extra OOB buffer in nandwrite."

So I'd actually recommend that Artem push this patch (patch 03) out of
'master' and into the 'brian' branch, then we can review the final
product there. In the end, I intended nandwrite to only have a single
buffer for OOB, instead of having a read buffer and an oob buffer. We
would simply pass our unmodified OOB data to mtd_write() and then to
the new MEMWRITE ioctl, where the kernel would do AUTO or PLACE
layouts for us depending on the mode we chose.

Of course, this is just the ideal approach; many kernels will not
support MEMWRITE yet, so they would fall back to the old code. That's
what patch 07 did; it cut out one buffer and moved the old layout code
into its own libmtd function, legacy_auto_oob_layout().
legacy_auto_oob_layout allocates its own buffer when needed; this part
is very inefficient (and actually has a memory leak now that I look at
it...) Perhaps the "legacy" handling should be reviewed a little more
to prevent too many trivial buffers and memcpy's.

If nothing else, I will at least need to send a v2 for patch 07 so we
free `tmp_buf' in `legacy_auto_oob_layout()'. But it'd be better just
to completely rework that function's buffers...

Brian
Mike Frysinger - Sept. 18, 2011, 2:53 a.m.
On Wed, Sep 14, 2011 at 14:22, Brian Norris wrote:
> On Wed, Sep 14, 2011 at 1:15 AM, Mike Frysinger wrote:
>> On Wed, Aug 31, 2011 at 16:00, Brian Norris wrote:
>>> Instead of using two different output buffers for OOB data, let's just
>>> use the same one for all output. This adds an extra memcpy, but it
>>> simplifies some future work, so it's worth it.
>>
>> could it be done by pulling out the pointer ?  make oobbuf a "char *",
>> rename existing oobbuf to like "char _oobbuf[]", and then assign
>> oobbuf to the relevant buffer and assume it's always set ...
>
> It could be done this way, but actually, this patch was not intended
> to stand alone; it was a precursor to removing one of the buffers from
> nandwrite.c

if the useless memcpy is removed by way of future patches, then this is fine
-mike
Brian Norris - Sept. 19, 2011, 6:50 p.m.
On Sat, Sep 17, 2011 at 7:53 PM, Mike Frysinger <vapier.adi@gmail.com> wrote:
> if the useless memcpy is removed by way of future patches, then this is fine

It's not completely removed. Please see patch 07.

I'll summarize: the memcpy is moved out of the main code path so that
if ioctl(MEMWRITE) is supported in your kernel, you have no extra
memcpy's. However, I didn't optimize the original code, which uses the
standard write+ioctl(MEMWRITEOOB[64]); it still has the useless memcpy
that we are discussing.

I'm not sure if/how your solution can work embedded at that level of
libmtd. Perhaps some functions I moved around need to be restructured
a bit.

Brian

Patch

diff --git a/nandwrite.c b/nandwrite.c
index 3f70cb3..3eea6e2 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -531,11 +531,12 @@  int main(int argc, char * const argv[])
 							oobreadbuf + start,
 							len);
 				}
+			} else {
+				memcpy(oobbuf, oobreadbuf, mtd.oob_size);
 			}
 			/* Write OOB data first, as ecc will be placed in there */
 			if (mtd_write_oob(mtd_desc, &mtd, fd, mtdoffset,
-						mtd.oob_size,
-						noecc ? oobreadbuf : oobbuf)) {
+						mtd.oob_size, oobbuf)) {
 				sys_errmsg("%s: MTD writeoob failure", mtd_device);
 				goto closeall;
 			}