public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
From: Heiko Schocher <hs@denx.de>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH] mtd: nand: revive "nand scrub" command
Date: Tue, 16 Dec 2014 07:21:19 +0100	[thread overview]
Message-ID: <548FCF5F.9020809@denx.de> (raw)
In-Reply-To: <20141216012619.GJ20704@bill-the-cat>

Hello Tom, Scott,

Am 16.12.2014 02:26, schrieb Tom Rini:
> On Mon, Dec 15, 2014 at 05:08:16PM -0600, Scott Wood wrote:
>> On Mon, 2014-12-15 at 12:13 +0100, Marek Vasut wrote:
>>> On Monday, December 15, 2014 at 11:54:08 AM, Masahiro Yamada wrote:
>>>> On Fri, 12 Dec 2014 07:19:21 +0100
>>>>
>>>> Heiko Schocher <hs@denx.de> wrote:
>>>>> Hello Scott,
>>>>>
>>>>> Am 11.12.2014 22:43, schrieb Scott Wood:
>>>>>> On Thu, 2014-12-11 at 22:37 +0100, Marek Vasut wrote:
>>>>>>> On Thursday, December 11, 2014 at 09:37:10 PM, Scott Wood wrote:
>>>>>>>> On Thu, 2014-12-11 at 19:49 +0900, Masahiro Yamada wrote:
>>>>>>>>> Since commit ff94bc40af34 (mtd, ubi, ubifs: resync with Linux-3.14),
>>>>>>>>> the "nand scrub" command has not been working.
>>>>>>>>>
>>>>>>>>> The scrub is a U-Boot extension and we have to modify nand_base.c
>>>>>>>>> that originates in Linux.
>>>>>>>>>
>>>>>>>>> Mark the code with #ifdef __UBOOT__ so we will never accidentally
>>>>>>>>> drop it when we re-sync the NAND framework with Linux in the future.
>>>>>>>>
>>>>>>>> No more "#ifdef __UBOOT__" please.
>>>>>>>
>>>>>>> Do you happen to have a helpful suggestion how to clearly mark those
>>>>>>> bits of code then please ?
>>>>>>
>>>>>> This was already discussed. :-)
>>>>>>
>>>>>> See the archives for why I think this is bad.
>>>>>>
>>>>>>>> Instead, never again do a "start
>>>>>>>> from scratch" resync the way that the above commit was done.
>>>>>>>
>>>>>>> This was already discussed, no need to revive this topic here now.
>>>>>>
>>>>>> Sorry, but these patches fixing breakages that resulted from that merge
>>>>>> demonstrate that there is a need to revive it, if there's anyone that
>>>>>> still thinks it's a good idea -- Heiko seemed to be in agreement that
>>>>>> there's no need to do that for future syncs:
>>>>>> http://lists.denx.de/pipermail/u-boot/2014-November/194256.html
>>>>>
>>>>> Yes, I hope a resync works now fine ... but I prefer to mark the
>>>>> differences between linux and u-boot somehow, because, you immediately
>>>>> see the differences between linux and u-boot, when you read the u-boot
>>>>> code ...
>>>>
>>>> I agree with Heiko.
>>>
>>> I second that.
>>
>> If you or Heiko want to take over NAND custodianship, the offer is still
>> there.
>>
>> I find these ifdefs to be harmful to code legibility, editability, and
>> searchability.
>>
>> I find these ifdefs to be harmful to merging from Linux, especially when
>> the ifndef section is large enough that the ifndef itself doesn't
>> conflict with the diff context.  This can lead to silent mismerges if
>> the U-Boot alternative needed to be updated.
>>
>> I find these ifdefs to be unreliable, because not every change is going
>> to end up getting marked (e.g. commit 35c204d8a9d0).  This can be
>> actively harmful if people see some changes marked, and rely on the
>> markers rather than checking with a diff.  And yes, that means I need to
>> send a patch to strip out the ifdefs from NAND files, just like I did
>> back in commit 5b8e6bb517ea4.
>
> As we're neck deep in "See? Scott was right" I'm inclined to go with
> what Scott is saying.  We tried it, it actively didn't work, lets step
> back, evaluate and move along.

Ok, I have no objections against that, it was just my point of view ...

And yes, I missed here and there some pieces when syncing with
linux 3.14 ... sorry for that, but I could not test every scenario for
every board ... and we had no clean base for using git, which at last
in my attempt ended in a big mess when syncing with linux ... so I started
more or less from scratch ...

So ...

Who will send a "delete ifdef" patch?
Who will test, how a resync with linux (saying 3.17 ?) work now with git?

If this test is done, we can decide, which way is the "way to go" ... ?

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

  reply	other threads:[~2014-12-16  6:21 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-12-11 10:49 [U-Boot] [PATCH] mtd: nand: revive "nand scrub" command Masahiro Yamada
2014-12-11 20:37 ` Scott Wood
2014-12-11 21:37   ` Marek Vasut
2014-12-11 21:43     ` Scott Wood
2014-12-12  6:19       ` Heiko Schocher
2014-12-15 10:54         ` Masahiro Yamada
2014-12-15 11:13           ` Marek Vasut
2014-12-15 23:08             ` Scott Wood
2014-12-16  1:26               ` Tom Rini
2014-12-16  6:21                 ` Heiko Schocher [this message]
2014-12-16  6:24                   ` Scott Wood
2014-12-16  6:39                     ` Heiko Schocher
2014-12-16  6:41                     ` Masahiro Yamada

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=548FCF5F.9020809@denx.de \
    --to=hs@denx.de \
    --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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox