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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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>