[edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change

Gao, Zhichao posted 1 patch 1 week ago
Failed in applying to current master (apply log)
MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)

[edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change

Posted by Gao, Zhichao 1 week ago
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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66124): https://edk2.groups.io/g/devel/message/66124
Mute This Topic: https://groups.io/mt/77455892/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change

Posted by Laszlo Ersek 6 days ago
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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66251): https://edk2.groups.io/g/devel/message/66251
Mute This Topic: https://groups.io/mt/77455892/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change

Posted by Gao, Zhichao 3 days ago
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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66368): https://edk2.groups.io/g/devel/message/66368
Mute This Topic: https://groups.io/mt/77455892/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change

Posted by Gary Lin 6 days ago
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
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66312): https://edk2.groups.io/g/devel/message/66312
Mute This Topic: https://groups.io/mt/77455892/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change

Posted by Laszlo Ersek 2 days ago
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
>>
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66386): https://edk2.groups.io/g/devel/message/66386
Mute This Topic: https://groups.io/mt/77455892/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change

Posted by Gao, Zhichao 3 days ago
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
> >
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66369): https://edk2.groups.io/g/devel/message/66369
Mute This Topic: https://groups.io/mt/77455892/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change

Posted by Ni, Ray 3 days ago
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
> > >
> >
> >
> >
> > 
> >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66376): https://edk2.groups.io/g/devel/message/66376
Mute This Topic: https://groups.io/mt/77455892/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change

Posted by Laszlo Ersek 1 day ago
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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66453): https://edk2.groups.io/g/devel/message/66453
Mute This Topic: https://groups.io/mt/77455892/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change

Posted by Gao, Zhichao 1 day ago
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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66462): https://edk2.groups.io/g/devel/message/66462
Mute This Topic: https://groups.io/mt/77455892/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change

Posted by Laszlo Ersek 20 hours ago
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
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66486): https://edk2.groups.io/g/devel/message/66486
Mute This Topic: https://groups.io/mt/77455892/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change

Posted by Gao, Zhichao 3 hours ago
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
> >
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66528): https://edk2.groups.io/g/devel/message/66528
Mute This Topic: https://groups.io/mt/77455892/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change

Posted by Gao, Zhichao 2 days ago
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
> > > >
> > >
> > >
> > >
> > > 
> > >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66385): https://edk2.groups.io/g/devel/message/66385
Mute This Topic: https://groups.io/mt/77455892/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change

Posted by Laszlo Ersek 2 days ago
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
>>>>>
>>>>
>>>>
>>>>
>>>> 
>>>>
>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66388): https://edk2.groups.io/g/devel/message/66388
Mute This Topic: https://groups.io/mt/77455892/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change

Posted by Gao, Zhichao 2 days ago
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
> >>>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66428): https://edk2.groups.io/g/devel/message/66428
Mute This Topic: https://groups.io/mt/77455892/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change

Posted by Ni, Ray 1 week ago
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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66150): https://edk2.groups.io/g/devel/message/66150
Mute This Topic: https://groups.io/mt/77455892/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change

Posted by Zhiguang Liu 1 week ago
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
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66208): https://edk2.groups.io/g/devel/message/66208
Mute This Topic: https://groups.io/mt/77455892/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-