[U-Boot] arm: sunxi: Bananapi_M2_Ultra not working with DM_MMC

Mon Apr 8 14:30:56 UTC 2019

Hi,

Le lundi 08 avril 2019 à 19:47 +0530, Jagan Teki a écrit :
> On Mon, Apr 8, 2019 at 7:31 PM Paul Kocialkowski
> <paul.kocialkowski at bootlin.com> wrote:
> > Hi,
> > 
> > Le lundi 08 avril 2019 à 19:21 +0530, Jagan Teki a écrit :
> > > On Mon, Apr 8, 2019 at 7:06 PM Paul Kocialkowski
> > > <paul.kocialkowski at bootlin.com> wrote:
> > > > Hi,
> > > > 
> > > > Le lundi 08 avril 2019 à 19:03 +0530, Jagan Teki a écrit :
> > > > > On Mon, Apr 8, 2019 at 6:40 PM Paul Kocialkowski
> > > > > <paul.kocialkowski at bootlin.com> wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > Le lundi 08 avril 2019 à 18:23 +0530, Jagan Teki a écrit :
> > > > > > > Hi Paul,
> > > > > > > 
> > > > > > > On Mon, Apr 8, 2019 at 6:00 PM Paul Kocialkowski
> > > > > > > <paul.kocialkowski at bootlin.com> wrote:
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > On Thu, 2019-04-04 at 05:51 -0300, Pablo Sebastián Greco wrote:
> > > > > > > > > A few days ago I tried to boot my Bananapi_M2_Ultra with 2019.04rc, I
> > > > > > > > > found that it wasn't booting, 2019.01 was working ok.
> > > > > > > > > Bisecting indicated that the problem was after
> > > > > > > > > http://git.denx.de/?p=u-boot.git;a=commitdiff;h=a7cca5793774ee139b75a704d6efaa4d29f09f93
> > > > > > > > 
> > > > > > > > I think the patch should be reverted ASAP since it obviously breaks
> > > > > > > > some supported configs. Sadly, the offending commit doesn't say
> > > > > > > > anything about the test coverage for the change and what the status is
> > > > > > > > after it. There is probably a reason why it was enabled for sun4i only
> > > > > > > > before and there must have been a motivation for doing this on all
> > > > > > > > sunxi platforms, but then again, the commit message says nothing about
> > > > > > > > those underlying reasons.
> > > > > > > > 
> > > > > > > > I believe we should be more strict on patch review and not let any
> > > > > > > > change bringing such a major change get applied with a commit message
> > > > > > > > that provides no context about why the change is okay and how it was
> > > > > > > > tested.
> > > > > > > 
> > > > > > > Appropriate your concern.
> > > > > > > 
> > > > > > > If you please list what all boards are not working with this effect,
> > > > > > > please write back. we will defiantly look into it. All these changes
> > > > > > > were merged in MW which is 2.5 months back, commenting in final stage
> > > > > > > like this is not the professional way.
> > > > > > 
> > > > > > I really do not think this is a sane approach to follow. You can't make
> > > > > > a change like this, with no context whatsoever in the commit message,
> > > > > > which ends up breaking other people's setups and wait for others to
> > > > > > debug subsequent issues it introduces that you don't encounter.
> > > > > > 
> > > > > > Sorry but your commit should never have been merged. Sure, I wasn't
> > > > > > there to review it either, but the code review process definitely did
> > > > > > not go as planned here.
> > > > > 
> > > > > Which commit message your referring to? are you referring this
> > > > > patch[1] commit message. let me point what exactly is the issue?
> > > > > 
> > > > > [1] http://git.denx.de/?p=u-boot.git;a=commit;h=a7cca5793774ee139b75a704d6efaa4d29f09f93
> > > > 
> > > > Yes, this is the patch I'm talking about.
> > > > 
> > > > The issue with it is that the commit message is totally redundant: the
> > > > description does not say more than what the diff does.
> > > > 
> > > > A git commit description should provide context about what the change
> > > > does above the modified lines: what problem it tries to resolve and how
> > > > , on what hardware, how it was tested, etc. Especially for a commit
> > > > making such a big change, the commit message must have all this
> > > > information.
> > > > 
> > > > Do you see why I think it's a problem?
> > > 
> > > I can't agree if we consider the series of changes in one set. As I
> > > mentioned in another mail, this patch is last from the DM_MMC
> > > migration series and as the last one it enable the DM_MMC global to
> > > Allwinner (not respective to board). The previous patches are trying
> > > to support and fix DM_MMC on respective SoC's and board. ie the reason
> > > I didn't mention any thing related to board or any other information
> > > since It is global SoC change.
> > 
> > I don't think this is relevant. You can't expect people to go through
> > the list archive, find which series this commit was attached to,
> > etc. Especially when running a bisect, where you'll end up with a
> > single offending commit.
> > 
> > You need to make sure that each commit message provides context about
> > what it's doing, and not just rephrase the diff.
> 
> Do you still think this message shows the diff? The change is as
> simple as enable DM_MMC for Allwinner and doesn't make any direct
> changes to underlying boards on the same change.
> 
> "Enable DM_MMC for all Allwinner SoCs, this will eventually
> enable BLK."

Of course! Enabling DM_MMC is a major change to Allwinner MMC support
and it is not "as simple as enable DM_MMC for Allwinner". You need to
think about the context and implications of the change, beyond the
change itself and present that. That's the whole point of having git
commit messages: to present changes and their implications.

I don't see how I could make myself any more clear here, so let me give
you a simple example. Consider the following code:

void foo(int *bar, int *baz)
{
	if (bar && baz)
		*bar = *baz;
	else if (baz)
		*baz = 2;
}

Now we change that to:

void foo(int *bar, int *baz)
{
	if (bar)
		*bar = *baz;
	else if (baz)
		*baz = 2;
}

Here is one commit message about it:

"Remove check before dereferencing baz

The check on baz is removed from the if."

Here is another commit message about it:

"Remove check before dereferencing baz

The core (which is by design the only caller to this function)
guarantees that whenever foo is called and bar is non-NULL, baz is also
a valid pointer. Drop the check on baz since it is redundant."

Now in the first case, we have no context or explanation and we have no
idea why the change is okay. In the second one, we know precisely why
we can remove the check on baz and why it's safe to reference it.

Without knowing this, it's impossible to tell if the patch is just
wrong (it removes a check on a pointer before dereferencing it) or if
it is legitimate and makes sense.

Do you see my point?

> > In order words, it's *never* okay to just re-work the diff, you
> > *always* need to describe the context. That's not something optional to
> > only do on special occasions.
> 
> Mentioning again, I don't see any point to mention board information
> for this commit since the diff or change clearly focusing on global
> Allwinner platform enablement for DM_MMC and the same mentioned in the
> body.

That's the point: of course you don't see why need to explain things
because they are already obvious to you as the author of the path. You
must think about everyone else who is not you and who will read the
patch with no other context. They must be able to understand what is
going on just by reading the patch. This is one of the most important
areas to contributing to public free software projects and it is what
allows us to keep sane code bases, understand previous decisions, etc.

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com