Thanks for pointing this, yes, this change actually replaces structure field from FeaturesCompat to FeaturesRoCompat. The commit message was already corrected in referenced repository fork.

With the commit message changed,

Reviewed-by: Marvin Häuser <mhaeuser@...>

On Fri, Jan 27, 2023 at 10:02 AM Marvin Häuser <mhaeuser@...> wrote:

On 27. Jan 2023, at 10:29, Savva Mitrofanov <savvamtr@...> wrote:

Missing comparison != 0 leads to broken logic condition.

The actual issue appears to be FeaturesCompat vs FeaturesRoCompat (latter is hidden by your usage of the macro). Checking for != 0 is redundant (albeit required by the code style guideline for non-functional reasons). Please confirm and update the commit message if you agree.

+1, the actual issue comes from using the wrong feature mask and not the != 0.
If we get a v4, please update, else I can rewrite it myself.

Reviewed-by: Pedro Falcato <pedro.falcato@...>

--
Pedro

On 27. Jan 2023, at 10:29, Savva Mitrofanov <savvamtr@...> wrote:

Missing comparison != 0 leads to broken logic condition.

The actual issue appears to be FeaturesCompat vs FeaturesRoCompat (latter is hidden by your usage of the macro). Checking for != 0 is redundant (albeit required by the code style guideline for non-functional reasons). Please confirm and update the commit message if you agree.

Also replaced
CSUM_SEED feature_incompat check with predefined macro EXT4_HAS_INCOMPAT

Cc: Marvin Häuser <mhaeuser@...>
Cc: Pedro Falcato <pedro.falcato@...>
Cc: Vitaly Cheptsov <vit9696@...>
Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
Signed-off-by: Savva Mitrofanov <savvamtr@...>
---
Features/Ext4Pkg/Ext4Dxe/Superblock.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
index 5a3c7f478187..35dcf3c007c8 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -220,13 +220,11 @@ Ext4OpenSuperblock (
}

// At the time of writing, it's the only supported checksum.
- if (Partition->FeaturesCompat & EXT4_FEATURE_RO_COMPAT_METADATA_CSUM &&
- (Sb->s_checksum_type != EXT4_CHECKSUM_CRC32C))
- {
+ if (EXT4_HAS_METADATA_CSUM (Partition) && (Sb->s_checksum_type != EXT4_CHECKSUM_CRC32C)) {
return EFI_UNSUPPORTED;
}

- if ((Partition->FeaturesIncompat & EXT4_FEATURE_INCOMPAT_CSUM_SEED) != 0) {
+ if (EXT4_HAS_INCOMPAT (Partition, EXT4_FEATURE_INCOMPAT_CSUM_SEED)) {
Partition->InitialSeed = Sb->s_checksum_seed;
} else {
Partition->InitialSeed = Ext4CalculateChecksum (Partition, Sb->s_uuid, 16, ~0U);
--
2.39.0

Missing comparison != 0 leads to broken logic condition. Also replaced
CSUM_SEED feature_incompat check with predefined macro EXT4_HAS_INCOMPAT

Cc: Marvin Häuser <mhaeuser@...>
Cc: Pedro Falcato <pedro.falcato@...>
Cc: Vitaly Cheptsov <vit9696@...>
Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
Signed-off-by: Savva Mitrofanov <savvamtr@...>
---
Features/Ext4Pkg/Ext4Dxe/Superblock.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
index 5a3c7f478187..35dcf3c007c8 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -220,13 +220,11 @@ Ext4OpenSuperblock (
}

// At the time of writing, it's the only supported checksum.
- if (Partition->FeaturesCompat & EXT4_FEATURE_RO_COMPAT_METADATA_CSUM &&
- (Sb->s_checksum_type != EXT4_CHECKSUM_CRC32C))
- {
+ if (EXT4_HAS_METADATA_CSUM (Partition) && (Sb->s_checksum_type != EXT4_CHECKSUM_CRC32C)) {
return EFI_UNSUPPORTED;
}

- if ((Partition->FeaturesIncompat & EXT4_FEATURE_INCOMPAT_CSUM_SEED) != 0) {
+ if (EXT4_HAS_INCOMPAT (Partition, EXT4_FEATURE_INCOMPAT_CSUM_SEED)) {
Partition->InitialSeed = Sb->s_checksum_seed;
} else {
Partition->InitialSeed = Ext4CalculateChecksum (Partition, Sb->s_uuid, 16, ~0U);
--
2.39.0