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

[Steve Rae]

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

> On Fri, Apr 8, 2016 at 12:39 AM, Steve Rae <steve.rae at broadcom.com> wrote:
> >
> >
> > 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"?
>
> It's not the point. The default behavior can be any of those two, I
> basically meant "previous" or "another" behavior.
>
> > I would argue that the default behavior should be "align with
> > "ep->maxpacket" size"!
>
> For the sake of argument, we don't know for sure which behavior is
> correct, because it's not documented.
>
> > 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.
> >
>
> The only actual documentation for fastboot protocol I found is [1],
> and I don't see any mention of alignment there at all. So it wouldn't
> surprise me if that patch was done just out of of empiric
> observations. Which doesn't make it any less significant.
>
> Please review [2], as it can be related. Especially this part of commit
> message:
>
> USB 2.0 specification, section 8.5.3.2 says:
>
> " 8.5.3.2 Variable-length Data Stage A control pipe may have a
> variable-length data phase in which the host requests more data than
> is contained in the specified data structure. When all of the data
> structure is returned to the host, the function should indicate that
> the Data stage is ended by returning a packet that is shorter than the
> MaxPacketSize for the pipe. If the data structure is an exact multiple
> of wMaxPacketSize for the pipe, the function will return a zero-length
> packet to indicate the end of the Data stage. "
>
> I'm too sleepy right now to interpret information and make any
> conclusions, but maybe we should check how the end of Data Stage is
> implemented for different UDC controllers in U-Boot. TI boards use
> DWC3 controller. Which is yours?
>

DWC2


>
> [1]
> https://android.googlesource.com/platform/system/core/+/master/fastboot/fastboot_protocol.txt
> [2]
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=36f84ffb45c75ff10442a9f8f2fd91dcf6c6f5dd
>
> >
> >>
> >> 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
> >> >
> >> >
> >
> >
>