* [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change @ 2020-10-12 7:22 Gao, Zhichao 2020-10-13 2:54 ` Ni, Ray 2020-10-15 10:17 ` Laszlo Ersek 0 siblings, 2 replies; 16+ messages in thread From: Gao, Zhichao @ 2020-10-12 7:22 UTC (permalink / raw) To: devel; +Cc: Ray Ni, Hao A Wu REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843 Revert the patch to change the block size in child handler. It would block the CD (Eltorito) Hard disk media type's sub partition being observed. The blocksize patch used to fix the CD image's MBR table issue. The CD MBR table would always be ignored because it would be handled by the Eltorito partition handler first and never go into the MBR handler. So directly revert it. Cc: Ray Ni <ray.ni@intel.com> Cc: Hao A Wu <hao.a.wu@intel.com> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> --- MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c index f10ce7c65b..473e091320 100644 --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c @@ -1149,8 +1149,8 @@ PartitionInstallChildHandle ( Private->Signature = PARTITION_PRIVATE_DATA_SIGNATURE; - Private->Start = MultU64x32 (Start, BlockSize); - Private->End = MultU64x32 (End + 1, BlockSize); + Private->Start = MultU64x32 (Start, ParentBlockIo->Media->BlockSize); + Private->End = MultU64x32 (End + 1, ParentBlockIo->Media->BlockSize); Private->BlockSize = BlockSize; Private->ParentBlockIo = ParentBlockIo; @@ -1187,7 +1187,13 @@ PartitionInstallChildHandle ( Private->Media.IoAlign = 0; Private->Media.LogicalPartition = TRUE; - Private->Media.LastBlock = End - Start; + Private->Media.LastBlock = DivU64x32 ( + MultU64x32 ( + End - Start + 1, + ParentBlockIo->Media->BlockSize + ), + BlockSize + ) - 1; Private->Media.BlockSize = (UINT32) BlockSize; -- 2.21.0.windows.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change 2020-10-12 7:22 [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change Gao, Zhichao @ 2020-10-13 2:54 ` Ni, Ray 2020-10-14 5:58 ` [edk2-devel] " Zhiguang Liu 2020-10-15 10:17 ` Laszlo Ersek 1 sibling, 1 reply; 16+ messages in thread From: Ni, Ray @ 2020-10-13 2:54 UTC (permalink / raw) To: Gao, Zhichao, devel@edk2.groups.io; +Cc: Wu, Hao A Reviewed-by: Ray Ni <ray.ni@intel.com> > -----Original Message----- > From: Gao, Zhichao <zhichao.gao@intel.com> > Sent: Monday, October 12, 2020 3:23 PM > To: devel@edk2.groups.io > Cc: Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com> > Subject: [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843 > > Revert the patch to change the block size in child handler. It would > block the CD (Eltorito) Hard disk media type's sub partition being > observed. > The blocksize patch used to fix the CD image's MBR table issue. The > CD MBR table would always be ignored because it would be handled > by the Eltorito partition handler first and never go into the MBR > handler. So directly revert it. > > Cc: Ray Ni <ray.ni@intel.com> > Cc: Hao A Wu <hao.a.wu@intel.com> > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > --- > MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > index f10ce7c65b..473e091320 100644 > --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > @@ -1149,8 +1149,8 @@ PartitionInstallChildHandle ( > > Private->Signature = PARTITION_PRIVATE_DATA_SIGNATURE; > > - Private->Start = MultU64x32 (Start, BlockSize); > - Private->End = MultU64x32 (End + 1, BlockSize); > + Private->Start = MultU64x32 (Start, ParentBlockIo->Media->BlockSize); > + Private->End = MultU64x32 (End + 1, ParentBlockIo->Media->BlockSize); > > Private->BlockSize = BlockSize; > Private->ParentBlockIo = ParentBlockIo; > @@ -1187,7 +1187,13 @@ PartitionInstallChildHandle ( > > Private->Media.IoAlign = 0; > Private->Media.LogicalPartition = TRUE; > - Private->Media.LastBlock = End - Start; > + Private->Media.LastBlock = DivU64x32 ( > + MultU64x32 ( > + End - Start + 1, > + ParentBlockIo->Media->BlockSize > + ), > + BlockSize > + ) - 1; > > Private->Media.BlockSize = (UINT32) BlockSize; > > -- > 2.21.0.windows.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change 2020-10-13 2:54 ` Ni, Ray @ 2020-10-14 5:58 ` Zhiguang Liu 0 siblings, 0 replies; 16+ messages in thread From: Zhiguang Liu @ 2020-10-14 5:58 UTC (permalink / raw) To: devel@edk2.groups.io, Ni, Ray, Gao, Zhichao; +Cc: Wu, Hao A Hi Ray, Could you help merge this patch? Thanks Zhiguang > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray > Sent: Tuesday, October 13, 2020 10:54 AM > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io > Cc: Wu, Hao A <hao.a.wu@intel.com> > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the > child handler blocksize change > > Reviewed-by: Ray Ni <ray.ni@intel.com> > > > -----Original Message----- > > From: Gao, Zhichao <zhichao.gao@intel.com> > > Sent: Monday, October 12, 2020 3:23 PM > > To: devel@edk2.groups.io > > Cc: Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com> > > Subject: [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler > > blocksize change > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843 > > > > Revert the patch to change the block size in child handler. It would > > block the CD (Eltorito) Hard disk media type's sub partition being > > observed. > > The blocksize patch used to fix the CD image's MBR table issue. The CD > > MBR table would always be ignored because it would be handled by the > > Eltorito partition handler first and never go into the MBR handler. So > > directly revert it. > > > > Cc: Ray Ni <ray.ni@intel.com> > > Cc: Hao A Wu <hao.a.wu@intel.com> > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > > --- > > MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12 > > +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > index f10ce7c65b..473e091320 100644 > > --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > @@ -1149,8 +1149,8 @@ PartitionInstallChildHandle ( > > > > Private->Signature = PARTITION_PRIVATE_DATA_SIGNATURE; > > > > - Private->Start = MultU64x32 (Start, BlockSize); > > - Private->End = MultU64x32 (End + 1, BlockSize); > > + Private->Start = MultU64x32 (Start, ParentBlockIo->Media- > >BlockSize); > > + Private->End = MultU64x32 (End + 1, ParentBlockIo->Media- > >BlockSize); > > > > Private->BlockSize = BlockSize; > > Private->ParentBlockIo = ParentBlockIo; > > @@ -1187,7 +1187,13 @@ PartitionInstallChildHandle ( > > > > Private->Media.IoAlign = 0; > > Private->Media.LogicalPartition = TRUE; > > - Private->Media.LastBlock = End - Start; > > + Private->Media.LastBlock = DivU64x32 ( > > + MultU64x32 ( > > + End - Start + 1, > > + ParentBlockIo->Media->BlockSize > > + ), > > + BlockSize > > + ) - 1; > > > > Private->Media.BlockSize = (UINT32) BlockSize; > > > > -- > > 2.21.0.windows.1 > > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change 2020-10-12 7:22 [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change Gao, Zhichao 2020-10-13 2:54 ` Ni, Ray @ 2020-10-15 10:17 ` Laszlo Ersek 2020-10-16 6:42 ` Gary Lin 2020-10-19 2:41 ` Gao, Zhichao 1 sibling, 2 replies; 16+ messages in thread From: Laszlo Ersek @ 2020-10-15 10:17 UTC (permalink / raw) To: zhichao.gao; +Cc: devel, Ray Ni, Hao A Wu, Gary Lin On 10/12/20 09:22, Gao, Zhichao wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843 > > Revert the patch to change the block size in child handler. It would > block the CD (Eltorito) Hard disk media type's sub partition being > observed. > The blocksize patch used to fix the CD image's MBR table issue. The > CD MBR table would always be ignored because it would be handled > by the Eltorito partition handler first and never go into the MBR > handler. So directly revert it. > > Cc: Ray Ni <ray.ni@intel.com> > Cc: Hao A Wu <hao.a.wu@intel.com> > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > --- > MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > index f10ce7c65b..473e091320 100644 > --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > @@ -1149,8 +1149,8 @@ PartitionInstallChildHandle ( > > Private->Signature = PARTITION_PRIVATE_DATA_SIGNATURE; > > - Private->Start = MultU64x32 (Start, BlockSize); > - Private->End = MultU64x32 (End + 1, BlockSize); > + Private->Start = MultU64x32 (Start, ParentBlockIo->Media->BlockSize); > + Private->End = MultU64x32 (End + 1, ParentBlockIo->Media->BlockSize); > > Private->BlockSize = BlockSize; > Private->ParentBlockIo = ParentBlockIo; > @@ -1187,7 +1187,13 @@ PartitionInstallChildHandle ( > > Private->Media.IoAlign = 0; > Private->Media.LogicalPartition = TRUE; > - Private->Media.LastBlock = End - Start; > + Private->Media.LastBlock = DivU64x32 ( > + MultU64x32 ( > + End - Start + 1, > + ParentBlockIo->Media->BlockSize > + ), > + BlockSize > + ) - 1; > > Private->Media.BlockSize = (UINT32) BlockSize; > > (1) Adding Gary Lin to the CC list. (2) I can see that the TianoCore bugzilla ticket, namely <https://bugzilla.tianocore.org/show_bug.cgi?id=2843>, has been reopened. That's wrong. TianoCore#2843 was about calculating the starting and ending LBAs with incorrect block sizes. It was fixed by commit e0eacd7daa6f. Therefore TianoCore#2843 should stay in RESOLVED|FIXED status. Now that we have realized that commit e0eacd7daa6f caused a regression, a *new BZ* should be filed, stating the particular compatibility issue (regression). It is a different symptom from the symptom originally reported under TianoCore#2843, so it belongs in a different ticket. In particular, the statement in <https://bugzilla.tianocore.org/show_bug.cgi?id=2843#c8> that the original commit (which now should be reverted) "doesn't fix any specific issue", is *completely wrong*. If you look at commit e0eacd7daa6f, it contains the tag Tested-by: Gary Lin <glin@suse.com> Furthermore, if you look at the mailing list archive, you will find the following confirmation from Gary: After applying this patch series, the firmware recognizes openSUSE/SUSE iso images again. In the v1 thread at [edk2-devel] [PATCH 0/3] MdeModulePkg/PartitionDxe: Make the parition driver match the spec https://edk2.groups.io/g/devel/message/63972 http://mid.mail-archive.com/20200811075443.GG21538@GaryWorkstation And then, in the v2 thread, Gary wrote I've tested the following ISO images and all booted as expected. [...] again giving a Tested-by: [edk2-devel] [PATCH V2 0/3] MdeModulePkg/PartitionDxe: Make the parition driver match the spec https://edk2.groups.io/g/devel/message/64047 http://mid.mail-archive.com/20200812062652.GL21538@GaryWorkstation Now that you are proposing a revert, you have missed all of the above feedback from Gary. That's because you never bothered to link the v1 and v2 mailing list threads into the bugzilla ticket. So this patch risks reintroducing the issue that Gary reported originally. (Of course, the original bug report from Gary is *also* not linked into TianoCore#2843: https://edk2.groups.io/g/devel/message/62648 https://bugzilla.tianocore.org/show_bug.cgi?id=2823#c6 http://mid.mail-archive.com/20200716033255.GL6058@GaryWorkstation so it's no wonder we have no idea whose use case we could regress with a revert!) This patch should *NOT* be merged until Gary confirms it's OK. (And if it's not OK, then a solution is needed that fixes both Gary's use case, and the compatibility regression. It might even need a PCD, if there is media out there that needs one kind of logic, and other media that needs the other kind of logic.) (3) If this patch is a revert of commit e0eacd7daa6f, then the revert should be prepared with the "git revert" command. In particular, the commit message should very clearly state that this patch reverts commit e0eacd7daa6f. Thanks Laszlo ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change 2020-10-15 10:17 ` Laszlo Ersek @ 2020-10-16 6:42 ` Gary Lin 2020-10-19 2:45 ` Gao, Zhichao 2020-10-19 12:46 ` Laszlo Ersek 2020-10-19 2:41 ` Gao, Zhichao 1 sibling, 2 replies; 16+ messages in thread From: Gary Lin @ 2020-10-16 6:42 UTC (permalink / raw) To: Laszlo Ersek; +Cc: zhichao.gao, devel, Ray Ni, Hao A Wu On Thu, Oct 15, 2020 at 12:17:50PM +0200, Laszlo Ersek wrote: > On 10/12/20 09:22, Gao, Zhichao wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843 > > > > Revert the patch to change the block size in child handler. It would > > block the CD (Eltorito) Hard disk media type's sub partition being > > observed. > > The blocksize patch used to fix the CD image's MBR table issue. The > > CD MBR table would always be ignored because it would be handled > > by the Eltorito partition handler first and never go into the MBR > > handler. So directly revert it. > > > > Cc: Ray Ni <ray.ni@intel.com> > > Cc: Hao A Wu <hao.a.wu@intel.com> > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > > --- > > MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > index f10ce7c65b..473e091320 100644 > > --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > @@ -1149,8 +1149,8 @@ PartitionInstallChildHandle ( > > > > Private->Signature = PARTITION_PRIVATE_DATA_SIGNATURE; > > > > - Private->Start = MultU64x32 (Start, BlockSize); > > - Private->End = MultU64x32 (End + 1, BlockSize); > > + Private->Start = MultU64x32 (Start, ParentBlockIo->Media->BlockSize); > > + Private->End = MultU64x32 (End + 1, ParentBlockIo->Media->BlockSize); > > > > Private->BlockSize = BlockSize; > > Private->ParentBlockIo = ParentBlockIo; > > @@ -1187,7 +1187,13 @@ PartitionInstallChildHandle ( > > > > Private->Media.IoAlign = 0; > > Private->Media.LogicalPartition = TRUE; > > - Private->Media.LastBlock = End - Start; > > + Private->Media.LastBlock = DivU64x32 ( > > + MultU64x32 ( > > + End - Start + 1, > > + ParentBlockIo->Media->BlockSize > > + ), > > + BlockSize > > + ) - 1; > > > > Private->Media.BlockSize = (UINT32) BlockSize; > > > > > Hi Laszlo, > (1) Adding Gary Lin to the CC list. > Thanks for noticing me :) > > (2) I can see that the TianoCore bugzilla ticket, namely > <https://bugzilla.tianocore.org/show_bug.cgi?id=2843>, has been reopened. > > That's wrong. > > TianoCore#2843 was about calculating the starting and ending LBAs with > incorrect block sizes. It was fixed by commit e0eacd7daa6f. Therefore > TianoCore#2843 should stay in RESOLVED|FIXED status. > > Now that we have realized that commit e0eacd7daa6f caused a regression, > a *new BZ* should be filed, stating the particular compatibility issue > (regression). It is a different symptom from the symptom originally > reported under TianoCore#2843, so it belongs in a different ticket. > > In particular, the statement in > <https://bugzilla.tianocore.org/show_bug.cgi?id=2843#c8> that the > original commit (which now should be reverted) "doesn't fix any specific > issue", is *completely wrong*. If you look at commit e0eacd7daa6f, it > contains the tag > > Tested-by: Gary Lin <glin@suse.com> > > Furthermore, if you look at the mailing list archive, you will find the > following confirmation from Gary: > > After applying this patch series, the firmware recognizes > openSUSE/SUSE iso images again. > > In the v1 thread at > > [edk2-devel] [PATCH 0/3] > MdeModulePkg/PartitionDxe: Make the parition driver match the spec > > https://edk2.groups.io/g/devel/message/63972 > http://mid.mail-archive.com/20200811075443.GG21538@GaryWorkstation > > And then, in the v2 thread, Gary wrote > > I've tested the following ISO images and all booted as expected. > [...] > > again giving a Tested-by: > > [edk2-devel] [PATCH V2 0/3] > MdeModulePkg/PartitionDxe: Make the parition driver match the spec > > https://edk2.groups.io/g/devel/message/64047 > http://mid.mail-archive.com/20200812062652.GL21538@GaryWorkstation > > > Now that you are proposing a revert, you have missed all of the above > feedback from Gary. That's because you never bothered to link the v1 and > v2 mailing list threads into the bugzilla ticket. > > So this patch risks reintroducing the issue that Gary reported originally. > > (Of course, the original bug report from Gary is *also* not linked into > TianoCore#2843: > > https://edk2.groups.io/g/devel/message/62648 > https://bugzilla.tianocore.org/show_bug.cgi?id=2823#c6 > http://mid.mail-archive.com/20200716033255.GL6058@GaryWorkstation > > so it's no wonder we have no idea whose use case we could regress with a > revert!) > > This patch should *NOT* be merged until Gary confirms it's OK. > > (And if it's not OK, then a solution is needed that fixes both Gary's > use case, and the compatibility regression. It might even need a PCD, if > there is media out there that needs one kind of logic, and other media > that needs the other kind of logic.) > I just tested the patch with the ISO files with SLE15-SP2, openSUSE Leap 15.2, Fedora 32, and ubuntu 20.04, and the VM loads them without any problem, so there is no regression I had before. I'd give it my Tested-by. Tested-by: Gary Lin <glin@suse.com> Gary Lin > > (3) If this patch is a revert of commit e0eacd7daa6f, then the revert > should be prepared with the "git revert" command. In particular, the > commit message should very clearly state that this patch reverts commit > e0eacd7daa6f. > > > Thanks > Laszlo > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change 2020-10-16 6:42 ` Gary Lin @ 2020-10-19 2:45 ` Gao, Zhichao 2020-10-19 5:56 ` Ni, Ray 2020-10-19 12:46 ` Laszlo Ersek 1 sibling, 1 reply; 16+ messages in thread From: Gao, Zhichao @ 2020-10-19 2:45 UTC (permalink / raw) To: devel@edk2.groups.io, glin@suse.com, Laszlo Ersek; +Cc: Ni, Ray, Wu, Hao A Thanks Gary for your test. I have give my comments base on Laszlo's reply. I don't think the regression would affect Linux ISO image except there is one Linux image with boot catalog media type not NO_EMULATOR. Anyway, thanks for your test and your quickly response. Thanks, Zhichao > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gary Lin > Sent: Friday, October 16, 2020 2:42 PM > To: Laszlo Ersek <lersek@redhat.com> > Cc: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io; Ni, Ray > <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com> > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child > handler blocksize change > > On Thu, Oct 15, 2020 at 12:17:50PM +0200, Laszlo Ersek wrote: > > On 10/12/20 09:22, Gao, Zhichao wrote: > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843 > > > > > > Revert the patch to change the block size in child handler. It would > > > block the CD (Eltorito) Hard disk media type's sub partition being > > > observed. > > > The blocksize patch used to fix the CD image's MBR table issue. The > > > CD MBR table would always be ignored because it would be handled by > > > the Eltorito partition handler first and never go into the MBR > > > handler. So directly revert it. > > > > > > Cc: Ray Ni <ray.ni@intel.com> > > > Cc: Hao A Wu <hao.a.wu@intel.com> > > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > > > --- > > > MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12 > > > +++++++++--- > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > > b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > > index f10ce7c65b..473e091320 100644 > > > --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > > +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > > @@ -1149,8 +1149,8 @@ PartitionInstallChildHandle ( > > > > > > Private->Signature = PARTITION_PRIVATE_DATA_SIGNATURE; > > > > > > - Private->Start = MultU64x32 (Start, BlockSize); > > > - Private->End = MultU64x32 (End + 1, BlockSize); > > > + Private->Start = MultU64x32 (Start, ParentBlockIo->Media- > >BlockSize); > > > + Private->End = MultU64x32 (End + 1, ParentBlockIo->Media- > >BlockSize); > > > > > > Private->BlockSize = BlockSize; > > > Private->ParentBlockIo = ParentBlockIo; > > > @@ -1187,7 +1187,13 @@ PartitionInstallChildHandle ( > > > > > > Private->Media.IoAlign = 0; > > > Private->Media.LogicalPartition = TRUE; > > > - Private->Media.LastBlock = End - Start; > > > + Private->Media.LastBlock = DivU64x32 ( > > > + MultU64x32 ( > > > + End - Start + 1, > > > + ParentBlockIo->Media->BlockSize > > > + ), > > > + BlockSize > > > + ) - 1; > > > > > > Private->Media.BlockSize = (UINT32) BlockSize; > > > > > > > > > Hi Laszlo, > > > (1) Adding Gary Lin to the CC list. > > > Thanks for noticing me :) > > > > > (2) I can see that the TianoCore bugzilla ticket, namely > > <https://bugzilla.tianocore.org/show_bug.cgi?id=2843>, has been reopened. > > > > That's wrong. > > > > TianoCore#2843 was about calculating the starting and ending LBAs with > > incorrect block sizes. It was fixed by commit e0eacd7daa6f. Therefore > > TianoCore#2843 should stay in RESOLVED|FIXED status. > > > > Now that we have realized that commit e0eacd7daa6f caused a > > regression, a *new BZ* should be filed, stating the particular > > compatibility issue (regression). It is a different symptom from the > > symptom originally reported under TianoCore#2843, so it belongs in a > different ticket. > > > > In particular, the statement in > > <https://bugzilla.tianocore.org/show_bug.cgi?id=2843#c8> that the > > original commit (which now should be reverted) "doesn't fix any > > specific issue", is *completely wrong*. If you look at commit > > e0eacd7daa6f, it contains the tag > > > > Tested-by: Gary Lin <glin@suse.com> > > > > Furthermore, if you look at the mailing list archive, you will find > > the following confirmation from Gary: > > > > After applying this patch series, the firmware recognizes > > openSUSE/SUSE iso images again. > > > > In the v1 thread at > > > > [edk2-devel] [PATCH 0/3] > > MdeModulePkg/PartitionDxe: Make the parition driver match the spec > > > > https://edk2.groups.io/g/devel/message/63972 > > http://mid.mail-archive.com/20200811075443.GG21538@GaryWorkstation > > > > And then, in the v2 thread, Gary wrote > > > > I've tested the following ISO images and all booted as expected. > > [...] > > > > again giving a Tested-by: > > > > [edk2-devel] [PATCH V2 0/3] > > MdeModulePkg/PartitionDxe: Make the parition driver match the spec > > > > https://edk2.groups.io/g/devel/message/64047 > > http://mid.mail-archive.com/20200812062652.GL21538@GaryWorkstation > > > > > > Now that you are proposing a revert, you have missed all of the above > > feedback from Gary. That's because you never bothered to link the v1 > > and > > v2 mailing list threads into the bugzilla ticket. > > > > So this patch risks reintroducing the issue that Gary reported originally. > > > > (Of course, the original bug report from Gary is *also* not linked > > into > > TianoCore#2843: > > > > https://edk2.groups.io/g/devel/message/62648 > > https://bugzilla.tianocore.org/show_bug.cgi?id=2823#c6 > > http://mid.mail-archive.com/20200716033255.GL6058@GaryWorkstation > > > > so it's no wonder we have no idea whose use case we could regress with > > a > > revert!) > > > > This patch should *NOT* be merged until Gary confirms it's OK. > > > > (And if it's not OK, then a solution is needed that fixes both Gary's > > use case, and the compatibility regression. It might even need a PCD, > > if there is media out there that needs one kind of logic, and other > > media that needs the other kind of logic.) > > > I just tested the patch with the ISO files with SLE15-SP2, openSUSE Leap 15.2, > Fedora 32, and ubuntu 20.04, and the VM loads them without any problem, so > there is no regression I had before. > > I'd give it my Tested-by. > > Tested-by: Gary Lin <glin@suse.com> > > Gary Lin > > > > > (3) If this patch is a revert of commit e0eacd7daa6f, then the revert > > should be prepared with the "git revert" command. In particular, the > > commit message should very clearly state that this patch reverts > > commit e0eacd7daa6f. > > > > > > Thanks > > Laszlo > > > > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change 2020-10-19 2:45 ` Gao, Zhichao @ 2020-10-19 5:56 ` Ni, Ray 2020-10-19 9:35 ` Gao, Zhichao 2020-10-20 9:52 ` Laszlo Ersek 0 siblings, 2 replies; 16+ messages in thread From: Ni, Ray @ 2020-10-19 5:56 UTC (permalink / raw) To: Gao, Zhichao, devel@edk2.groups.io, glin@suse.com, Laszlo Ersek; +Cc: Wu, Hao A Zhichao, Can you please update the commit message to address Laszlo's comments? Thanks, Ray > -----Original Message----- > From: Gao, Zhichao <zhichao.gao@intel.com> > Sent: Monday, October 19, 2020 10:46 AM > To: devel@edk2.groups.io; glin@suse.com; Laszlo Ersek <lersek@redhat.com> > Cc: Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com> > Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the > child handler blocksize change > > Thanks Gary for your test. I have give my comments base on Laszlo's reply. I > don't think the regression would affect Linux ISO image except there is one > Linux image with boot catalog media type not NO_EMULATOR. Anyway, thanks > for your test and your quickly response. > > Thanks, > Zhichao > > > -----Original Message----- > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gary Lin > > Sent: Friday, October 16, 2020 2:42 PM > > To: Laszlo Ersek <lersek@redhat.com> > > Cc: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io; Ni, Ray > > <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com> > > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the > child > > handler blocksize change > > > > On Thu, Oct 15, 2020 at 12:17:50PM +0200, Laszlo Ersek wrote: > > > On 10/12/20 09:22, Gao, Zhichao wrote: > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843 > > > > > > > > Revert the patch to change the block size in child handler. It would > > > > block the CD (Eltorito) Hard disk media type's sub partition being > > > > observed. > > > > The blocksize patch used to fix the CD image's MBR table issue. The > > > > CD MBR table would always be ignored because it would be handled by > > > > the Eltorito partition handler first and never go into the MBR > > > > handler. So directly revert it. > > > > > > > > Cc: Ray Ni <ray.ni@intel.com> > > > > Cc: Hao A Wu <hao.a.wu@intel.com> > > > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > > > > --- > > > > MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12 > > > > +++++++++--- > > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > > > b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > > > index f10ce7c65b..473e091320 100644 > > > > --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > > > +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > > > @@ -1149,8 +1149,8 @@ PartitionInstallChildHandle ( > > > > > > > > Private->Signature = PARTITION_PRIVATE_DATA_SIGNATURE; > > > > > > > > - Private->Start = MultU64x32 (Start, BlockSize); > > > > - Private->End = MultU64x32 (End + 1, BlockSize); > > > > + Private->Start = MultU64x32 (Start, ParentBlockIo->Media- > > >BlockSize); > > > > + Private->End = MultU64x32 (End + 1, ParentBlockIo->Media- > > >BlockSize); > > > > > > > > Private->BlockSize = BlockSize; > > > > Private->ParentBlockIo = ParentBlockIo; > > > > @@ -1187,7 +1187,13 @@ PartitionInstallChildHandle ( > > > > > > > > Private->Media.IoAlign = 0; > > > > Private->Media.LogicalPartition = TRUE; > > > > - Private->Media.LastBlock = End - Start; > > > > + Private->Media.LastBlock = DivU64x32 ( > > > > + MultU64x32 ( > > > > + End - Start + 1, > > > > + ParentBlockIo->Media->BlockSize > > > > + ), > > > > + BlockSize > > > > + ) - 1; > > > > > > > > Private->Media.BlockSize = (UINT32) BlockSize; > > > > > > > > > > > > > Hi Laszlo, > > > > > (1) Adding Gary Lin to the CC list. > > > > > Thanks for noticing me :) > > > > > > > > (2) I can see that the TianoCore bugzilla ticket, namely > > > <https://bugzilla.tianocore.org/show_bug.cgi?id=2843>, has been reopened. > > > > > > That's wrong. > > > > > > TianoCore#2843 was about calculating the starting and ending LBAs with > > > incorrect block sizes. It was fixed by commit e0eacd7daa6f. Therefore > > > TianoCore#2843 should stay in RESOLVED|FIXED status. > > > > > > Now that we have realized that commit e0eacd7daa6f caused a > > > regression, a *new BZ* should be filed, stating the particular > > > compatibility issue (regression). It is a different symptom from the > > > symptom originally reported under TianoCore#2843, so it belongs in a > > different ticket. > > > > > > In particular, the statement in > > > <https://bugzilla.tianocore.org/show_bug.cgi?id=2843#c8> that the > > > original commit (which now should be reverted) "doesn't fix any > > > specific issue", is *completely wrong*. If you look at commit > > > e0eacd7daa6f, it contains the tag > > > > > > Tested-by: Gary Lin <glin@suse.com> > > > > > > Furthermore, if you look at the mailing list archive, you will find > > > the following confirmation from Gary: > > > > > > After applying this patch series, the firmware recognizes > > > openSUSE/SUSE iso images again. > > > > > > In the v1 thread at > > > > > > [edk2-devel] [PATCH 0/3] > > > MdeModulePkg/PartitionDxe: Make the parition driver match the spec > > > > > > https://edk2.groups.io/g/devel/message/63972 > > > http://mid.mail- > archive.com/20200811075443.GG21538@GaryWorkstation > > > > > > And then, in the v2 thread, Gary wrote > > > > > > I've tested the following ISO images and all booted as expected. > > > [...] > > > > > > again giving a Tested-by: > > > > > > [edk2-devel] [PATCH V2 0/3] > > > MdeModulePkg/PartitionDxe: Make the parition driver match the spec > > > > > > https://edk2.groups.io/g/devel/message/64047 > > > http://mid.mail- > archive.com/20200812062652.GL21538@GaryWorkstation > > > > > > > > > Now that you are proposing a revert, you have missed all of the above > > > feedback from Gary. That's because you never bothered to link the v1 > > > and > > > v2 mailing list threads into the bugzilla ticket. > > > > > > So this patch risks reintroducing the issue that Gary reported originally. > > > > > > (Of course, the original bug report from Gary is *also* not linked > > > into > > > TianoCore#2843: > > > > > > https://edk2.groups.io/g/devel/message/62648 > > > https://bugzilla.tianocore.org/show_bug.cgi?id=2823#c6 > > > http://mid.mail-archive.com/20200716033255.GL6058@GaryWorkstation > > > > > > so it's no wonder we have no idea whose use case we could regress with > > > a > > > revert!) > > > > > > This patch should *NOT* be merged until Gary confirms it's OK. > > > > > > (And if it's not OK, then a solution is needed that fixes both Gary's > > > use case, and the compatibility regression. It might even need a PCD, > > > if there is media out there that needs one kind of logic, and other > > > media that needs the other kind of logic.) > > > > > I just tested the patch with the ISO files with SLE15-SP2, openSUSE Leap 15.2, > > Fedora 32, and ubuntu 20.04, and the VM loads them without any problem, so > > there is no regression I had before. > > > > I'd give it my Tested-by. > > > > Tested-by: Gary Lin <glin@suse.com> > > > > Gary Lin > > > > > > > > (3) If this patch is a revert of commit e0eacd7daa6f, then the revert > > > should be prepared with the "git revert" command. In particular, the > > > commit message should very clearly state that this patch reverts > > > commit e0eacd7daa6f. > > > > > > > > > Thanks > > > Laszlo > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change 2020-10-19 5:56 ` Ni, Ray @ 2020-10-19 9:35 ` Gao, Zhichao 2020-10-19 13:12 ` Laszlo Ersek 2020-10-20 9:52 ` Laszlo Ersek 1 sibling, 1 reply; 16+ messages in thread From: Gao, Zhichao @ 2020-10-19 9:35 UTC (permalink / raw) To: Ni, Ray, devel@edk2.groups.io, glin@suse.com, Laszlo Ersek; +Cc: Wu, Hao A Yes. But I need to confirm the comment #3. The directly revert would make the title of the commit message larger which may be larger than 72 chars. That is another reason I didn't use revert directly. Thanks, Zhichao > -----Original Message----- > From: Ni, Ray <ray.ni@intel.com> > Sent: Monday, October 19, 2020 1:56 PM > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io; > glin@suse.com; Laszlo Ersek <lersek@redhat.com> > Cc: Wu, Hao A <hao.a.wu@intel.com> > Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child > handler blocksize change > > Zhichao, > Can you please update the commit message to address Laszlo's comments? > > Thanks, > Ray > > > -----Original Message----- > > From: Gao, Zhichao <zhichao.gao@intel.com> > > Sent: Monday, October 19, 2020 10:46 AM > > To: devel@edk2.groups.io; glin@suse.com; Laszlo Ersek > > <lersek@redhat.com> > > Cc: Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com> > > Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert > > the child handler blocksize change > > > > Thanks Gary for your test. I have give my comments base on Laszlo's > > reply. I don't think the regression would affect Linux ISO image > > except there is one Linux image with boot catalog media type not > > NO_EMULATOR. Anyway, thanks for your test and your quickly response. > > > > Thanks, > > Zhichao > > > > > -----Original Message----- > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gary > > > Lin > > > Sent: Friday, October 16, 2020 2:42 PM > > > To: Laszlo Ersek <lersek@redhat.com> > > > Cc: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io; Ni, > > > Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com> > > > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert > > > the > > child > > > handler blocksize change > > > > > > On Thu, Oct 15, 2020 at 12:17:50PM +0200, Laszlo Ersek wrote: > > > > On 10/12/20 09:22, Gao, Zhichao wrote: > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843 > > > > > > > > > > Revert the patch to change the block size in child handler. It > > > > > would block the CD (Eltorito) Hard disk media type's sub > > > > > partition being observed. > > > > > The blocksize patch used to fix the CD image's MBR table issue. > > > > > The CD MBR table would always be ignored because it would be > > > > > handled by the Eltorito partition handler first and never go > > > > > into the MBR handler. So directly revert it. > > > > > > > > > > Cc: Ray Ni <ray.ni@intel.com> > > > > > Cc: Hao A Wu <hao.a.wu@intel.com> > > > > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > > > > > --- > > > > > MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12 > > > > > +++++++++--- > > > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git > > > > > a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > > > > b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > > > > index f10ce7c65b..473e091320 100644 > > > > > --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > > > > +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > > > > @@ -1149,8 +1149,8 @@ PartitionInstallChildHandle ( > > > > > > > > > > Private->Signature = PARTITION_PRIVATE_DATA_SIGNATURE; > > > > > > > > > > - Private->Start = MultU64x32 (Start, BlockSize); > > > > > - Private->End = MultU64x32 (End + 1, BlockSize); > > > > > + Private->Start = MultU64x32 (Start, ParentBlockIo->Media- > > > >BlockSize); > > > > > + Private->End = MultU64x32 (End + 1, ParentBlockIo->Media- > > > >BlockSize); > > > > > > > > > > Private->BlockSize = BlockSize; > > > > > Private->ParentBlockIo = ParentBlockIo; > > > > > @@ -1187,7 +1187,13 @@ PartitionInstallChildHandle ( > > > > > > > > > > Private->Media.IoAlign = 0; > > > > > Private->Media.LogicalPartition = TRUE; > > > > > - Private->Media.LastBlock = End - Start; > > > > > + Private->Media.LastBlock = DivU64x32 ( > > > > > + MultU64x32 ( > > > > > + End - Start + 1, > > > > > + ParentBlockIo->Media->BlockSize > > > > > + ), > > > > > + BlockSize > > > > > + ) - 1; > > > > > > > > > > Private->Media.BlockSize = (UINT32) BlockSize; > > > > > > > > > > > > > > > > > Hi Laszlo, > > > > > > > (1) Adding Gary Lin to the CC list. > > > > > > > Thanks for noticing me :) > > > > > > > > > > > (2) I can see that the TianoCore bugzilla ticket, namely > > > > <https://bugzilla.tianocore.org/show_bug.cgi?id=2843>, has been > reopened. > > > > > > > > That's wrong. > > > > > > > > TianoCore#2843 was about calculating the starting and ending LBAs > > > > with incorrect block sizes. It was fixed by commit e0eacd7daa6f. > > > > Therefore > > > > TianoCore#2843 should stay in RESOLVED|FIXED status. > > > > > > > > Now that we have realized that commit e0eacd7daa6f caused a > > > > regression, a *new BZ* should be filed, stating the particular > > > > compatibility issue (regression). It is a different symptom from > > > > the symptom originally reported under TianoCore#2843, so it > > > > belongs in a > > > different ticket. > > > > > > > > In particular, the statement in > > > > <https://bugzilla.tianocore.org/show_bug.cgi?id=2843#c8> that the > > > > original commit (which now should be reverted) "doesn't fix any > > > > specific issue", is *completely wrong*. If you look at commit > > > > e0eacd7daa6f, it contains the tag > > > > > > > > Tested-by: Gary Lin <glin@suse.com> > > > > > > > > Furthermore, if you look at the mailing list archive, you will > > > > find the following confirmation from Gary: > > > > > > > > After applying this patch series, the firmware recognizes > > > > openSUSE/SUSE iso images again. > > > > > > > > In the v1 thread at > > > > > > > > [edk2-devel] [PATCH 0/3] > > > > MdeModulePkg/PartitionDxe: Make the parition driver match the > > > > spec > > > > > > > > https://edk2.groups.io/g/devel/message/63972 > > > > http://mid.mail- > > archive.com/20200811075443.GG21538@GaryWorkstation > > > > > > > > And then, in the v2 thread, Gary wrote > > > > > > > > I've tested the following ISO images and all booted as expected. > > > > [...] > > > > > > > > again giving a Tested-by: > > > > > > > > [edk2-devel] [PATCH V2 0/3] > > > > MdeModulePkg/PartitionDxe: Make the parition driver match the > > > > spec > > > > > > > > https://edk2.groups.io/g/devel/message/64047 > > > > http://mid.mail- > > archive.com/20200812062652.GL21538@GaryWorkstation > > > > > > > > > > > > Now that you are proposing a revert, you have missed all of the > > > > above feedback from Gary. That's because you never bothered to > > > > link the v1 and > > > > v2 mailing list threads into the bugzilla ticket. > > > > > > > > So this patch risks reintroducing the issue that Gary reported originally. > > > > > > > > (Of course, the original bug report from Gary is *also* not linked > > > > into > > > > TianoCore#2843: > > > > > > > > https://edk2.groups.io/g/devel/message/62648 > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=2823#c6 > > > > > > > > http://mid.mail-archive.com/20200716033255.GL6058@GaryWorkstation > > > > > > > > so it's no wonder we have no idea whose use case we could regress > > > > with a > > > > revert!) > > > > > > > > This patch should *NOT* be merged until Gary confirms it's OK. > > > > > > > > (And if it's not OK, then a solution is needed that fixes both > > > > Gary's use case, and the compatibility regression. It might even > > > > need a PCD, if there is media out there that needs one kind of > > > > logic, and other media that needs the other kind of logic.) > > > > > > > I just tested the patch with the ISO files with SLE15-SP2, openSUSE > > > Leap 15.2, Fedora 32, and ubuntu 20.04, and the VM loads them > > > without any problem, so there is no regression I had before. > > > > > > I'd give it my Tested-by. > > > > > > Tested-by: Gary Lin <glin@suse.com> > > > > > > Gary Lin > > > > > > > > > > > (3) If this patch is a revert of commit e0eacd7daa6f, then the > > > > revert should be prepared with the "git revert" command. In > > > > particular, the commit message should very clearly state that this > > > > patch reverts commit e0eacd7daa6f. > > > > > > > > > > > > Thanks > > > > Laszlo > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change 2020-10-19 9:35 ` Gao, Zhichao @ 2020-10-19 13:12 ` Laszlo Ersek 2020-10-20 1:06 ` Gao, Zhichao 0 siblings, 1 reply; 16+ messages in thread From: Laszlo Ersek @ 2020-10-19 13:12 UTC (permalink / raw) To: Gao, Zhichao, Ni, Ray, devel@edk2.groups.io, glin@suse.com; +Cc: Wu, Hao A On 10/19/20 11:35, Gao, Zhichao wrote: > Yes. But I need to confirm the comment #3. The directly revert would > make the title of the commit message larger which may be larger than > 72 chars. That is another reason I didn't use revert directly. If the desired action is to undo (roll back / revert) a specific earlier commit, then git-revert is always the right tool to *start with*. Then git-revert either works, or it doesn't: - if git-revert completes at once, code-wise, then the commit message is supposed to be edited manually, in every case. The commit message prepared by git-revert is just a template. What's important is that the subject of the revert patch clearly reflect (a) the subject of the original patch, and (b) the fact that we're undoing an earlier commit. Additionally, the commit message body should clearly identify the commit hash of the patch being rolled back. Beyond that, the author of the revert patch is welcome to extend the commit message with as many details as necessary. - git-revert might find that, due to changes committed in the meantime, it cannot just undo the original commit (i.e., the inverse diff doesn't apply cleanly, and cannot be fixed up automatically). In that case, manual code editing is necessary. This can mean a successful manual conflict resolution; after wich git-revert counts as "successful", so see the previous paragraph. - Failure of git-revert can also necessitate entirely new, manual coding, for rolling back the original patch. Semantically, the situation stays the same, thus the same information as in the previous bullets should be included in the commit message. The formal requirements are a bit looser, because git-revert never completed, and so it didn't give us a commit message template to extend. It's still preferred to say something like "Undo '<original subject>'" in the subject line, and to include the original commit hash in the commit message body. If git-revert completes, but proposes an overlong subject (according to PatchCheck.py), then personally I prefer truncating the original, quoted subject, with an ellipsis (...). In this particular case, "git revert e0eacd7daa6f" seems to work without manual code editing. The subject line it creates is 3 characters too long. Editing that to the following variant: Revert "MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child ..." works OK. Summary: (1) always start with git-revert please, (2) try to stick with the subject pattern that git-revert creates; truncate as needed, (3) *always* name the commit being reverted by commit hash, in the new commit message body, (4) append as much information as you see fit, to the commit message body. Thanks Laszlo > > Thanks, > Zhichao > >> -----Original Message----- >> From: Ni, Ray <ray.ni@intel.com> >> Sent: Monday, October 19, 2020 1:56 PM >> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io; >> glin@suse.com; Laszlo Ersek <lersek@redhat.com> >> Cc: Wu, Hao A <hao.a.wu@intel.com> >> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child >> handler blocksize change >> >> Zhichao, >> Can you please update the commit message to address Laszlo's comments? >> >> Thanks, >> Ray >> >>> -----Original Message----- >>> From: Gao, Zhichao <zhichao.gao@intel.com> >>> Sent: Monday, October 19, 2020 10:46 AM >>> To: devel@edk2.groups.io; glin@suse.com; Laszlo Ersek >>> <lersek@redhat.com> >>> Cc: Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com> >>> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert >>> the child handler blocksize change >>> >>> Thanks Gary for your test. I have give my comments base on Laszlo's >>> reply. I don't think the regression would affect Linux ISO image >>> except there is one Linux image with boot catalog media type not >>> NO_EMULATOR. Anyway, thanks for your test and your quickly response. >>> >>> Thanks, >>> Zhichao >>> >>>> -----Original Message----- >>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gary >>>> Lin >>>> Sent: Friday, October 16, 2020 2:42 PM >>>> To: Laszlo Ersek <lersek@redhat.com> >>>> Cc: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io; Ni, >>>> Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com> >>>> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert >>>> the >>> child >>>> handler blocksize change >>>> >>>> On Thu, Oct 15, 2020 at 12:17:50PM +0200, Laszlo Ersek wrote: >>>>> On 10/12/20 09:22, Gao, Zhichao wrote: >>>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843 >>>>>> >>>>>> Revert the patch to change the block size in child handler. It >>>>>> would block the CD (Eltorito) Hard disk media type's sub >>>>>> partition being observed. >>>>>> The blocksize patch used to fix the CD image's MBR table issue. >>>>>> The CD MBR table would always be ignored because it would be >>>>>> handled by the Eltorito partition handler first and never go >>>>>> into the MBR handler. So directly revert it. >>>>>> >>>>>> Cc: Ray Ni <ray.ni@intel.com> >>>>>> Cc: Hao A Wu <hao.a.wu@intel.com> >>>>>> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> >>>>>> --- >>>>>> MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12 >>>>>> +++++++++--- >>>>>> 1 file changed, 9 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git >>>>>> a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c >>>>>> b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c >>>>>> index f10ce7c65b..473e091320 100644 >>>>>> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c >>>>>> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c >>>>>> @@ -1149,8 +1149,8 @@ PartitionInstallChildHandle ( >>>>>> >>>>>> Private->Signature = PARTITION_PRIVATE_DATA_SIGNATURE; >>>>>> >>>>>> - Private->Start = MultU64x32 (Start, BlockSize); >>>>>> - Private->End = MultU64x32 (End + 1, BlockSize); >>>>>> + Private->Start = MultU64x32 (Start, ParentBlockIo->Media- >>>>> BlockSize); >>>>>> + Private->End = MultU64x32 (End + 1, ParentBlockIo->Media- >>>>> BlockSize); >>>>>> >>>>>> Private->BlockSize = BlockSize; >>>>>> Private->ParentBlockIo = ParentBlockIo; >>>>>> @@ -1187,7 +1187,13 @@ PartitionInstallChildHandle ( >>>>>> >>>>>> Private->Media.IoAlign = 0; >>>>>> Private->Media.LogicalPartition = TRUE; >>>>>> - Private->Media.LastBlock = End - Start; >>>>>> + Private->Media.LastBlock = DivU64x32 ( >>>>>> + MultU64x32 ( >>>>>> + End - Start + 1, >>>>>> + ParentBlockIo->Media->BlockSize >>>>>> + ), >>>>>> + BlockSize >>>>>> + ) - 1; >>>>>> >>>>>> Private->Media.BlockSize = (UINT32) BlockSize; >>>>>> >>>>>> >>>>> >>>> Hi Laszlo, >>>> >>>>> (1) Adding Gary Lin to the CC list. >>>>> >>>> Thanks for noticing me :) >>>> >>>>> >>>>> (2) I can see that the TianoCore bugzilla ticket, namely >>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=2843>, has been >> reopened. >>>>> >>>>> That's wrong. >>>>> >>>>> TianoCore#2843 was about calculating the starting and ending LBAs >>>>> with incorrect block sizes. It was fixed by commit e0eacd7daa6f. >>>>> Therefore >>>>> TianoCore#2843 should stay in RESOLVED|FIXED status. >>>>> >>>>> Now that we have realized that commit e0eacd7daa6f caused a >>>>> regression, a *new BZ* should be filed, stating the particular >>>>> compatibility issue (regression). It is a different symptom from >>>>> the symptom originally reported under TianoCore#2843, so it >>>>> belongs in a >>>> different ticket. >>>>> >>>>> In particular, the statement in >>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=2843#c8> that the >>>>> original commit (which now should be reverted) "doesn't fix any >>>>> specific issue", is *completely wrong*. If you look at commit >>>>> e0eacd7daa6f, it contains the tag >>>>> >>>>> Tested-by: Gary Lin <glin@suse.com> >>>>> >>>>> Furthermore, if you look at the mailing list archive, you will >>>>> find the following confirmation from Gary: >>>>> >>>>> After applying this patch series, the firmware recognizes >>>>> openSUSE/SUSE iso images again. >>>>> >>>>> In the v1 thread at >>>>> >>>>> [edk2-devel] [PATCH 0/3] >>>>> MdeModulePkg/PartitionDxe: Make the parition driver match the >>>>> spec >>>>> >>>>> https://edk2.groups.io/g/devel/message/63972 >>>>> http://mid.mail- >>> archive.com/20200811075443.GG21538@GaryWorkstation >>>>> >>>>> And then, in the v2 thread, Gary wrote >>>>> >>>>> I've tested the following ISO images and all booted as expected. >>>>> [...] >>>>> >>>>> again giving a Tested-by: >>>>> >>>>> [edk2-devel] [PATCH V2 0/3] >>>>> MdeModulePkg/PartitionDxe: Make the parition driver match the >>>>> spec >>>>> >>>>> https://edk2.groups.io/g/devel/message/64047 >>>>> http://mid.mail- >>> archive.com/20200812062652.GL21538@GaryWorkstation >>>>> >>>>> >>>>> Now that you are proposing a revert, you have missed all of the >>>>> above feedback from Gary. That's because you never bothered to >>>>> link the v1 and >>>>> v2 mailing list threads into the bugzilla ticket. >>>>> >>>>> So this patch risks reintroducing the issue that Gary reported originally. >>>>> >>>>> (Of course, the original bug report from Gary is *also* not linked >>>>> into >>>>> TianoCore#2843: >>>>> >>>>> https://edk2.groups.io/g/devel/message/62648 >>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=2823#c6 >>>>> >>>>> http://mid.mail-archive.com/20200716033255.GL6058@GaryWorkstation >>>>> >>>>> so it's no wonder we have no idea whose use case we could regress >>>>> with a >>>>> revert!) >>>>> >>>>> This patch should *NOT* be merged until Gary confirms it's OK. >>>>> >>>>> (And if it's not OK, then a solution is needed that fixes both >>>>> Gary's use case, and the compatibility regression. It might even >>>>> need a PCD, if there is media out there that needs one kind of >>>>> logic, and other media that needs the other kind of logic.) >>>>> >>>> I just tested the patch with the ISO files with SLE15-SP2, openSUSE >>>> Leap 15.2, Fedora 32, and ubuntu 20.04, and the VM loads them >>>> without any problem, so there is no regression I had before. >>>> >>>> I'd give it my Tested-by. >>>> >>>> Tested-by: Gary Lin <glin@suse.com> >>>> >>>> Gary Lin >>>> >>>>> >>>>> (3) If this patch is a revert of commit e0eacd7daa6f, then the >>>>> revert should be prepared with the "git revert" command. In >>>>> particular, the commit message should very clearly state that this >>>>> patch reverts commit e0eacd7daa6f. >>>>> >>>>> >>>>> Thanks >>>>> Laszlo >>>>> >>>> >>>> >>>> >>>> >>>> > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change 2020-10-19 13:12 ` Laszlo Ersek @ 2020-10-20 1:06 ` Gao, Zhichao 0 siblings, 0 replies; 16+ messages in thread From: Gao, Zhichao @ 2020-10-20 1:06 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com, Ni, Ray, glin@suse.com; +Cc: Wu, Hao A Thanks, Laszlo. It helps a lot and gives a nice guide for revert. Thanks, Zhichao > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek > Sent: Monday, October 19, 2020 9:13 PM > To: Gao, Zhichao <zhichao.gao@intel.com>; Ni, Ray <ray.ni@intel.com>; > devel@edk2.groups.io; glin@suse.com > Cc: Wu, Hao A <hao.a.wu@intel.com> > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child > handler blocksize change > > On 10/19/20 11:35, Gao, Zhichao wrote: > > Yes. But I need to confirm the comment #3. The directly revert would > > make the title of the commit message larger which may be larger than > > 72 chars. That is another reason I didn't use revert directly. > > If the desired action is to undo (roll back / revert) a specific earlier commit, then > git-revert is always the right tool to *start with*. Then git-revert either works, or > it doesn't: > > - if git-revert completes at once, code-wise, then the commit message is > supposed to be edited manually, in every case. The commit message > prepared by git-revert is just a template. What's important is that > the subject of the revert patch clearly reflect (a) the subject of the > original patch, and (b) the fact that we're undoing an earlier commit. > Additionally, the commit message body should clearly identify the > commit hash of the patch being rolled back. Beyond that, the author of > the revert patch is welcome to extend the commit message with as many > details as necessary. > > - git-revert might find that, due to changes committed in the meantime, > it cannot just undo the original commit (i.e., the inverse diff > doesn't apply cleanly, and cannot be fixed up automatically). In that > case, manual code editing is necessary. This can mean a successful > manual conflict resolution; after wich git-revert counts as > "successful", so see the previous paragraph. > > - Failure of git-revert can also necessitate entirely new, manual > coding, for rolling back the original patch. Semantically, the > situation stays the same, thus the same information as in the previous > bullets should be included in the commit message. The formal > requirements are a bit looser, because git-revert never completed, and > so it didn't give us a commit message template to extend. It's still > preferred to say something like "Undo '<original subject>'" in the > subject line, and to include the original commit hash in the commit > message body. > > If git-revert completes, but proposes an overlong subject (according to > PatchCheck.py), then personally I prefer truncating the original, quoted subject, > with an ellipsis (...). > > In this particular case, "git revert e0eacd7daa6f" seems to work without manual > code editing. The subject line it creates is 3 characters too long. Editing that to > the following variant: > > Revert "MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child ..." > > works OK. > > Summary: > > (1) always start with git-revert please, > > (2) try to stick with the subject pattern that git-revert creates; > truncate as needed, > > (3) *always* name the commit being reverted by commit hash, in the new > commit message body, > > (4) append as much information as you see fit, to the commit message > body. > > Thanks > Laszlo > > > > > Thanks, > > Zhichao > > > >> -----Original Message----- > >> From: Ni, Ray <ray.ni@intel.com> > >> Sent: Monday, October 19, 2020 1:56 PM > >> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io; > >> glin@suse.com; Laszlo Ersek <lersek@redhat.com> > >> Cc: Wu, Hao A <hao.a.wu@intel.com> > >> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert > >> the child handler blocksize change > >> > >> Zhichao, > >> Can you please update the commit message to address Laszlo's comments? > >> > >> Thanks, > >> Ray > >> > >>> -----Original Message----- > >>> From: Gao, Zhichao <zhichao.gao@intel.com> > >>> Sent: Monday, October 19, 2020 10:46 AM > >>> To: devel@edk2.groups.io; glin@suse.com; Laszlo Ersek > >>> <lersek@redhat.com> > >>> Cc: Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com> > >>> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert > >>> the child handler blocksize change > >>> > >>> Thanks Gary for your test. I have give my comments base on Laszlo's > >>> reply. I don't think the regression would affect Linux ISO image > >>> except there is one Linux image with boot catalog media type not > >>> NO_EMULATOR. Anyway, thanks for your test and your quickly response. > >>> > >>> Thanks, > >>> Zhichao > >>> > >>>> -----Original Message----- > >>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gary > >>>> Lin > >>>> Sent: Friday, October 16, 2020 2:42 PM > >>>> To: Laszlo Ersek <lersek@redhat.com> > >>>> Cc: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io; Ni, > >>>> Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com> > >>>> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert > >>>> the > >>> child > >>>> handler blocksize change > >>>> > >>>> On Thu, Oct 15, 2020 at 12:17:50PM +0200, Laszlo Ersek wrote: > >>>>> On 10/12/20 09:22, Gao, Zhichao wrote: > >>>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843 > >>>>>> > >>>>>> Revert the patch to change the block size in child handler. It > >>>>>> would block the CD (Eltorito) Hard disk media type's sub > >>>>>> partition being observed. > >>>>>> The blocksize patch used to fix the CD image's MBR table issue. > >>>>>> The CD MBR table would always be ignored because it would be > >>>>>> handled by the Eltorito partition handler first and never go into > >>>>>> the MBR handler. So directly revert it. > >>>>>> > >>>>>> Cc: Ray Ni <ray.ni@intel.com> > >>>>>> Cc: Hao A Wu <hao.a.wu@intel.com> > >>>>>> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > >>>>>> --- > >>>>>> MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12 > >>>>>> +++++++++--- > >>>>>> 1 file changed, 9 insertions(+), 3 deletions(-) > >>>>>> > >>>>>> diff --git > >>>>>> a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > >>>>>> b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > >>>>>> index f10ce7c65b..473e091320 100644 > >>>>>> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > >>>>>> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > >>>>>> @@ -1149,8 +1149,8 @@ PartitionInstallChildHandle ( > >>>>>> > >>>>>> Private->Signature = PARTITION_PRIVATE_DATA_SIGNATURE; > >>>>>> > >>>>>> - Private->Start = MultU64x32 (Start, BlockSize); > >>>>>> - Private->End = MultU64x32 (End + 1, BlockSize); > >>>>>> + Private->Start = MultU64x32 (Start, ParentBlockIo->Media- > >>>>> BlockSize); > >>>>>> + Private->End = MultU64x32 (End + 1, ParentBlockIo->Media- > >>>>> BlockSize); > >>>>>> > >>>>>> Private->BlockSize = BlockSize; > >>>>>> Private->ParentBlockIo = ParentBlockIo; > >>>>>> @@ -1187,7 +1187,13 @@ PartitionInstallChildHandle ( > >>>>>> > >>>>>> Private->Media.IoAlign = 0; > >>>>>> Private->Media.LogicalPartition = TRUE; > >>>>>> - Private->Media.LastBlock = End - Start; > >>>>>> + Private->Media.LastBlock = DivU64x32 ( > >>>>>> + MultU64x32 ( > >>>>>> + End - Start + 1, > >>>>>> + ParentBlockIo->Media->BlockSize > >>>>>> + ), > >>>>>> + BlockSize > >>>>>> + ) - 1; > >>>>>> > >>>>>> Private->Media.BlockSize = (UINT32) BlockSize; > >>>>>> > >>>>>> > >>>>> > >>>> Hi Laszlo, > >>>> > >>>>> (1) Adding Gary Lin to the CC list. > >>>>> > >>>> Thanks for noticing me :) > >>>> > >>>>> > >>>>> (2) I can see that the TianoCore bugzilla ticket, namely > >>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=2843>, has been > >> reopened. > >>>>> > >>>>> That's wrong. > >>>>> > >>>>> TianoCore#2843 was about calculating the starting and ending LBAs > >>>>> with incorrect block sizes. It was fixed by commit e0eacd7daa6f. > >>>>> Therefore > >>>>> TianoCore#2843 should stay in RESOLVED|FIXED status. > >>>>> > >>>>> Now that we have realized that commit e0eacd7daa6f caused a > >>>>> regression, a *new BZ* should be filed, stating the particular > >>>>> compatibility issue (regression). It is a different symptom from > >>>>> the symptom originally reported under TianoCore#2843, so it > >>>>> belongs in a > >>>> different ticket. > >>>>> > >>>>> In particular, the statement in > >>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=2843#c8> that the > >>>>> original commit (which now should be reverted) "doesn't fix any > >>>>> specific issue", is *completely wrong*. If you look at commit > >>>>> e0eacd7daa6f, it contains the tag > >>>>> > >>>>> Tested-by: Gary Lin <glin@suse.com> > >>>>> > >>>>> Furthermore, if you look at the mailing list archive, you will > >>>>> find the following confirmation from Gary: > >>>>> > >>>>> After applying this patch series, the firmware recognizes > >>>>> openSUSE/SUSE iso images again. > >>>>> > >>>>> In the v1 thread at > >>>>> > >>>>> [edk2-devel] [PATCH 0/3] > >>>>> MdeModulePkg/PartitionDxe: Make the parition driver match the > >>>>> spec > >>>>> > >>>>> https://edk2.groups.io/g/devel/message/63972 > >>>>> http://mid.mail- > >>> archive.com/20200811075443.GG21538@GaryWorkstation > >>>>> > >>>>> And then, in the v2 thread, Gary wrote > >>>>> > >>>>> I've tested the following ISO images and all booted as expected. > >>>>> [...] > >>>>> > >>>>> again giving a Tested-by: > >>>>> > >>>>> [edk2-devel] [PATCH V2 0/3] > >>>>> MdeModulePkg/PartitionDxe: Make the parition driver match the > >>>>> spec > >>>>> > >>>>> https://edk2.groups.io/g/devel/message/64047 > >>>>> http://mid.mail- > >>> archive.com/20200812062652.GL21538@GaryWorkstation > >>>>> > >>>>> > >>>>> Now that you are proposing a revert, you have missed all of the > >>>>> above feedback from Gary. That's because you never bothered to > >>>>> link the v1 and > >>>>> v2 mailing list threads into the bugzilla ticket. > >>>>> > >>>>> So this patch risks reintroducing the issue that Gary reported originally. > >>>>> > >>>>> (Of course, the original bug report from Gary is *also* not linked > >>>>> into > >>>>> TianoCore#2843: > >>>>> > >>>>> https://edk2.groups.io/g/devel/message/62648 > >>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=2823#c6 > >>>>> > >>>>> http://mid.mail-archive.com/20200716033255.GL6058@GaryWorkstation > >>>>> > >>>>> so it's no wonder we have no idea whose use case we could regress > >>>>> with a > >>>>> revert!) > >>>>> > >>>>> This patch should *NOT* be merged until Gary confirms it's OK. > >>>>> > >>>>> (And if it's not OK, then a solution is needed that fixes both > >>>>> Gary's use case, and the compatibility regression. It might even > >>>>> need a PCD, if there is media out there that needs one kind of > >>>>> logic, and other media that needs the other kind of logic.) > >>>>> > >>>> I just tested the patch with the ISO files with SLE15-SP2, openSUSE > >>>> Leap 15.2, Fedora 32, and ubuntu 20.04, and the VM loads them > >>>> without any problem, so there is no regression I had before. > >>>> > >>>> I'd give it my Tested-by. > >>>> > >>>> Tested-by: Gary Lin <glin@suse.com> > >>>> > >>>> Gary Lin > >>>> > >>>>> > >>>>> (3) If this patch is a revert of commit e0eacd7daa6f, then the > >>>>> revert should be prepared with the "git revert" command. In > >>>>> particular, the commit message should very clearly state that this > >>>>> patch reverts commit e0eacd7daa6f. > >>>>> > >>>>> > >>>>> Thanks > >>>>> Laszlo > >>>>> > >>>> > >>>> > >>>> > >>>> > >>>> > > > > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change 2020-10-19 5:56 ` Ni, Ray 2020-10-19 9:35 ` Gao, Zhichao @ 2020-10-20 9:52 ` Laszlo Ersek 2020-10-21 1:33 ` Gao, Zhichao 1 sibling, 1 reply; 16+ messages in thread From: Laszlo Ersek @ 2020-10-20 9:52 UTC (permalink / raw) To: devel, ray.ni, Gao, Zhichao, glin@suse.com Cc: Wu, Hao A, Michael Kinney, Tom Lendacky Ray, On 10/19/20 07:56, Ni, Ray wrote: > Zhichao, > Can you please update the commit message to address Laszlo's comments? why did you merge <https://github.com/tianocore/edk2/pull/1033>? PR#1033 was originally submitted as a personal build for Zhichao. When it passed CI (and was auto-closed), Zhichao should have posted the updated patch (with the cleaned up commit message) as v2 to the mailing list, for the next round of review. Instead, you reopened the (auto-closed) PR, added the push label, and the mergify bot merged the patch. You merged a patch that was not reviewed on the list; specifically you didn't give me any chance to re-check the commit message, after I pointed out problems with it under the v1 thread. Of course, sometimes we (participants in a patch review thread) agree that the maintainer will perform some final (small) updates just before merging the patch or series, but in this case, that has not happened -- no specific wording was proposed or accepted in the thread, as far as I can see. Regardin the timeline: as of this writing, Zhichao opened PR#1033 eight hours ago, and you made the mergify bot merge the (unreviewed) patch 4 hours ago. That is, you just wanted to get rid of it as quickly as possible, without any regard to the community (again -- I provided *specific* feedback under v1). This urgency is especially appalling when contrasted with your and Eric's total lack of feedback for Tom Lendacky's patch [edk2-devel] [PATCH v2 1/1] UefiCpuPkg/MpInitLib: Reduce reset vector memory pressure https://edk2.groups.io/g/devel/message/65540 for almost a *month*. I had to merge that patch yesterday out of desperation and embarrassment for your behaviors, with only my R-b added. You forced me to break the development process in order not to alienate a prolific contributor. All one of you had to do was post an Acked-by. You can't make a contributor wait for this long, and you also can't sneak in patches without public review (or at least public agreement about the final touch-ups). If you can't do maintenance responsibly, for any reason, then *quit*; let someone else become maintainer. This behavior is a hallmark of the edk2 project not having a healthy open source community. Sad. Laszlo ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change 2020-10-20 9:52 ` Laszlo Ersek @ 2020-10-21 1:33 ` Gao, Zhichao 2020-10-21 12:26 ` Laszlo Ersek 0 siblings, 1 reply; 16+ messages in thread From: Gao, Zhichao @ 2020-10-21 1:33 UTC (permalink / raw) To: Laszlo Ersek, devel@edk2.groups.io, Ni, Ray, glin@suse.com Cc: Wu, Hao A, Kinney, Michael D, Tom Lendacky Hi Laszlo, Apologize for the patch merge thing. That is my fault. Base on your comments, I update the commit message with revert info and part of my V1 commit message. And I am thinking there is no other doubt with the reverting. Sometimes the maintainers and reviewers my give the change comment and let the submitter not to send the patch again because the change is tiny. I treat this patch as that condition incorrectly. That's why I contact Ray to review the new commit message and help to push directly if he is OK with it. Sorry for missing you in the process. Thanks, Zhichao > -----Original Message----- > From: Laszlo Ersek <lersek@redhat.com> > Sent: Tuesday, October 20, 2020 5:53 PM > To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao > <zhichao.gao@intel.com>; glin@suse.com > Cc: Wu, Hao A <hao.a.wu@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com>; Tom Lendacky <thomas.lendacky@amd.com> > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child > handler blocksize change > > Ray, > > On 10/19/20 07:56, Ni, Ray wrote: > > Zhichao, > > Can you please update the commit message to address Laszlo's comments? > > why did you merge <https://github.com/tianocore/edk2/pull/1033>? > > PR#1033 was originally submitted as a personal build for Zhichao. When it passed > CI (and was auto-closed), Zhichao should have posted the updated patch (with > the cleaned up commit message) as v2 to the mailing list, for the next round of > review. > > Instead, you reopened the (auto-closed) PR, added the push label, and the > mergify bot merged the patch. You merged a patch that was not reviewed on the > list; specifically you didn't give me any chance to re-check the commit message, > after I pointed out problems with it under the v1 thread. Of course, sometimes > we (participants in a patch review > thread) agree that the maintainer will perform some final (small) updates just > before merging the patch or series, but in this case, that has not happened -- no > specific wording was proposed or accepted in the thread, as far as I can see. > > Regardin the timeline: as of this writing, Zhichao opened PR#1033 eight hours > ago, and you made the mergify bot merge the (unreviewed) patch 4 hours ago. > That is, you just wanted to get rid of it as quickly as possible, without any regard > to the community (again -- I provided > *specific* feedback under v1). > > This urgency is especially appalling when contrasted with your and Eric's total > lack of feedback for Tom Lendacky's patch > > [edk2-devel] [PATCH v2 1/1] > UefiCpuPkg/MpInitLib: Reduce reset vector memory pressure > > https://edk2.groups.io/g/devel/message/65540 > > for almost a *month*. I had to merge that patch yesterday out of desperation > and embarrassment for your behaviors, with only my R-b added. You forced me > to break the development process in order not to alienate a prolific contributor. > All one of you had to do was post an Acked-by. > > You can't make a contributor wait for this long, and you also can't sneak in > patches without public review (or at least public agreement about the final touch- > ups). If you can't do maintenance responsibly, for any reason, then *quit*; let > someone else become maintainer. > > This behavior is a hallmark of the edk2 project not having a healthy open source > community. Sad. > > Laszlo ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change 2020-10-21 1:33 ` Gao, Zhichao @ 2020-10-21 12:26 ` Laszlo Ersek 2020-10-22 5:36 ` Gao, Zhichao 0 siblings, 1 reply; 16+ messages in thread From: Laszlo Ersek @ 2020-10-21 12:26 UTC (permalink / raw) To: Gao, Zhichao, devel@edk2.groups.io, Ni, Ray, glin@suse.com Cc: Wu, Hao A, Kinney, Michael D, Tom Lendacky On 10/21/20 03:33, Gao, Zhichao wrote: > Hi Laszlo, > > Apologize for the patch merge thing. That is my fault. Base on your comments, I update the commit message with revert info and part of my V1 commit message. And I am thinking there is no other doubt with the reverting. Sometimes the maintainers and reviewers my give the change comment and let the submitter not to send the patch again because the change is tiny. Indeed this is very common, but it is always agreed upon beforehand. > I treat this patch as that condition incorrectly. That's why I contact Ray to review the new commit message and help to push directly if he is OK with it. Sorry for missing you in the process. Another problem is that the PR itself has to be created by the maintainer. https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process#the-maintainer-process-for-the-edk-ii-project As I understand it, the current process does not permit a contributor to create a PR, and a maintainer to just set the "push" label on that PR. Thanks Laszlo > > Thanks, > Zhichao > >> -----Original Message----- >> From: Laszlo Ersek <lersek@redhat.com> >> Sent: Tuesday, October 20, 2020 5:53 PM >> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao >> <zhichao.gao@intel.com>; glin@suse.com >> Cc: Wu, Hao A <hao.a.wu@intel.com>; Kinney, Michael D >> <michael.d.kinney@intel.com>; Tom Lendacky <thomas.lendacky@amd.com> >> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child >> handler blocksize change >> >> Ray, >> >> On 10/19/20 07:56, Ni, Ray wrote: >>> Zhichao, >>> Can you please update the commit message to address Laszlo's comments? >> >> why did you merge <https://github.com/tianocore/edk2/pull/1033>? >> >> PR#1033 was originally submitted as a personal build for Zhichao. When it passed >> CI (and was auto-closed), Zhichao should have posted the updated patch (with >> the cleaned up commit message) as v2 to the mailing list, for the next round of >> review. >> >> Instead, you reopened the (auto-closed) PR, added the push label, and the >> mergify bot merged the patch. You merged a patch that was not reviewed on the >> list; specifically you didn't give me any chance to re-check the commit message, >> after I pointed out problems with it under the v1 thread. Of course, sometimes >> we (participants in a patch review >> thread) agree that the maintainer will perform some final (small) updates just >> before merging the patch or series, but in this case, that has not happened -- no >> specific wording was proposed or accepted in the thread, as far as I can see. >> >> Regardin the timeline: as of this writing, Zhichao opened PR#1033 eight hours >> ago, and you made the mergify bot merge the (unreviewed) patch 4 hours ago. >> That is, you just wanted to get rid of it as quickly as possible, without any regard >> to the community (again -- I provided >> *specific* feedback under v1). >> >> This urgency is especially appalling when contrasted with your and Eric's total >> lack of feedback for Tom Lendacky's patch >> >> [edk2-devel] [PATCH v2 1/1] >> UefiCpuPkg/MpInitLib: Reduce reset vector memory pressure >> >> https://edk2.groups.io/g/devel/message/65540 >> >> for almost a *month*. I had to merge that patch yesterday out of desperation >> and embarrassment for your behaviors, with only my R-b added. You forced me >> to break the development process in order not to alienate a prolific contributor. >> All one of you had to do was post an Acked-by. >> >> You can't make a contributor wait for this long, and you also can't sneak in >> patches without public review (or at least public agreement about the final touch- >> ups). If you can't do maintenance responsibly, for any reason, then *quit*; let >> someone else become maintainer. >> >> This behavior is a hallmark of the edk2 project not having a healthy open source >> community. Sad. >> >> Laszlo > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change 2020-10-21 12:26 ` Laszlo Ersek @ 2020-10-22 5:36 ` Gao, Zhichao 0 siblings, 0 replies; 16+ messages in thread From: Gao, Zhichao @ 2020-10-22 5:36 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com, Ni, Ray, glin@suse.com Cc: Wu, Hao A, Kinney, Michael D, Tom Lendacky I used to collect the RB and send the finial patch to the maintainers to merge before. I keep that behavior since the PR process. Thanks for notice me the incorrect process. I would let the maintainers do this in the future. About the comment not to send another patch to community and take the RB after change base on the comment, I would give the changed personal fork branch for final review and keep all the CCers noticed. Thanks, Zhichao > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek > Sent: Wednesday, October 21, 2020 8:27 PM > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io; Ni, Ray > <ray.ni@intel.com>; glin@suse.com > Cc: Wu, Hao A <hao.a.wu@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com>; Tom Lendacky <thomas.lendacky@amd.com> > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child > handler blocksize change > > On 10/21/20 03:33, Gao, Zhichao wrote: > > Hi Laszlo, > > > > Apologize for the patch merge thing. That is my fault. Base on your comments, > I update the commit message with revert info and part of my V1 commit > message. And I am thinking there is no other doubt with the reverting. > Sometimes the maintainers and reviewers my give the change comment and let > the submitter not to send the patch again because the change is tiny. > > Indeed this is very common, but it is always agreed upon beforehand. > > > I treat this patch as that condition incorrectly. That's why I contact Ray to > review the new commit message and help to push directly if he is OK with it. > Sorry for missing you in the process. > > Another problem is that the PR itself has to be created by the maintainer. > > https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development- > Process#the-maintainer-process-for-the-edk-ii-project > > As I understand it, the current process does not permit a contributor to create a > PR, and a maintainer to just set the "push" label on that PR. > > Thanks > Laszlo > > > > > > Thanks, > > Zhichao > > > >> -----Original Message----- > >> From: Laszlo Ersek <lersek@redhat.com> > >> Sent: Tuesday, October 20, 2020 5:53 PM > >> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao > >> <zhichao.gao@intel.com>; glin@suse.com > >> Cc: Wu, Hao A <hao.a.wu@intel.com>; Kinney, Michael D > >> <michael.d.kinney@intel.com>; Tom Lendacky <thomas.lendacky@amd.com> > >> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert > >> the child handler blocksize change > >> > >> Ray, > >> > >> On 10/19/20 07:56, Ni, Ray wrote: > >>> Zhichao, > >>> Can you please update the commit message to address Laszlo's comments? > >> > >> why did you merge <https://github.com/tianocore/edk2/pull/1033>? > >> > >> PR#1033 was originally submitted as a personal build for Zhichao. > >> When it passed CI (and was auto-closed), Zhichao should have posted > >> the updated patch (with the cleaned up commit message) as v2 to the > >> mailing list, for the next round of review. > >> > >> Instead, you reopened the (auto-closed) PR, added the push label, and > >> the mergify bot merged the patch. You merged a patch that was not > >> reviewed on the list; specifically you didn't give me any chance to > >> re-check the commit message, after I pointed out problems with it > >> under the v1 thread. Of course, sometimes we (participants in a patch > >> review > >> thread) agree that the maintainer will perform some final (small) > >> updates just before merging the patch or series, but in this case, > >> that has not happened -- no specific wording was proposed or accepted in the > thread, as far as I can see. > >> > >> Regardin the timeline: as of this writing, Zhichao opened PR#1033 > >> eight hours ago, and you made the mergify bot merge the (unreviewed) patch > 4 hours ago. > >> That is, you just wanted to get rid of it as quickly as possible, > >> without any regard to the community (again -- I provided > >> *specific* feedback under v1). > >> > >> This urgency is especially appalling when contrasted with your and > >> Eric's total lack of feedback for Tom Lendacky's patch > >> > >> [edk2-devel] [PATCH v2 1/1] > >> UefiCpuPkg/MpInitLib: Reduce reset vector memory pressure > >> > >> https://edk2.groups.io/g/devel/message/65540 > >> > >> for almost a *month*. I had to merge that patch yesterday out of > >> desperation and embarrassment for your behaviors, with only my R-b > >> added. You forced me to break the development process in order not to > alienate a prolific contributor. > >> All one of you had to do was post an Acked-by. > >> > >> You can't make a contributor wait for this long, and you also can't > >> sneak in patches without public review (or at least public agreement > >> about the final touch- ups). If you can't do maintenance responsibly, > >> for any reason, then *quit*; let someone else become maintainer. > >> > >> This behavior is a hallmark of the edk2 project not having a healthy > >> open source community. Sad. > >> > >> Laszlo > > > > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change 2020-10-16 6:42 ` Gary Lin 2020-10-19 2:45 ` Gao, Zhichao @ 2020-10-19 12:46 ` Laszlo Ersek 1 sibling, 0 replies; 16+ messages in thread From: Laszlo Ersek @ 2020-10-19 12:46 UTC (permalink / raw) To: Gary Lin; +Cc: zhichao.gao, devel, Ray Ni, Hao A Wu On 10/16/20 08:42, Gary Lin wrote: > On Thu, Oct 15, 2020 at 12:17:50PM +0200, Laszlo Ersek wrote: >> On 10/12/20 09:22, Gao, Zhichao wrote: >>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843 >>> >>> Revert the patch to change the block size in child handler. It would >>> block the CD (Eltorito) Hard disk media type's sub partition being >>> observed. >>> The blocksize patch used to fix the CD image's MBR table issue. The >>> CD MBR table would always be ignored because it would be handled >>> by the Eltorito partition handler first and never go into the MBR >>> handler. So directly revert it. >>> >>> Cc: Ray Ni <ray.ni@intel.com> >>> Cc: Hao A Wu <hao.a.wu@intel.com> >>> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> >>> --- >>> MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12 +++++++++--- >>> 1 file changed, 9 insertions(+), 3 deletions(-) >>> >>> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c >>> index f10ce7c65b..473e091320 100644 >>> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c >>> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c >>> @@ -1149,8 +1149,8 @@ PartitionInstallChildHandle ( >>> >>> Private->Signature = PARTITION_PRIVATE_DATA_SIGNATURE; >>> >>> - Private->Start = MultU64x32 (Start, BlockSize); >>> - Private->End = MultU64x32 (End + 1, BlockSize); >>> + Private->Start = MultU64x32 (Start, ParentBlockIo->Media->BlockSize); >>> + Private->End = MultU64x32 (End + 1, ParentBlockIo->Media->BlockSize); >>> >>> Private->BlockSize = BlockSize; >>> Private->ParentBlockIo = ParentBlockIo; >>> @@ -1187,7 +1187,13 @@ PartitionInstallChildHandle ( >>> >>> Private->Media.IoAlign = 0; >>> Private->Media.LogicalPartition = TRUE; >>> - Private->Media.LastBlock = End - Start; >>> + Private->Media.LastBlock = DivU64x32 ( >>> + MultU64x32 ( >>> + End - Start + 1, >>> + ParentBlockIo->Media->BlockSize >>> + ), >>> + BlockSize >>> + ) - 1; >>> >>> Private->Media.BlockSize = (UINT32) BlockSize; >>> >>> >> > Hi Laszlo, > >> (1) Adding Gary Lin to the CC list. >> > Thanks for noticing me :) > >> >> (2) I can see that the TianoCore bugzilla ticket, namely >> <https://bugzilla.tianocore.org/show_bug.cgi?id=2843>, has been reopened. >> >> That's wrong. >> >> TianoCore#2843 was about calculating the starting and ending LBAs with >> incorrect block sizes. It was fixed by commit e0eacd7daa6f. Therefore >> TianoCore#2843 should stay in RESOLVED|FIXED status. >> >> Now that we have realized that commit e0eacd7daa6f caused a regression, >> a *new BZ* should be filed, stating the particular compatibility issue >> (regression). It is a different symptom from the symptom originally >> reported under TianoCore#2843, so it belongs in a different ticket. >> >> In particular, the statement in >> <https://bugzilla.tianocore.org/show_bug.cgi?id=2843#c8> that the >> original commit (which now should be reverted) "doesn't fix any specific >> issue", is *completely wrong*. If you look at commit e0eacd7daa6f, it >> contains the tag >> >> Tested-by: Gary Lin <glin@suse.com> >> >> Furthermore, if you look at the mailing list archive, you will find the >> following confirmation from Gary: >> >> After applying this patch series, the firmware recognizes >> openSUSE/SUSE iso images again. >> >> In the v1 thread at >> >> [edk2-devel] [PATCH 0/3] >> MdeModulePkg/PartitionDxe: Make the parition driver match the spec >> >> https://edk2.groups.io/g/devel/message/63972 >> http://mid.mail-archive.com/20200811075443.GG21538@GaryWorkstation >> >> And then, in the v2 thread, Gary wrote >> >> I've tested the following ISO images and all booted as expected. >> [...] >> >> again giving a Tested-by: >> >> [edk2-devel] [PATCH V2 0/3] >> MdeModulePkg/PartitionDxe: Make the parition driver match the spec >> >> https://edk2.groups.io/g/devel/message/64047 >> http://mid.mail-archive.com/20200812062652.GL21538@GaryWorkstation >> >> >> Now that you are proposing a revert, you have missed all of the above >> feedback from Gary. That's because you never bothered to link the v1 and >> v2 mailing list threads into the bugzilla ticket. >> >> So this patch risks reintroducing the issue that Gary reported originally. >> >> (Of course, the original bug report from Gary is *also* not linked into >> TianoCore#2843: >> >> https://edk2.groups.io/g/devel/message/62648 >> https://bugzilla.tianocore.org/show_bug.cgi?id=2823#c6 >> http://mid.mail-archive.com/20200716033255.GL6058@GaryWorkstation >> >> so it's no wonder we have no idea whose use case we could regress with a >> revert!) >> >> This patch should *NOT* be merged until Gary confirms it's OK. >> >> (And if it's not OK, then a solution is needed that fixes both Gary's >> use case, and the compatibility regression. It might even need a PCD, if >> there is media out there that needs one kind of logic, and other media >> that needs the other kind of logic.) >> > I just tested the patch with the ISO files with SLE15-SP2, openSUSE Leap > 15.2, Fedora 32, and ubuntu 20.04, and the VM loads them without any > problem, so there is no regression I had before. > > I'd give it my Tested-by. > > Tested-by: Gary Lin <glin@suse.com> Thank you, Gary! Laszlo > > Gary Lin > >> >> (3) If this patch is a revert of commit e0eacd7daa6f, then the revert >> should be prepared with the "git revert" command. In particular, the >> commit message should very clearly state that this patch reverts commit >> e0eacd7daa6f. >> >> >> Thanks >> Laszlo >> > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change 2020-10-15 10:17 ` Laszlo Ersek 2020-10-16 6:42 ` Gary Lin @ 2020-10-19 2:41 ` Gao, Zhichao 1 sibling, 0 replies; 16+ messages in thread From: Gao, Zhichao @ 2020-10-19 2:41 UTC (permalink / raw) To: Laszlo Ersek; +Cc: devel@edk2.groups.io, Ni, Ray, Wu, Hao A, Gary Lin Hi Laszlo, > -----Original Message----- > From: Laszlo Ersek <lersek@redhat.com> > Sent: Thursday, October 15, 2020 6:18 PM > To: Gao, Zhichao <zhichao.gao@intel.com> > Cc: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Wu, Hao A > <hao.a.wu@intel.com>; Gary Lin <glin@suse.com> > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child > handler blocksize change > > On 10/12/20 09:22, Gao, Zhichao wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843 > > > > Revert the patch to change the block size in child handler. It would > > block the CD (Eltorito) Hard disk media type's sub partition being > > observed. > > The blocksize patch used to fix the CD image's MBR table issue. The CD > > MBR table would always be ignored because it would be handled by the > > Eltorito partition handler first and never go into the MBR handler. So > > directly revert it. > > > > Cc: Ray Ni <ray.ni@intel.com> > > Cc: Hao A Wu <hao.a.wu@intel.com> > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > > --- > > MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12 > > +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > index f10ce7c65b..473e091320 100644 > > --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c > > @@ -1149,8 +1149,8 @@ PartitionInstallChildHandle ( > > > > Private->Signature = PARTITION_PRIVATE_DATA_SIGNATURE; > > > > - Private->Start = MultU64x32 (Start, BlockSize); > > - Private->End = MultU64x32 (End + 1, BlockSize); > > + Private->Start = MultU64x32 (Start, ParentBlockIo->Media->BlockSize); > > + Private->End = MultU64x32 (End + 1, ParentBlockIo->Media- > >BlockSize); > > > > Private->BlockSize = BlockSize; > > Private->ParentBlockIo = ParentBlockIo; > > @@ -1187,7 +1187,13 @@ PartitionInstallChildHandle ( > > > > Private->Media.IoAlign = 0; > > Private->Media.LogicalPartition = TRUE; > > - Private->Media.LastBlock = End - Start; > > + Private->Media.LastBlock = DivU64x32 ( > > + MultU64x32 ( > > + End - Start + 1, > > + ParentBlockIo->Media->BlockSize > > + ), > > + BlockSize > > + ) - 1; > > > > Private->Media.BlockSize = (UINT32) BlockSize; > > > > > > (1) Adding Gary Lin to the CC list. > > > (2) I can see that the TianoCore bugzilla ticket, namely > <https://bugzilla.tianocore.org/show_bug.cgi?id=2843>, has been reopened. > > That's wrong. > > TianoCore#2843 was about calculating the starting and ending LBAs with > incorrect block sizes. It was fixed by commit e0eacd7daa6f. Therefore > TianoCore#2843 should stay in RESOLVED|FIXED status. My bad. I want the keep all the description in one page. Seems it is not a good way to do this. I would open another BZ and change 2843 as it previous status. > > Now that we have realized that commit e0eacd7daa6f caused a regression, a > *new BZ* should be filed, stating the particular compatibility issue (regression). > It is a different symptom from the symptom originally reported under > TianoCore#2843, so it belongs in a different ticket. > > In particular, the statement in > <https://bugzilla.tianocore.org/show_bug.cgi?id=2843#c8> that the original > commit (which now should be reverted) "doesn't fix any specific issue", is > *completely wrong*. If you look at commit e0eacd7daa6f, it contains the tag > > Tested-by: Gary Lin <glin@suse.com> > > Furthermore, if you look at the mailing list archive, you will find the following > confirmation from Gary: > > After applying this patch series, the firmware recognizes > openSUSE/SUSE iso images again. > > In the v1 thread at > > [edk2-devel] [PATCH 0/3] > MdeModulePkg/PartitionDxe: Make the parition driver match the spec > > https://edk2.groups.io/g/devel/message/63972 > http://mid.mail-archive.com/20200811075443.GG21538@GaryWorkstation > > And then, in the v2 thread, Gary wrote > > I've tested the following ISO images and all booted as expected. > [...] > > again giving a Tested-by: > > [edk2-devel] [PATCH V2 0/3] > MdeModulePkg/PartitionDxe: Make the parition driver match the spec > > https://edk2.groups.io/g/devel/message/64047 > http://mid.mail-archive.com/20200812062652.GL21538@GaryWorkstation > > > Now that you are proposing a revert, you have missed all of the above feedback > from Gary. That's because you never bothered to link the v1 and > v2 mailing list threads into the bugzilla ticket. > > So this patch risks reintroducing the issue that Gary reported originally. > > (Of course, the original bug report from Gary is *also* not linked into > TianoCore#2843: > > https://edk2.groups.io/g/devel/message/62648 > https://bugzilla.tianocore.org/show_bug.cgi?id=2823#c6 > http://mid.mail-archive.com/20200716033255.GL6058@GaryWorkstation > > so it's no wonder we have no idea whose use case we could regress with a > revert!) > > This patch should *NOT* be merged until Gary confirms it's OK. > > (And if it's not OK, then a solution is needed that fixes both Gary's use case, and > the compatibility regression. It might even need a PCD, if there is media out > there that needs one kind of logic, and other media that needs the other kind of > logic.) The patch V2 #3 is an optimization. There is no issue report related to it. I find it when I analysis the Linux CD iso image. Before the patch set, the MBR handler is ahead of the UDF (Eltorito compatible). And the ISO FAT cannot be found with the ISO's MBR table. But this issue is already gone with patch V2 #1 because the MBR of ISO image would be ignored and the ISO image would always be handled as UDF/Eltorito. The regression is for the CD image with Media type ELTORITO_12_DISKETTE/ELTORITO_14_DISKETTE/ELTORITO_28_DISKETTE/ELTORITO_HARD_DISK which is not the normal Linux iso image. I cannot find all these types of ISO image. But there is one type ELTORITO_12_DISKETTE can be treat as an example, it is DOS6.22_bootdisk.iso. Now that the legacy boot is dropped from edk2 but the FAT disk should be found in shell. I suggest to revert the patch because of above regression and I already tested with the DOS image. The DOS image's FAT can be found after reverting. Another reason to revert it is the code logic has been used for more than ten years and seems no issue is reported related to the handler. That is why I think the original logic would be more compatible. > > > (3) If this patch is a revert of commit e0eacd7daa6f, then the revert should be > prepared with the "git revert" command. In particular, the commit message > should very clearly state that this patch reverts commit e0eacd7daa6f. Can we? Directly revert cannot give the detail and the reason. Thanks, Zhichao > > > Thanks > Laszlo ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-10-22 5:36 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-10-12 7:22 [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change Gao, Zhichao 2020-10-13 2:54 ` Ni, Ray 2020-10-14 5:58 ` [edk2-devel] " Zhiguang Liu 2020-10-15 10:17 ` Laszlo Ersek 2020-10-16 6:42 ` Gary Lin 2020-10-19 2:45 ` Gao, Zhichao 2020-10-19 5:56 ` Ni, Ray 2020-10-19 9:35 ` Gao, Zhichao 2020-10-19 13:12 ` Laszlo Ersek 2020-10-20 1:06 ` Gao, Zhichao 2020-10-20 9:52 ` Laszlo Ersek 2020-10-21 1:33 ` Gao, Zhichao 2020-10-21 12:26 ` Laszlo Ersek 2020-10-22 5:36 ` Gao, Zhichao 2020-10-19 12:46 ` Laszlo Ersek 2020-10-19 2:41 ` Gao, Zhichao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox