[U-Boot] [PATCH v4 0/3] mtd, ubi, ubifs: resync with Linux-3.14

[Scott Wood]

On Wed, 2014-06-18 at 09:55 +0200, Heiko Schocher wrote:
> Hello Scott,
> 
> Am 17.06.2014 17:54, schrieb Scott Wood:
> > On Tue, 2014-06-17 at 09:15 +0200, Heiko Schocher wrote:
> >> - Following Code in drivers/mtd/nand/nand_base.c nand_do_write_ops()
> >>    adapted for U-Boot:
> >>
> >>    +#ifndef __UBOOT__
> >>          /* Reject writes, which are not page aligned */
> >>          if (NOTALIGNED(to) || NOTALIGNED(ops->len)) {
> >>    +else
> >>    +     /* Reject writes, which are not page aligned */
> >>    +     if (NOTALIGNED(to)) {
> >>    +endif
> >
> > This sort of ifdeffing is a bad idea -- if a larger number of lines are
> > ifdeffed, to avoid context conflicts, subsequent merges can silently put
> > updates into the non-U-Boot section that are needed in the U-Boot
> > section.
> 
> Yes, in general I agree, this is a bad idea. But I introduced it
> here to get all differences between U-Boot and Linux marked with
> the "_UBOOT_" define, so a future sync with linux should be easier ...

Again, I don't think having the differences be marked with "#ifdef
__UBOOT__" is a good thing.

> > Plus, it impairs readability, and in this case breaks text-editor
> > brace-matching.
> 
> Yes, could be fixed, see below.
> 
> > Plus plus, that line has been that way in Linux since 2006 -- why is it
> > being touched at all?  Did you merge in changes from the previous mtd
> > sync, or did you just copy the new files over and then try to fix
> > resulting problems? (answered below)
> 
> Yes. Above Code is a good example. In Linux this piece of code looks like:
> -------------
> static int nand_do_write_ops(struct mtd_info *mtd, loff_t to,
>                               struct mtd_oob_ops *ops)
> [...]
>          ops->retlen = 0;
>          if (!writelen)
>                  return 0;
> 
>          /* Reject writes, which are not page aligned */
>          if (NOTALIGNED(to) || NOTALIGNED(ops->len)) {
>                  pr_notice("%s: attempt to write non page aligned data\n",
>                             __func__);
>                  return -EINVAL;
>          }
> 
>          column = to & (mtd->writesize - 1);
> -------------
> 
> in current U-Boot:
> 
> -------------
> static int nand_do_write_ops(struct mtd_info *mtd, loff_t to,
>                               struct mtd_oob_ops *ops)
> [...]
> 
>          ops->retlen = 0;
>          if (!writelen)
>                  return 0;
> 
>          column = to & (mtd->writesize - 1);
> -------------
> 
> Why is this "if (NOTALIGNED(to) || NOTALIGNED(ops->len)) {"
> missing in U-Boot? Do we allow "not aligned writes" in U-Boot?

The block skipping accessors have their own enforcement of non-aligned
accesses.   Other than that, non-aligned accesses should work.

> If so, we should have documented this, and best place for it,
> I think, would be with a comment in the source code...

You could look at the commit history where the code was altered, though
in some cases a comment may be warranted.  I don't think that comment
should take the form of the Linux code under an ifndef.

> Above Code is a good example for that. Linux has this code
> since 2006, why has U-Boot not this code?

It was removed by commit f9a5254111a6be2a39464f65a96f4fc2305e3c76, which
moved the check into the block-skipping accessors.

> > history to see what Linux version corresponded to the last mtd sync, and
> > generate a diff relative to that.
> 
> I think, we should have the linux code as the reference, and on top
> of this, we should add our U-Boot changes ... Thats the reason
> for the git tree [1].

One of the strengths of version control is the ability to do three-way
merges, rather than throw away the downstream code and reassemble it
from scratch.

What does #ifndef __UBOOT__ tell you that a diff between the U-Boot and
Linux files, as of the SHA1 that was last merged into U-Boot, doesn't
tell you?

> There we have the "simple copy from linux patch" [2]. It overwrites
> the U-Boot specific changes, yes, but only in an intermediate step
> in this git tree ... (Maybe I should add in the commit message, that
> this is a simple copy from linux sources)
> 
> Then a U-Boot specifc patch [3], which introduces again hopefully
> all U-Boot specific changes... so *all* U-Boot specific changs are
> documented. At last a seperate licencse file patch [4] added which
> changes the license text.
> 
> My hope was, that future linux mtd syncs could look like:
> 
> a) copy new linux source files to u-boot tree [1] (make a new branch
>     go back to commit [2] ... and copy new files from linux)
>     (commit this, to have all linux changes since last sync
>      with linux documented)
> b) apply [3] and fix problems
>     All U-Boot changes documented
> c) apply [4] and fix problems
> d) add newly added U-Boot specific patches
>     (mark them with _UBOOT_ define if they are not yet marked with it)
>     (maybe squash them to step b ?)
> 
>     -> new git tree ready
> 
> e) squash all patches into one patch and send it to mainline ...
>     so the mainline patch has only new changes with all U-Boot
>     specific changes.

If you take over as NAND custodian, you can of course do the updates as
you please (unless Tom or Wolfgang disagree), but until then I insist on
doing a proper merge, and not littering the code with #ifndef __UBOOT__.

> Thats sounds easy ... and you have all linux and all U-Boot
> changes visible and documented ... I must admit, I did not
> tried your proposal to look into the linux changes since
> the last mtd sync ... but that sounds the more tricky way
> to get this sync, as the history can be long ... and shouldn;t
> be the result the same?

Why does the length of the history matter?  I'm not asking you to go
patch by patch.  Just take a diff between the SHA1 of last merge and the
SHA1 you want to merge now, and apply that patch.

The results will only be the same (ignoring the ifdefs you want to add)
if no errors are made, and I find your process more error prone.  E.g.
how do you know you've fixed all problems (much less fixed them in the
same way so as to make the results the same, and reduce code churn)?  Do
you test every board U-Boot supports NAND on?  Every feature of U-Boot
code relating to NAND?  Limiting the update to things that have actually
changed in Linux since the previous sync reduces the likelihood of
regressions.

-Scott