diff mbox series

[edk2,edk2-staging,09/19] IntelUndiPkg/GigUndiDxe: add missing UINT8* cast

Message ID 20181106175833.26964-10-ard.biesheuvel@linaro.org
State Superseded
Headers show
Series IntelUndiPkg/GigUndiDxe: build fixes for AARCH64/ARM/GCC | expand

Commit Message

Ard Biesheuvel Nov. 6, 2018, 5:58 p.m. UTC 
UINT8 and CHAR8 are not the same underlying type on all architectures,
so add an explicit cast where necessary.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 IntelUndiPkg/GigUndiDxe/Hii.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.19.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Philippe Mathieu-Daudé Nov. 6, 2018, 8:31 p.m. UTC | #1 
Hi Ard,

On 6/11/18 18:58, Ard Biesheuvel wrote:
> UINT8 and CHAR8 are not the same underlying type on all architectures,

> so add an explicit cast where necessary.

> 

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>   IntelUndiPkg/GigUndiDxe/Hii.c | 2 +-

>   1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/IntelUndiPkg/GigUndiDxe/Hii.c b/IntelUndiPkg/GigUndiDxe/Hii.c

> index a5d8ae207819..737a59fbbbac 100644

> --- a/IntelUndiPkg/GigUndiDxe/Hii.c

> +++ b/IntelUndiPkg/GigUndiDxe/Hii.c

