[PATCH 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue

[PATCH 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

Line number 1348 does contain a typecast with UINT32, but it is after
all the operations (16-bit left shift followed by OR'ing) are over.
To avoid any SIGN_EXTENSION, typecast the intermediate result after
16-bit left shift operation immediately with UINT32.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
---
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
index 50406fe0270d..70c4ca27dc68 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
@@ -1345,7 +1345,7 @@ AtaPassThruPassThru (
     // Check logical block size
     //
     if ((IdentifyData->AtaData.phy_logic_sector_support & BIT12) != 0) {
-      BlockSize = (UINT32)(((IdentifyData->AtaData.logic_sector_size_hi << 16) | IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
+      BlockSize = (((UINT32)(IdentifyData->AtaData.logic_sector_size_hi << 16) | IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
     }
   }
 
-- 
2.34.1

[PATCH 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

The return value stored in Status after call to SetDriveParameters
is not made of any use thereafter and hence it remains as UNUSED.
Assuming, this non-usage is deliberate, the storage in Status can be
done away with.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
---
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
index 75403886e44a..c6d637afa989 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
@@ -2555,7 +2555,7 @@ DetectAndConfigIdeDevice (
       DriveParameters.Heads          = (UINT8)(((ATA5_IDENTIFY_DATA *)(&Buffer.AtaData))->heads - 1);
       DriveParameters.MultipleSector = (UINT8)((ATA5_IDENTIFY_DATA *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
 
-      Status = SetDriveParameters (Instance, IdeChannel, IdeDevice, &DriveParameters, NULL);
+      SetDriveParameters (Instance, IdeChannel, IdeDevice, &DriveParameters, NULL);
     }
 
     //
-- 
2.34.1

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Pedro Falcato]

On Fri, Jun 2, 2023 at 8:42 PM Ranbir Singh <rsingh@ventanamicro.com> wrote:
>
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
>
> The return value stored in Status after call to SetDriveParameters
> is not made of any use thereafter and hence it remains as UNUSED.
> Assuming, this non-usage is deliberate, the storage in Status can be
> done away with.
>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> ---
>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> index 75403886e44a..c6d637afa989 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> @@ -2555,7 +2555,7 @@ DetectAndConfigIdeDevice (
>        DriveParameters.Heads          = (UINT8)(((ATA5_IDENTIFY_DATA *)(&Buffer.AtaData))->heads - 1);
>        DriveParameters.MultipleSector = (UINT8)((ATA5_IDENTIFY_DATA *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
>
> -      Status = SetDriveParameters (Instance, IdeChannel, IdeDevice, &DriveParameters, NULL);
> +      SetDriveParameters (Instance, IdeChannel, IdeDevice, &DriveParameters, NULL);

I'm /fairly/ sure this is wrong and that you need to use Status.


-- 
Pedro

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue

[Ard Biesheuvel]

On Fri, 2 Jun 2023 at 21:42, Ranbir Singh <rsingh@ventanamicro.com> wrote:
>
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
>
> Line number 1348 does contain a typecast with UINT32, but it is after
> all the operations (16-bit left shift followed by OR'ing) are over.
> To avoid any SIGN_EXTENSION, typecast the intermediate result after
> 16-bit left shift operation immediately with UINT32.
>

What is wrong with sign extension?

> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> ---
>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> index 50406fe0270d..70c4ca27dc68 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> @@ -1345,7 +1345,7 @@ AtaPassThruPassThru (
>      // Check logical block size
>      //
>      if ((IdentifyData->AtaData.phy_logic_sector_support & BIT12) != 0) {
> -      BlockSize = (UINT32)(((IdentifyData->AtaData.logic_sector_size_hi << 16) | IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
> +      BlockSize = (((UINT32)(IdentifyData->AtaData.logic_sector_size_hi << 16) | IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));


The outer parens are now redundant, which means you're assigning
something to BlockSize whose type is based on the type of <something>
* sizeof(UINT16), which is unsigned long not unsigned int, so this
will produce a truncation warning on some compilers.

If you want to suppress the coverity warning without introducing new
ones, better to cast the sizeof() to (UINT32).

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Ard Biesheuvel]

On Sat, 3 Jun 2023 at 18:04, Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> On Fri, Jun 2, 2023 at 8:42 PM Ranbir Singh <rsingh@ventanamicro.com> wrote:
> >
> > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >
> > The return value stored in Status after call to SetDriveParameters
> > is not made of any use thereafter and hence it remains as UNUSED.
> > Assuming, this non-usage is deliberate, the storage in Status can be
> > done away with.
> >
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > ---
> >  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > index 75403886e44a..c6d637afa989 100644
> > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > @@ -2555,7 +2555,7 @@ DetectAndConfigIdeDevice (
> >        DriveParameters.Heads          = (UINT8)(((ATA5_IDENTIFY_DATA *)(&Buffer.AtaData))->heads - 1);
> >        DriveParameters.MultipleSector = (UINT8)((ATA5_IDENTIFY_DATA *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
> >
> > -      Status = SetDriveParameters (Instance, IdeChannel, IdeDevice, &DriveParameters, NULL);
> > +      SetDriveParameters (Instance, IdeChannel, IdeDevice, &DriveParameters, NULL);
>
> I'm /fairly/ sure this is wrong and that you need to use Status.
>

Yeah, removing the assignment fixes the coverity warning, but now you
are hiding a bug instead of fixing it.

SetDriveParameters () can apparently fail, and this is being ignored.
At the very least, we should emit a diagnostic here in DEBUG mode to
log this. E.g.,

DEBUG ((DEBUG_WARN, "%a: SetDriveParameters () failed - %r\n",
__func__, Status));

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 2597 bytes --]

Please read the Coverity report attached in BZ 4204 for more details on
sign-extension issue.

The data types of logic_sector_size_hi and logic_sector_size_lo are UINT16
and Isn't the return type of *sizeof* already unsigned ?

Now I have no means to run Coverity and test further changes.
Anyway, my understanding back then was that the sign-extension was
primarily happening because of the 16 bits left shift operation. I did test
the patch for coverity warning resolution back in Jan'23 which went off.


On Mon, Jun 5, 2023 at 4:28 AM Ard Biesheuvel <ardb@kernel.org> wrote:

> On Fri, 2 Jun 2023 at 21:42, Ranbir Singh <rsingh@ventanamicro.com> wrote:
> >
> > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >
> > Line number 1348 does contain a typecast with UINT32, but it is after
> > all the operations (16-bit left shift followed by OR'ing) are over.
> > To avoid any SIGN_EXTENSION, typecast the intermediate result after
> > 16-bit left shift operation immediately with UINT32.
> >
>
> What is wrong with sign extension?
>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > ---
> >  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> > index 50406fe0270d..70c4ca27dc68 100644
> > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> > @@ -1345,7 +1345,7 @@ AtaPassThruPassThru (
> >      // Check logical block size
> >      //
> >      if ((IdentifyData->AtaData.phy_logic_sector_support & BIT12) != 0) {
> > -      BlockSize = (UINT32)(((IdentifyData->AtaData.logic_sector_size_hi
> << 16) | IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
> > +      BlockSize = (((UINT32)(IdentifyData->AtaData.logic_sector_size_hi
> << 16) | IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
>
>
> The outer parens are now redundant, which means you're assigning
> something to BlockSize whose type is based on the type of <something>
> * sizeof(UINT16), which is unsigned long not unsigned int, so this
> will produce a truncation warning on some compilers.
>
> If you want to suppress the coverity warning without introducing new
> ones, better to cast the sizeof() to (UINT32).
>

[-- Attachment #2: Type: text/html, Size: 3843 bytes --]

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 2763 bytes --]

I counted myself as not the right person to decide what all to do if
*Status* is not successful. Adding the DEBUG statement from the Coverity
aspect doesn't count as a fix as RELEASE mode behavior remains the same.

In the comments / description, I already mentioned - Assuming, this
non-usage is deliberate, so as such I did not intend to hide anything - and
left it in the status quo.

The patch proposed may not be appropriate, but should now give a thinking
point to active module developers / owners / maintainers if they indeed
feel that there is a bug here and it needs to be fixed.

On Mon, Jun 5, 2023 at 4:33 AM Ard Biesheuvel <ardb@kernel.org> wrote:

> On Sat, 3 Jun 2023 at 18:04, Pedro Falcato <pedro.falcato@gmail.com>
> wrote:
> >
> > On Fri, Jun 2, 2023 at 8:42 PM Ranbir Singh <rsingh@ventanamicro.com>
> wrote:
> > >
> > > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > >
> > > The return value stored in Status after call to SetDriveParameters
> > > is not made of any use thereafter and hence it remains as UNUSED.
> > > Assuming, this non-usage is deliberate, the storage in Status can be
> > > done away with.
> > >
> > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
> > > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > > ---
> > >  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > > index 75403886e44a..c6d637afa989 100644
> > > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > > @@ -2555,7 +2555,7 @@ DetectAndConfigIdeDevice (
> > >        DriveParameters.Heads          = (UINT8)(((ATA5_IDENTIFY_DATA
> *)(&Buffer.AtaData))->heads - 1);
> > >        DriveParameters.MultipleSector = (UINT8)((ATA5_IDENTIFY_DATA
> *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
> > >
> > > -      Status = SetDriveParameters (Instance, IdeChannel, IdeDevice,
> &DriveParameters, NULL);
> > > +      SetDriveParameters (Instance, IdeChannel, IdeDevice,
> &DriveParameters, NULL);
> >
> > I'm /fairly/ sure this is wrong and that you need to use Status.
> >
>
> Yeah, removing the assignment fixes the coverity warning, but now you
> are hiding a bug instead of fixing it.
>
> SetDriveParameters () can apparently fail, and this is being ignored.
> At the very least, we should emit a diagnostic here in DEBUG mode to
> log this. E.g.,
>
> DEBUG ((DEBUG_WARN, "%a: SetDriveParameters () failed - %r\n",
> __func__, Status));
>

[-- Attachment #2: Type: text/html, Size: 3874 bytes --]

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Ard Biesheuvel]

On Mon, 5 Jun 2023 at 10:10, Ranbir Singh <rsingh@ventanamicro.com> wrote:
>
> I counted myself as not the right person to decide what all to do if Status is not successful. Adding the DEBUG statement from the Coverity aspect doesn't count as a fix as RELEASE mode behavior remains the same.
>
> In the comments / description, I already mentioned - Assuming, this non-usage is deliberate, so as such I did not intend to hide anything - and left it in the status quo.
>
> The patch proposed may not be appropriate, but should now give a thinking point to active module developers / owners / maintainers if they indeed feel that there is a bug here and it needs to be fixed.
>

Thanks - I agree that it is a good thing that people are aware of this now.

> On Mon, Jun 5, 2023 at 4:33 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Sat, 3 Jun 2023 at 18:04, Pedro Falcato <pedro.falcato@gmail.com> wrote:
>> >
>> > On Fri, Jun 2, 2023 at 8:42 PM Ranbir Singh <rsingh@ventanamicro.com> wrote:
>> > >
>> > > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
>> > >
>> > > The return value stored in Status after call to SetDriveParameters
>> > > is not made of any use thereafter and hence it remains as UNUSED.
>> > > Assuming, this non-usage is deliberate, the storage in Status can be
>> > > done away with.
>> > >
>> > > Cc: Hao A Wu <hao.a.wu@intel.com>
>> > > Cc: Ray Ni <ray.ni@intel.com>
>> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
>> > > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
>> > > ---
>> > >  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 2 +-
>> > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > >
>> > > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
>> > > index 75403886e44a..c6d637afa989 100644
>> > > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
>> > > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
>> > > @@ -2555,7 +2555,7 @@ DetectAndConfigIdeDevice (
>> > >        DriveParameters.Heads          = (UINT8)(((ATA5_IDENTIFY_DATA *)(&Buffer.AtaData))->heads - 1);
>> > >        DriveParameters.MultipleSector = (UINT8)((ATA5_IDENTIFY_DATA *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
>> > >
>> > > -      Status = SetDriveParameters (Instance, IdeChannel, IdeDevice, &DriveParameters, NULL);
>> > > +      SetDriveParameters (Instance, IdeChannel, IdeDevice, &DriveParameters, NULL);
>> >
>> > I'm /fairly/ sure this is wrong and that you need to use Status.
>> >
>>
>> Yeah, removing the assignment fixes the coverity warning, but now you
>> are hiding a bug instead of fixing it.
>>
>> SetDriveParameters () can apparently fail, and this is being ignored.
>> At the very least, we should emit a diagnostic here in DEBUG mode to
>> log this. E.g.,
>>
>> DEBUG ((DEBUG_WARN, "%a: SetDriveParameters () failed - %r\n",
>> __func__, Status));

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue

[Ard Biesheuvel]

On Mon, 5 Jun 2023 at 09:54, Ranbir Singh <rsingh@ventanamicro.com> wrote:
>
> Please read the Coverity report attached in BZ 4204 for more details on sign-extension issue.
>

I did read it - explanation below.

> The data types of logic_sector_size_hi and logic_sector_size_lo are UINT16 and Isn't the return type of sizeof already unsigned ?
>

Yes, it is unsigned long, so 64 bits wide on LP64 architectures

> Now I have no means to run Coverity and test further changes.
> Anyway, my understanding back then was that the sign-extension was primarily happening because of the 16 bits left shift operation. I did test the patch for coverity warning resolution back in Jan'23 which went off.
>

The warning actually describes exactly what is happening:
- The type of a UINT16 shifted left by 16 is promoted to signed integer.
- The multiplication by sizeof() converts the former result to
unsigned integer before widening to unsigned long integer.

This means that the widening does not perform any sign extension,
which is what Coverity warns about.

None of this actually matters, given that those upper bits get
truncated again anyway when we assign to BlockSize, which is only 32
bits wide.

Removing the outer (UINT32) cast is not the correct solution, as it
may result in spurious truncation warnings on some toolchains.

If you want to silence this warning (I wouldn't call it a fix), you
need to either prevent the widening, or cast the intermediate result
to (UINT32), and the latter is what your patch does. So it silences
the warning, but now, the type of the right hand side of the
assignment is UINTN not UINT32.

So either leave the outer (UINT32) cast in place, or move it to before
the sizeof() as I suggested before: in both cases, there is no longer
any promotion to unsigned long, which should make the warning go away.



>
> On Mon, Jun 5, 2023 at 4:28 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Fri, 2 Jun 2023 at 21:42, Ranbir Singh <rsingh@ventanamicro.com> wrote:
>> >
>> > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
>> >
>> > Line number 1348 does contain a typecast with UINT32, but it is after
>> > all the operations (16-bit left shift followed by OR'ing) are over.
>> > To avoid any SIGN_EXTENSION, typecast the intermediate result after
>> > 16-bit left shift operation immediately with UINT32.
>> >
>>
>> What is wrong with sign extension?
>>
>> > Cc: Hao A Wu <hao.a.wu@intel.com>
>> > Cc: Ray Ni <ray.ni@intel.com>
>> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
>> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
>> > ---
>> >  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
>> > index 50406fe0270d..70c4ca27dc68 100644
>> > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
>> > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
>> > @@ -1345,7 +1345,7 @@ AtaPassThruPassThru (
>> >      // Check logical block size
>> >      //
>> >      if ((IdentifyData->AtaData.phy_logic_sector_support & BIT12) != 0) {
>> > -      BlockSize = (UINT32)(((IdentifyData->AtaData.logic_sector_size_hi << 16) | IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
>> > +      BlockSize = (((UINT32)(IdentifyData->AtaData.logic_sector_size_hi << 16) | IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
>>
>>
>> The outer parens are now redundant, which means you're assigning
>> something to BlockSize whose type is based on the type of <something>
>> * sizeof(UINT16), which is unsigned long not unsigned int, so this
>> will produce a truncation warning on some compilers.
>>
>> If you want to suppress the coverity warning without introducing new
>> ones, better to cast the sizeof() to (UINT32).

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 4701 bytes --]

Thanks Ard for adding the explanation.

Ok, so retaining outer cast along with intermediate casting

BlockSize = (UINT32)(((UINT32)(IdentifyData->AtaData.logic_sector_size_hi
<< 16) | IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));

should be acceptable. I considered outer cast is being implicitly done as
per the data type of *BlockSize *and also there was no Coverity warning
after the removal*.*




On Mon, Jun 5, 2023 at 2:14 PM Ard Biesheuvel <ardb@kernel.org> wrote:

> On Mon, 5 Jun 2023 at 09:54, Ranbir Singh <rsingh@ventanamicro.com> wrote:
> >
> > Please read the Coverity report attached in BZ 4204 for more details on
> sign-extension issue.
> >
>
> I did read it - explanation below.
>
> > The data types of logic_sector_size_hi and logic_sector_size_lo are
> UINT16 and Isn't the return type of sizeof already unsigned ?
> >
>
> Yes, it is unsigned long, so 64 bits wide on LP64 architectures
>
> > Now I have no means to run Coverity and test further changes.
> > Anyway, my understanding back then was that the sign-extension was
> primarily happening because of the 16 bits left shift operation. I did test
> the patch for coverity warning resolution back in Jan'23 which went off.
> >
>
> The warning actually describes exactly what is happening:
> - The type of a UINT16 shifted left by 16 is promoted to signed integer.
> - The multiplication by sizeof() converts the former result to
> unsigned integer before widening to unsigned long integer.
>
> This means that the widening does not perform any sign extension,
> which is what Coverity warns about.
>
> None of this actually matters, given that those upper bits get
> truncated again anyway when we assign to BlockSize, which is only 32
> bits wide.
>
> Removing the outer (UINT32) cast is not the correct solution, as it
> may result in spurious truncation warnings on some toolchains.
>
> If you want to silence this warning (I wouldn't call it a fix), you
> need to either prevent the widening, or cast the intermediate result
> to (UINT32), and the latter is what your patch does. So it silences
> the warning, but now, the type of the right hand side of the
> assignment is UINTN not UINT32.
>
> So either leave the outer (UINT32) cast in place, or move it to before
> the sizeof() as I suggested before: in both cases, there is no longer
> any promotion to unsigned long, which should make the warning go away.
>
>
>
> >
> > On Mon, Jun 5, 2023 at 4:28 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >>
> >> On Fri, 2 Jun 2023 at 21:42, Ranbir Singh <rsingh@ventanamicro.com>
> wrote:
> >> >
> >> > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >> >
> >> > Line number 1348 does contain a typecast with UINT32, but it is after
> >> > all the operations (16-bit left shift followed by OR'ing) are over.
> >> > To avoid any SIGN_EXTENSION, typecast the intermediate result after
> >> > 16-bit left shift operation immediately with UINT32.
> >> >
> >>
> >> What is wrong with sign extension?
> >>
> >> > Cc: Hao A Wu <hao.a.wu@intel.com>
> >> > Cc: Ray Ni <ray.ni@intel.com>
> >> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
> >> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >> > ---
> >> >  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> >> > index 50406fe0270d..70c4ca27dc68 100644
> >> > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> >> > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> >> > @@ -1345,7 +1345,7 @@ AtaPassThruPassThru (
> >> >      // Check logical block size
> >> >      //
> >> >      if ((IdentifyData->AtaData.phy_logic_sector_support & BIT12) !=
> 0) {
> >> > -      BlockSize =
> (UINT32)(((IdentifyData->AtaData.logic_sector_size_hi << 16) |
> IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
> >> > +      BlockSize =
> (((UINT32)(IdentifyData->AtaData.logic_sector_size_hi << 16) |
> IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
> >>
> >>
> >> The outer parens are now redundant, which means you're assigning
> >> something to BlockSize whose type is based on the type of <something>
> >> * sizeof(UINT16), which is unsigned long not unsigned int, so this
> >> will produce a truncation warning on some compilers.
> >>
> >> If you want to suppress the coverity warning without introducing new
> >> ones, better to cast the sizeof() to (UINT32).
>

[-- Attachment #2: Type: text/html, Size: 6878 bytes --]

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Wu, Hao A]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> Biesheuvel
> Sent: Monday, June 5, 2023 4:32 PM
> To: Ranbir Singh <rsingh@ventanamicro.com>
> Cc: devel@edk2.groups.io; pedro.falcato@gmail.com; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH 2/2]
> MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity
> issue
> 
> On Mon, 5 Jun 2023 at 10:10, Ranbir Singh <rsingh@ventanamicro.com>
> wrote:
> >
> > I counted myself as not the right person to decide what all to do if Status is
> not successful. Adding the DEBUG statement from the Coverity aspect
> doesn't count as a fix as RELEASE mode behavior remains the same.
> >
> > In the comments / description, I already mentioned - Assuming, this non-
> usage is deliberate, so as such I did not intend to hide anything - and left it in
> the status quo.
> >
> > The patch proposed may not be appropriate, but should now give a
> thinking point to active module developers / owners / maintainers if they
> indeed feel that there is a bug here and it needs to be fixed.
> >
> 
> Thanks - I agree that it is a good thing that people are aware of this now.


Judging from the context in DetectAndConfigIdeDevice(), I think a fix can be:

  Status = SetDriveParameters (Instance, IdeChannel, IdeDevice, &DriveParameters, NULL);
  if (EFI_ERROR (Status)) {
    DEBUG ((DEBUG_ERROR, "Set drive parameters Fail, Status = %r\n", Status));
    continue;
  }

But I do not have the device and platform environment to verify the change.
Really sorry for this. I am not sure on the potential risk of making this change without at least some kind of 'no-break' tests.

Best Regards,
Hao Wu


> 
> > On Mon, Jun 5, 2023 at 4:33 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >>
> >> On Sat, 3 Jun 2023 at 18:04, Pedro Falcato <pedro.falcato@gmail.com>
> wrote:
> >> >
> >> > On Fri, Jun 2, 2023 at 8:42 PM Ranbir Singh <rsingh@ventanamicro.com>
> wrote:
> >> > >
> >> > > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >> > >
> >> > > The return value stored in Status after call to SetDriveParameters
> >> > > is not made of any use thereafter and hence it remains as UNUSED.
> >> > > Assuming, this non-usage is deliberate, the storage in Status can be
> >> > > done away with.
> >> > >
> >> > > Cc: Hao A Wu <hao.a.wu@intel.com>
> >> > > Cc: Ray Ni <ray.ni@intel.com>
> >> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
> >> > > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >> > > ---
> >> > >  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 2 +-
> >> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> > >
> >> > > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> >> > > index 75403886e44a..c6d637afa989 100644
> >> > > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> >> > > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> >> > > @@ -2555,7 +2555,7 @@ DetectAndConfigIdeDevice (
> >> > >        DriveParameters.Heads          = (UINT8)(((ATA5_IDENTIFY_DATA
> *)(&Buffer.AtaData))->heads - 1);
> >> > >        DriveParameters.MultipleSector = (UINT8)((ATA5_IDENTIFY_DATA
> *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
> >> > >
> >> > > -      Status = SetDriveParameters (Instance, IdeChannel, IdeDevice,
> &DriveParameters, NULL);
> >> > > +      SetDriveParameters (Instance, IdeChannel, IdeDevice,
> &DriveParameters, NULL);
> >> >
> >> > I'm /fairly/ sure this is wrong and that you need to use Status.
> >> >
> >>
> >> Yeah, removing the assignment fixes the coverity warning, but now you
> >> are hiding a bug instead of fixing it.
> >>
> >> SetDriveParameters () can apparently fail, and this is being ignored.
> >> At the very least, we should emit a diagnostic here in DEBUG mode to
> >> log this. E.g.,
> >>
> >> DEBUG ((DEBUG_WARN, "%a: SetDriveParameters () failed - %r\n",
> >> __func__, Status));
> 
> 
> 
> 

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 4686 bytes --]

I mentioned similar approach in
https://bugzilla.tianocore.org/show_bug.cgi?id=4204#c1

Let me know if I should update the patch as Hao proposed -

if (EFI_ERROR (Status)) {
    DEBUG ((DEBUG_ERROR, "Set drive parameters Fail, Status = %r\n",
Status));
    continue;
  }

On Thu, Jun 8, 2023 at 12:25 PM Wu, Hao A <hao.a.wu@intel.com> wrote:

> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> > Biesheuvel
> > Sent: Monday, June 5, 2023 4:32 PM
> > To: Ranbir Singh <rsingh@ventanamicro.com>
> > Cc: devel@edk2.groups.io; pedro.falcato@gmail.com; Wu, Hao A
> > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> > Subject: Re: [edk2-devel] [PATCH 2/2]
> > MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity
> > issue
> >
> > On Mon, 5 Jun 2023 at 10:10, Ranbir Singh <rsingh@ventanamicro.com>
> > wrote:
> > >
> > > I counted myself as not the right person to decide what all to do if
> Status is
> > not successful. Adding the DEBUG statement from the Coverity aspect
> > doesn't count as a fix as RELEASE mode behavior remains the same.
> > >
> > > In the comments / description, I already mentioned - Assuming, this
> non-
> > usage is deliberate, so as such I did not intend to hide anything - and
> left it in
> > the status quo.
> > >
> > > The patch proposed may not be appropriate, but should now give a
> > thinking point to active module developers / owners / maintainers if they
> > indeed feel that there is a bug here and it needs to be fixed.
> > >
> >
> > Thanks - I agree that it is a good thing that people are aware of this
> now.
>
>
> Judging from the context in DetectAndConfigIdeDevice(), I think a fix can
> be:
>
>   Status = SetDriveParameters (Instance, IdeChannel, IdeDevice,
> &DriveParameters, NULL);
>   if (EFI_ERROR (Status)) {
>     DEBUG ((DEBUG_ERROR, "Set drive parameters Fail, Status = %r\n",
> Status));
>     continue;
>   }
>
> But I do not have the device and platform environment to verify the change.
> Really sorry for this. I am not sure on the potential risk of making this
> change without at least some kind of 'no-break' tests.
>
> Best Regards,
> Hao Wu
>
>
> >
> > > On Mon, Jun 5, 2023 at 4:33 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >>
> > >> On Sat, 3 Jun 2023 at 18:04, Pedro Falcato <pedro.falcato@gmail.com>
> > wrote:
> > >> >
> > >> > On Fri, Jun 2, 2023 at 8:42 PM Ranbir Singh <
> rsingh@ventanamicro.com>
> > wrote:
> > >> > >
> > >> > > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > >> > >
> > >> > > The return value stored in Status after call to SetDriveParameters
> > >> > > is not made of any use thereafter and hence it remains as UNUSED.
> > >> > > Assuming, this non-usage is deliberate, the storage in Status can
> be
> > >> > > done away with.
> > >> > >
> > >> > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > >> > > Cc: Ray Ni <ray.ni@intel.com>
> > >> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
> > >> > > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > >> > > ---
> > >> > >  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 2 +-
> > >> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >> > >
> > >> > > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > >> > > index 75403886e44a..c6d637afa989 100644
> > >> > > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > >> > > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > >> > > @@ -2555,7 +2555,7 @@ DetectAndConfigIdeDevice (
> > >> > >        DriveParameters.Heads          =
> (UINT8)(((ATA5_IDENTIFY_DATA
> > *)(&Buffer.AtaData))->heads - 1);
> > >> > >        DriveParameters.MultipleSector =
> (UINT8)((ATA5_IDENTIFY_DATA
> > *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
> > >> > >
> > >> > > -      Status = SetDriveParameters (Instance, IdeChannel,
> IdeDevice,
> > &DriveParameters, NULL);
> > >> > > +      SetDriveParameters (Instance, IdeChannel, IdeDevice,
> > &DriveParameters, NULL);
> > >> >
> > >> > I'm /fairly/ sure this is wrong and that you need to use Status.
> > >> >
> > >>
> > >> Yeah, removing the assignment fixes the coverity warning, but now you
> > >> are hiding a bug instead of fixing it.
> > >>
> > >> SetDriveParameters () can apparently fail, and this is being ignored.
> > >> At the very least, we should emit a diagnostic here in DEBUG mode to
> > >> log this. E.g.,
> > >>
> > >> DEBUG ((DEBUG_WARN, "%a: SetDriveParameters () failed - %r\n",
> > >> __func__, Status));
> >
> >
> > 
> >
>
>

[-- Attachment #2: Type: text/html, Size: 7001 bytes --]

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Wu, Hao A]

[-- Attachment #1: Type: text/plain, Size: 5624 bytes --]

Hello,

Is it possible for you to verify the proposed change on a hard disk working under IDE mode to see:
  a) If it can still be successfully recognized;
  b) The SetDriveParameters call has been reached and returns with no error.

My opinion is that the change needs to be tested at least to ensure it will not bring issue to previously working devices.

Best Regards,
Hao Wu

From: Ranbir Singh <rsingh@ventanamicro.com>
Sent: Thursday, June 8, 2023 6:17 PM
To: Wu, Hao A <hao.a.wu@intel.com>
Cc: devel@edk2.groups.io; ardb@kernel.org; pedro.falcato@gmail.com; Ni, Ray <ray.ni@intel.com>
Subject: Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

I mentioned similar approach in https://bugzilla.tianocore.org/show_bug.cgi?id=4204#c1

Let me know if I should update the patch as Hao proposed -

if (EFI_ERROR (Status)) {
    DEBUG ((DEBUG_ERROR, "Set drive parameters Fail, Status = %r\n", Status));
    continue;
  }

On Thu, Jun 8, 2023 at 12:25 PM Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>> wrote:
> -----Original Message-----
> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Ard
> Biesheuvel
> Sent: Monday, June 5, 2023 4:32 PM
> To: Ranbir Singh <rsingh@ventanamicro.com<mailto:rsingh@ventanamicro.com>>
> Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; pedro.falcato@gmail.com<mailto:pedro.falcato@gmail.com>; Wu, Hao A
> <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> Subject: Re: [edk2-devel] [PATCH 2/2]
> MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity
> issue
>
> On Mon, 5 Jun 2023 at 10:10, Ranbir Singh <rsingh@ventanamicro.com<mailto:rsingh@ventanamicro.com>>
> wrote:
> >
> > I counted myself as not the right person to decide what all to do if Status is
> not successful. Adding the DEBUG statement from the Coverity aspect
> doesn't count as a fix as RELEASE mode behavior remains the same.
> >
> > In the comments / description, I already mentioned - Assuming, this non-
> usage is deliberate, so as such I did not intend to hide anything - and left it in
> the status quo.
> >
> > The patch proposed may not be appropriate, but should now give a
> thinking point to active module developers / owners / maintainers if they
> indeed feel that there is a bug here and it needs to be fixed.
> >
>
> Thanks - I agree that it is a good thing that people are aware of this now.


Judging from the context in DetectAndConfigIdeDevice(), I think a fix can be:

  Status = SetDriveParameters (Instance, IdeChannel, IdeDevice, &DriveParameters, NULL);
  if (EFI_ERROR (Status)) {
    DEBUG ((DEBUG_ERROR, "Set drive parameters Fail, Status = %r\n", Status));
    continue;
  }

But I do not have the device and platform environment to verify the change.
Really sorry for this. I am not sure on the potential risk of making this change without at least some kind of 'no-break' tests.

Best Regards,
Hao Wu


>
> > On Mon, Jun 5, 2023 at 4:33 AM Ard Biesheuvel <ardb@kernel.org<mailto:ardb@kernel.org>> wrote:
> >>
> >> On Sat, 3 Jun 2023 at 18:04, Pedro Falcato <pedro.falcato@gmail.com<mailto:pedro.falcato@gmail.com>>
> wrote:
> >> >
> >> > On Fri, Jun 2, 2023 at 8:42 PM Ranbir Singh <rsingh@ventanamicro.com<mailto:rsingh@ventanamicro.com>>
> wrote:
> >> > >
> >> > > From: Ranbir Singh <Ranbir.Singh3@Dell.com<mailto:Ranbir.Singh3@Dell.com>>
> >> > >
> >> > > The return value stored in Status after call to SetDriveParameters
> >> > > is not made of any use thereafter and hence it remains as UNUSED.
> >> > > Assuming, this non-usage is deliberate, the storage in Status can be
> >> > > done away with.
> >> > >
> >> > > Cc: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
> >> > > Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> >> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
> >> > > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com<mailto:Ranbir.Singh3@Dell.com>>
> >> > > ---
> >> > >  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 2 +-
> >> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> > >
> >> > > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> >> > > index 75403886e44a..c6d637afa989 100644
> >> > > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> >> > > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> >> > > @@ -2555,7 +2555,7 @@ DetectAndConfigIdeDevice (
> >> > >        DriveParameters.Heads          = (UINT8)(((ATA5_IDENTIFY_DATA
> *)(&Buffer.AtaData))->heads - 1);
> >> > >        DriveParameters.MultipleSector = (UINT8)((ATA5_IDENTIFY_DATA
> *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
> >> > >
> >> > > -      Status = SetDriveParameters (Instance, IdeChannel, IdeDevice,
> &DriveParameters, NULL);
> >> > > +      SetDriveParameters (Instance, IdeChannel, IdeDevice,
> &DriveParameters, NULL);
> >> >
> >> > I'm /fairly/ sure this is wrong and that you need to use Status.
> >> >
> >>
> >> Yeah, removing the assignment fixes the coverity warning, but now you
> >> are hiding a bug instead of fixing it.
> >>
> >> SetDriveParameters () can apparently fail, and this is being ignored.
> >> At the very least, we should emit a diagnostic here in DEBUG mode to
> >> log this. E.g.,
> >>
> >> DEBUG ((DEBUG_WARN, "%a: SetDriveParameters () failed - %r\n",
> >> __func__, Status));
>
>
> 
>

[-- Attachment #2: Type: text/html, Size: 10635 bytes --]