| Message ID | 1314820839-7107-4-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | Accepted |
| Commit | bf01f2960ba82468b1b25f00e044fd0c3ee0770a |
| Headers | show |
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!
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
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
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
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
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; }
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(-)