> @@ -817,7 +817,7 @@ HiiSetMenuStrings (

>   

>         Status = ReadPbaString (

>                    &UndiPrivateData->NicInfo,

> -                 PBAString8,

> +                 (UINT8 *)PBAString8,

>                    MAX_PBA_STR_LENGTH

>                  );

>         if (Status == EFI_SUCCESS) {

> 


I'm not sure why ReadPbaString() takes UINT8* instead of CHAR8*.
Having the device part number stored into a CHAR8[] seems correct, what 
do you think?
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Nov. 6, 2018, 8:35 p.m. UTC | #2 
On 6 November 2018 at 21:31, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> Hi Ard,
>
> On 6/11/18 18:58, Ard Biesheuvel wrote:
>>
>> UINT8 and CHAR8 are not the same underlying type on all architectures,
>> so add an explicit cast where necessary.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>   IntelUndiPkg/GigUndiDxe/Hii.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/IntelUndiPkg/GigUndiDxe/Hii.c b/IntelUndiPkg/GigUndiDxe/Hii.c
>> index a5d8ae207819..737a59fbbbac 100644
>> --- a/IntelUndiPkg/GigUndiDxe/Hii.c
>> +++ b/IntelUndiPkg/GigUndiDxe/Hii.c
>> @@ -817,7 +817,7 @@ HiiSetMenuStrings (
>>           Status = ReadPbaString (
>>                    &UndiPrivateData->NicInfo,
>> -                 PBAString8,
>> +                 (UINT8 *)PBAString8,
>>                    MAX_PBA_STR_LENGTH
>>                  );
>>         if (Status == EFI_SUCCESS) {
>>
>
> I'm not sure why ReadPbaString() takes UINT8* instead of CHAR8*.
> Having the device part number stored into a CHAR8[] seems correct, what do
> you think?

I guess. But that just moves the bubble in the waterbed to elsewhere:

EFI_STATUS
ReadPbaString (
  IN     GIG_DRIVER_DATA *GigAdapter,
  IN OUT UINT8 *          PbaNumber,
  IN     UINT32           PbaNumberSize
  )
{
  if (e1000_read_pba_string (&GigAdapter->Hw, PbaNumber,
PbaNumberSize) == E1000_SUCCESS) {
    return EFI_SUCCESS;
  } else {
    return EFI_DEVICE_ERROR;
  }
}

and

$ git grep e1000_read_pba_string
IntelUndiPkg/GigUndiDxe/e1000.c:  if (e1000_read_pba_string
(&GigAdapter->Hw, PbaNumber, PbaNumberSize) == E1000_SUCCESS) {
IntelUndiPkg/GigUndiDxe/e1000_api.c: *  e1000_read_pba_string - Read
device part number string
IntelUndiPkg/GigUndiDxe/e1000_api.c:s32 e1000_read_pba_string(struct
e1000_hw *hw, u8 *pba_num, u32 pba_num_size)
IntelUndiPkg/GigUndiDxe/e1000_api.c:    return
e1000_read_pba_string_generic(hw, pba_num, pba_num_size);
IntelUndiPkg/GigUndiDxe/e1000_api.h:s32 e1000_read_pba_string(struct
e1000_hw *hw, u8 *pba_num, u32 pba_num_size);
IntelUndiPkg/GigUndiDxe/e1000_nvm.c: *  e1000_read_pba_string_generic
- Read device part number
IntelUndiPkg/GigUndiDxe/e1000_nvm.c:s32
e1000_read_pba_string_generic(struct e1000_hw *hw, u8 *pba_num,
IntelUndiPkg/GigUndiDxe/e1000_nvm.c:
DEBUGFUNC("e1000_read_pba_string_generic");
IntelUndiPkg/GigUndiDxe/e1000_nvm.h:s32
e1000_read_pba_string_generic(struct e1000_hw *hw, u8 *pba_num,

(unless you want to add a cast in ReadPbaString() instead)
Philippe Mathieu-Daudé Nov. 7, 2018, 9:08 a.m. UTC | #3 
On 6/11/18 21:35, Ard Biesheuvel wrote:
> On 6 November 2018 at 21:31, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> Hi Ard,
>>
>> On 6/11/18 18:58, Ard Biesheuvel wrote:
>>>
>>> UINT8 and CHAR8 are not the same underlying type on all architectures,
>>> so add an explicit cast where necessary.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>    IntelUndiPkg/GigUndiDxe/Hii.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/IntelUndiPkg/GigUndiDxe/Hii.c b/IntelUndiPkg/GigUndiDxe/Hii.c
>>> index a5d8ae207819..737a59fbbbac 100644
>>> --- a/IntelUndiPkg/GigUndiDxe/Hii.c
>>> +++ b/IntelUndiPkg/GigUndiDxe/Hii.c
>>> @@ -817,7 +817,7 @@ HiiSetMenuStrings (
>>>            Status = ReadPbaString (
>>>                     &UndiPrivateData->NicInfo,
>>> -                 PBAString8,
>>> +                 (UINT8 *)PBAString8,
>>>                     MAX_PBA_STR_LENGTH
>>>                   );
>>>          if (Status == EFI_SUCCESS) {
>>>
>>
>> I'm not sure why ReadPbaString() takes UINT8* instead of CHAR8*.
>> Having the device part number stored into a CHAR8[] seems correct, what do
>> you think?
> 
> I guess. But that just moves the bubble in the waterbed to elsewhere:
> 
> EFI_STATUS
> ReadPbaString (
>    IN     GIG_DRIVER_DATA *GigAdapter,
>    IN OUT UINT8 *          PbaNumber,
>    IN     UINT32           PbaNumberSize
>    )
> {
>    if (e1000_read_pba_string (&GigAdapter->Hw, PbaNumber,
> PbaNumberSize) == E1000_SUCCESS) {
>      return EFI_SUCCESS;
>    } else {
>      return EFI_DEVICE_ERROR;
>    }
> }
> 
> and
> 
> $ git grep e1000_read_pba_string
> IntelUndiPkg/GigUndiDxe/e1000.c:  if (e1000_read_pba_string
> (&GigAdapter->Hw, PbaNumber, PbaNumberSize) == E1000_SUCCESS) {
> IntelUndiPkg/GigUndiDxe/e1000_api.c: *  e1000_read_pba_string - Read
> device part number string
> IntelUndiPkg/GigUndiDxe/e1000_api.c:s32 e1000_read_pba_string(struct
> e1000_hw *hw, u8 *pba_num, u32 pba_num_size)
> IntelUndiPkg/GigUndiDxe/e1000_api.c:    return
> e1000_read_pba_string_generic(hw, pba_num, pba_num_size);
> IntelUndiPkg/GigUndiDxe/e1000_api.h:s32 e1000_read_pba_string(struct
> e1000_hw *hw, u8 *pba_num, u32 pba_num_size);
> IntelUndiPkg/GigUndiDxe/e1000_nvm.c: *  e1000_read_pba_string_generic
> - Read device part number
> IntelUndiPkg/GigUndiDxe/e1000_nvm.c:s32
> e1000_read_pba_string_generic(struct e1000_hw *hw, u8 *pba_num,
> IntelUndiPkg/GigUndiDxe/e1000_nvm.c:
> DEBUGFUNC("e1000_read_pba_string_generic");
> IntelUndiPkg/GigUndiDxe/e1000_nvm.h:s32
> e1000_read_pba_string_generic(struct e1000_hw *hw, u8 *pba_num,
> 
> (unless you want to add a cast in ReadPbaString() instead)

Hmm I now see the inconsistency.

Since the goal of this series is not to sort it out, then I'm OK with 
your patch.
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Ryszard Knop Jan. 30, 2019, 12:30 p.m. UTC | #4 
Reviewed-by: Ryszard Knop <ryszard.knop@linux.intel.com>


On Tue, 2018-11-06 at 18:58 +0100, ard.biesheuvela wrote:
> UINT8 and CHAR8 are not the same underlying type on all

> architectures,

> so add an explicit cast where necessary.

> 

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>

> ---

>  IntelUndiPkg/GigUndiDxe/Hii.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/IntelUndiPkg/GigUndiDxe/Hii.c

> b/IntelUndiPkg/GigUndiDxe/Hii.c

> index a5d8ae207819..737a59fbbbac 100644

> --- a/IntelUndiPkg/GigUndiDxe/Hii.c

> +++ b/IntelUndiPkg/GigUndiDxe/Hii.c

> @@ -817,7 +817,7 @@ HiiSetMenuStrings (

>  

>        Status = ReadPbaString (

>                   &UndiPrivateData->NicInfo,

> -                 PBAString8,

> +                 (UINT8 *)PBAString8,

>                   MAX_PBA_STR_LENGTH

>                 );

>        if (Status == EFI_SUCCESS) {


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ryszard Knop Jan. 30, 2019, 12:37 p.m. UTC | #5 
On Tue, 2018-11-06 at 21:31 +0100, philmd at redhat.com wrote:
> Hi Ard,

> 

> On 6/11/18 18:58, Ard Biesheuvel wrote:

> > UINT8 and CHAR8 are not the same underlying type on all

> > architectures,

> > so add an explicit cast where necessary.

> > 

> > Contributed-under: TianoCore Contribution Agreement 1.1

> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>

> > ---

> >   IntelUndiPkg/GigUndiDxe/Hii.c | 2 +-

> >   1 file changed, 1 insertion(+), 1 deletion(-)

> > 

> > diff --git a/IntelUndiPkg/GigUndiDxe/Hii.c

> > b/IntelUndiPkg/GigUndiDxe/Hii.c

> > index a5d8ae207819..737a59fbbbac 100644

> > --- a/IntelUndiPkg/GigUndiDxe/Hii.c

> > +++ b/IntelUndiPkg/GigUndiDxe/Hii.c

> > @@ -817,7 +817,7 @@ HiiSetMenuStrings (

> >   

> >         Status = ReadPbaString (

> >                    &UndiPrivateData->NicInfo,

> > -                 PBAString8,

> > +                 (UINT8 *)PBAString8,

> >                    MAX_PBA_STR_LENGTH

> >                  );

> >         if (Status == EFI_SUCCESS) {

> > 

> 

> I'm not sure why ReadPbaString() takes UINT8* instead of CHAR8*.

> Having the device part number stored into a CHAR8[] seems correct,

> what 

> do you think?

I agree that it should use CHAR8, but the underlying code in
e1000_nvm.c is shared between multiple drivers expecting uint8_t or
equivalent, so as Ard said it'd just move the problem elsewhere. It'd
be nice to keep this conversion in ReadPbaString, but as this is the
only place where this function is used, it's fine for now. Hii.c and
friends overall need some solid refactoring if time allows.


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox series

Patch

diff --git a/IntelUndiPkg/GigUndiDxe/Hii.c b/IntelUndiPkg/GigUndiDxe/Hii.c
index a5d8ae207819..737a59fbbbac 100644
--- a/IntelUndiPkg/GigUndiDxe/Hii.c
+++ b/IntelUndiPkg/GigUndiDxe/Hii.c
@@ -817,7 +817,7 @@  HiiSetMenuStrings (
 
       Status = ReadPbaString (
                  &UndiPrivateData->NicInfo,
-                 PBAString8,
+                 (UINT8 *)PBAString8,
                  MAX_PBA_STR_LENGTH
                );
       if (Status == EFI_SUCCESS) {