[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Steve Rae]

On Thu, Apr 7, 2016 at 2:16 PM, Sam Protsenko <semen.protsenko at linaro.org>
wrote:

> On Thu, Apr 7, 2016 at 8:07 PM, Steve Rae <steve.rae at broadcom.com> wrote:
> > Hi Sam,
> >
> > On Thu, Apr 7, 2016 at 9:46 AM, Sam Protsenko <
> semen.protsenko at linaro.org>
> > wrote:
> >>
> >> On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski <
> l.majewski at samsung.com>
> >> wrote:
> >> > Hi Steve,
> >> >
> >> >> No -- I do not believe that this issue is caused by different
> fastboot
> >> >> (client) versions (the executable that runs on the host computer -
> >> >> Linux, Windows, Mac, etc.)
> >> >> I have personally attempted three (3) different versions, and the
> >> >> results are consistent.
> >> >>
> >> >> And no I don't think that I "am the only hope at fixing this proper"
> >> >> -- as you will see below,
> >> >> this" issue" seems to be unique to the "TI platforms" (... nobody
> else
> >> >> has stated they have an issue either way -- but I don't think many
> use
> >> >> this feature ....)
> >> >> So maybe someone with "TI platforms" could investigate this more
> >> >> thoroughly...
> >> >>
> >> >> HISTORY:
> >> >>
> >> >> The U-Boot code, up to Feb 25, worked properly on my Broadcom boards
> >> >> -- this code contains:
> >> >>                req->length = rx_bytes_expected();
> >> >>                 if (req->length < ep->maxpacket)
> >> >>                         req->length = ep->maxpacket;
> >> >> which aligned the remaining "rx_bytes_expected" to be aligned to the
> >> >> "ep->maxpacket" size.
> >> >>
> >> >> On Feb 25, there was a patch applied from <dileep.katta at linaro.org>
> >> >> which forces the remaining "rx_bytes_expected" to be aligned to the
> >> >> "wMaxPacketSize" size -- this patch broke all Broadcom boards:
> >> >> +       if (rx_remain < maxpacket) {
> >> >> +               rx_remain = maxpacket;
> >> >> +       } else if (rx_remain % maxpacket != 0) {
> >> >> +               rem = rx_remain % maxpacket;
> >> >> +               rx_remain = rx_remain + (maxpacket - rem);
> >> >> +       }
> >> >>
> >> >> After attempting to unsuccessfully contact Dileep, I requested that
> >> >> this patch be reverted -- because it broke my boards! (see the other
> >> >> email thread).
> >> >>
> >> >> Sam Protsenko <semen.protsenko at linaro.org> has stated that this Feb
> 25
> >> >> change is required to make "fastboot work on TI platforms".
> >> >>
> >> >> Thus,
> >> >> - Broadcom boards require alignment to "ep->maxpacket" size
> >> >> - TI platforms require alignment to "wMaxPacketSize" size
> >> >> And we seem to be at a stale-mate.
> >> >> Unfortunately, I do not know enough about the USB internals to
> >> >> understand why this change breaks the Broadcom boards; or why it _is_
> >> >> required on the TI platforms....
> >> >> ( Is there any debugging that can be turned on to validate what is
> >> >> happening at the lower levels? )
> >> >
> >> > I can only speak about DWC2 (from Synopsis) embedded at Samsung
> boards.
> >> > There are low level debugging registers (documented, but not supposed
> >> > to be used at normal operation), which give you some impression
> >> > regarding very low level events.
> >> >
> >> > DWC2 at Samsung is using those to work properly since we had some
> >> > problems with dwc2 IP blocks implementation on early Samsung
> >> > platforms :-). This approach works in u-boot up till now.
> >> >
> >> > Another option is to use JTAG debugger (like Lauterbach) to inspect
> >> > state of this IP block.
> >> >
> >> >> ( Can anyone explain why "wMaxPacketSize" size would be required? --
> >> >> my limited understanding of endpoints makes me think that
> >> >> "ep->maxpacket" size is actually the correct value! )
> >> >>
> >> >> I asked Sam to submit a patch which conditionally applied the
> >> >> alignment to "wMaxPacketSize" size change -- he stated that he was
> too
> >> >> busy right now -- so I submitted this patch on his behalf (although
> he
> >> >> still needs to add the Kconfig for the TI platforms in order to make
> >> >> his boards work)....
> >> >>
> >> >> I suppose I could also propose a patch where the condition _removes_
> >> >> this feature (and define it on the Broadcom boards)  -- do we
> >> >> generally like "negated" conditionals?
> >> >> +#ifndef
> >> >>
> >> >>
> CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_DISABLE_ALIGNMENT_WITH_WMAXPACKETSIZE
> >> >> Please advise!
> >> >>
> >> >> Further, how does the U-Boot community respond to a change which
> >> >> breaks something which is already working? Doesn't the "author" of
> >> >> that change bear any responsibility on assisting to get "their"
> change
> >> >> working properly with "all" the existing boards?
> >> >
> >> > As we know the author of this change is not working at Linaro anymore.
> >> >
> >> >> I'm getting the
> >> >> impression that "because the current code works for me", that I am
> not
> >> >> getting any assistance in resolving this issue -- which is why I
> >> >> suggested "reverting" this change back to the original code; that
> way,
> >> >> it would (politely?) force someone interested in "TI platforms" to
> >> >> step up and look into this....
> >> >>
> >> >> Sorry for asking so many questions in one email -- but I'd appreciate
> >> >> answers....
> >> >> ( I also apologize in advance for the "attitude" which is leaking
> into
> >> >> this email... )
> >> >> Please tell me what I can do! I had working boards; now they are all
> >> >> broken -- and I don't how how to get them working again....
> >> >
> >> > If you don't have enough time (and HW) for investigate the issue, I
> >> > think that Kconfig option with documentation entry is the way to go.
> >> >
> >> > I hope that Sam don't have any objections with such approach.
> >> >
> >>
> >> If this commit doesn't break any platform -- I'm ok with that. If it
> >> breaks anything (TI boards particularly) -- I'd ask to revert it at
> >> once, as this is I believe not right way to do things.
> >
> >
> > I'm confused...
> > You are saying that it is OK to checkin a change that fixes TI boards
> (Feb
> > 25), even though it breaks Broadcom boards;
> > but if _this_ change "breaks anything" then it is NOT OK ?????
> > ( I politely disagree.... )
> > PS - therefore - what is the right way? (..."this is I believe not right
> way
> > to do things"...)
> >
>
> Look, it's current state of things. Some stuff is broken, I admit
> that. But you can't just break something while fixing another stuff.
> It's not even about "your" boards or "my" boards. It's just not right,
> I thought it's pretty obvious. So what is correct way to do in that
> case? I believe it's fix only boards you know for sure are broken, but
> keep old fastboot behaviour for the rest of boards. Not only TI, but
> all boards except yours. So that after buildman run you can see that
> only your boards were changed, something like that.
>

I cannot agree with the assumptions that you are making -- their is no
evidence that "all boards except <mine>" where broken prior to the Feb 25
patch....


> >>
> >> So Steve, please add
> >> CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED option to all
> >> required defconfigs (except yours), so that your patch only fixes your
> >> platforms, but doesn't break any other platform at the same time.
> >
> >
> > So -- here is why I cannot complete this task:
> > I have absolutely no idea which boards actually _require_ this
> capability,
> > therefore, I have no idea which defconfigs I would need to update!
> >
>
> As I see it:
>
> - look into include/configs/*.h
> - find all headers that use fastboot capability
> - find corresponding TARGET_ for each header
> - find all defconfigs for each TARGET_
> - your defconfigs should disable alignment
> - rest of defconfigs should enable alignment (default behavior)
>

Why should the default behavior be "align with wMaxPacketSize"?
I would argue that the default behavior should be "align with "ep->maxpacket"
size"!
U-Boot history shows that "align with "ep->maxpacket" size" was the
original code;
then a patch was added to change it to "align with wMaxPacketSize" --
however, there is NO EXPLANATION given, other than the commit message:

    fastboot: OUT transaction length must be aligned to wMaxPacketSize

    OUT transactions must be aligned to wMaxPacketSize for each transfer,
    or else transfer will not complete successfully. This patch modifies
    rx_bytes_expected to return a transfer length that is aligned to
    wMaxPacketSize.

    Note that the value of wMaxPacketSize and ep->maxpacket may not be
    the same value, and it is the value of wMaxPacketSize that should be
    used for alignment. wMaxPacketSize is passed depending on the speed of
    connection.



> This way you fix your boards (that you know need to be fixed) but keep
> rest of boards intact. If some other boards need to be fixed too --
> they will be fixed further by some folks who need that.
>
> > So, if you would send me a list of which defconfig files need to add this
> > line, I'll update it....
> > OR (I would prefer) you could submit a v3 which includes the boards that
> you
> > know require this capability!
> >
>
> I'm not gonna send this patch for you, sorry. I just don't need that,
> and I'm not the author of original patch, so it's just not my concern.
> I can't even test it for boards that actually broken.
>
> > Let me know,
> > Thanks, Steve
> >
> >>
> >>  Also
> >> good thing to do after that is check options order in changed
> >> defconfigs with "make savedefconfig" rule. Both your current changes
> >> and appropriate lines in defconfigs should be committed as a single
> >> patch, so that it doesn't break anything and "git bisect" may be used
> >> to look for regressions. Also, really nice thing to do after all of
> >> this, is to use "./tools/buildman/buildman" tool to check all ARM
> >> boards for regressions after your patch (you should see that only your
> >> boards were changed).
> >
> >
> > yup -- I use buildman almost exclusively....
> >
> >>
> >> Ideally, we should check it on all boards (or at least on all UDC
> >> controllers supported in U-Boot) and figure out what is happening
> >> exactly. But I'm totally fine with hack if it fixes real problem on
> >> some platforms. I just ask you guys to not break anything else at the
> >> same time (although it surely takes much more effort, but still).
> >
> >
> > I'm confused (again) -- why are you asking: "you guys to not break
> anything
> > else"...
> > IT IS ALREADY BROKEN, it is broken right now, and has been broken since
> Feb
> > 25 !
> >
> > Please fix this!
> >
>
> ...So let's fix half of platforms and break the other half of
> platforms altogether? It's not for me to decide, I'm not the
> maintainer. But it just doesn't feel right to me.
>
> I understand your concern, and I can help you test your patches on my
> boards any time and also run some debug patches to see the difference.
> But I can't fix it for you. Also I'm not sure that your patch would be
> merged in current shape (it's basically a hack). So if I were you I'd
> try to figure out the root cause of this issue by comparing results of
> some debug patches and tests, by running them on your boards (where
> fastboot is broken) and on some boards where fastboot is working.
> Maybe running wireshark in both cases can help to understand why it's
> happening. From my POV it was a good assumption (made by someone
> earlier) that possible reason is different UDC controllers (I have
> DWC3 on my TI boards).
>
> >>
> >> >> Thanks, Steve
> >> >>
> >> >> On Wed, Apr 6, 2016 at 4:01 AM, Marek Vasut <marex at denx.de> wrote:
> >> >> > On 04/06/2016 07:35 AM, Steve Rae wrote:
> >> >> >>
> >> >> >> On Apr 5, 2016 3:07 PM, "Marek Vasut" <marex at denx.de
> >> >> >> <mailto:marex at denx.de>> wrote:
> >> >> >>>
> >> >> >>> On 04/05/2016 08:31 PM, Steve Rae wrote:
> >> >> >>> > commit 9e4b510 fastboot: OUT transaction length must be aligned
> >> >> >>> > to
> >> >> >> wMaxPacketSize
> >> >> >>> > breaks some boards...
> >> >> >>> >
> >> >> >>> > Therefore add a conditional Kconfig to optionally enable this
> >> >> >>> > feature.
> >> >> >>>
> >> >> >>> Did you drill into it to figure out why this is needed ?
> >> >> >>>
> >> >> >>
> >> >> >> Marek,
> >> >> >> Let me clarify....
> >> >> >> All my boards work with the original code (before the commit which
> >> >> >> aligned  the size to the wMaxPacketSize).... Since that commit,
> >> >> >> all my boards are broken.
> >> >> >> And you will notice in this patch, that none of my boards define
> >> >> >> this CONFIG_ ...
> >> >> >>
> >> >> >> So I think you are asking the wrong person to drill down into this
> >> >> >> issue.... Sorry, Steve
> >> >> >
> >> >> > Well who else can I ask ? You're our only hope at fixing this
> >> >> > proper.
> >> >> >
> >> >> > Anyway, see my other reply, maybe we should just add an arg to
> >> >> > fastboot command to select one more of operation or the other and
> >> >> > default to the one which works.
> >> >> >
> >> >> > --
> >> >> > Best regards,
> >> >> > Marek Vasut
> >> >>
> >> >
> >> >
> >> >
> >> > --
> >> > Best regards,
> >> >
> >> > Lukasz Majewski
> >> >
> >> > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
> >
> >
>