On 10/04/18 11:24, Leif Lindholm wrote: > +Peter > > On Wed, Oct 03, 2018 at 04:59:54PM -0700, aaron.yo...@oracle.com wrote: >> I am suspecting that this patch to GRUB is the cause of a Buffer being >> re-transmitted before reaping the Buffer via SNP->GetStatus(): >> >> https://git.centos.org/blob/rpms!grub2.git/1065bd29e776aef83f927747882140dcb6fd5cde/SOURCES!0183-efinet-retransmit-if-our-device-is-busy.patch >> >> So, to reproduce the issue, the GRUB used via PXE boot needs to include >> this patch. > > So the issue cannot be reproduced with upstream GRUB? > > Does Fedora/Red Hat include the same patch?
Here's what I can see. (1) In upstream grub <https://git.savannah.gnu.org/git/grub.git>, at commit 8ada906031d9 ("msdos: Fix overflow in converting partition start and length into 512B blocks", 2018-09-27), on the master branch, the patch is not present. (2) In "rhboot" grub2 <https://github.com/rhboot/grub2>, where the master branch seems to track upstream grub, the patch is present on at least the "fedora-28" and "rhel-7.5" branches. Commit hashes, respectively: c2b126f52143, 1b9767c136082. (3) In the commit message, Josef wrote, "When I fixed the txbuf handling I ripped out the retransmission code". I think he referred to his earlier commit 4fe8e6d4a127 ("efinet: handle get_status() on buggy firmware properly", 2015-08-09). That commit is upstream. In my opinion, commit 4fe8e6d4a127, the chronologically first, and upstream, tweak, was right (assuming the comment it added was true, about grub). On the other hand, the downstream-only (chronologically 2nd) commit was wrong. Not only did it break the spec, it broke even grub's own internal invariant, described in the comment that was added in upstream commit 4fe8e6d4a127. The comment states, "we only transmit one packet at a time". With the downstream-only tweak applied, that's no longer true. Namely, SNP.Transmit() is called while we know another transmission is pending on the egress queue. That's the definition of sending more than one packet at a time. I'm curious why the patch in question is not upstream. Was it submitted and rejected? Submitted and ignored? Not submitted at all? I'm not a fan of the hard-liner "spec above everything" approach. In this case though, after the downstream-only tweak, grub is inconsistent not only with the spec, but with itself too. IMO, downstream(s) should revert the downstream-only patch. Thanks, Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
