Re: [edk2-platform][PATCH v1 3/4] Silicon/AsixPkg: Add ASIX Ax88179 driver

["Samer El-Haj-Mahmoud" <samer.el-haj-mahmoud@arm.com>]

> -----Original Message-----
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: Tuesday, May 12, 2020 9:03 AM
> To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> Cc: devel@edk2.groups.io; Ray Ni <ray.ni@intel.com>; Ard Biesheuvel
> <Ard.Biesheuvel@arm.com>; Michael D Kinney
> <michael.d.kinney@intel.com>
> Subject: Re: [edk2-platform][PATCH v1 3/4] Silicon/AsixPkg: Add ASIX
> Ax88179 driver
>
> On Sun, May 10, 2020 at 07:15:29 -0400, Samer El-Haj-Mahmoud wrote:
> > This is the initial revision of ASIX USB networking UEFI driver
> > version 2.9.0 for Ax88179 and Ax88178a
> >
> https://www.asix.com.tw/products.php?op=pItemdetail&PItemID=131;71;1
> 12
> >
> > Source code contributed by ASIX.
>
> I don't object to the shout-out, but can any more information be
> given? That could be useful for archeological purposes.
> Through a provate git repo? If so, at what commit?
> Through a .zip file? If so, what was it called?
>

Yes the source is in a ZIP file that I made available here:
https://github.com/samerhaj/uefi_drivers/blob/master/UsbNetworking/Asix/zip/source/AX88179_178a_UEFI_v2.9.0_Source.zip
Will add a reference in v2

> Also, semantics - could you say "provided" rather than "contributed"?
Sure


> > +## @file
> > +# Component description file for ASIX AX88772 USB/Ethernet driver.
> > +#
> > +# This module provides support for the ASIX AX88772 USB/Ethernet
> adapter.
> > +# Copyright (c) 2011, Intel Corporation
>
> I first thought this was a copyright line retained from using a
> template, but the same appears for all files in patch.
> Why does Intel own the copyright on sources from ASIX.

I have no clue. The code came from ASIX with these Intel copyrights. I just updated the license from
"BSD License' to "BSD-2-Clause-Patent".

I also noticed the original drivers carry the Intel copyrights, and have been so since the initial contribution:
e.g. https://github.com/tianocore/edk2/commit/8dde1f6e9574967b71dc4c6ca15cb7cfd16b38a5
I am speculating, but maybe Intel wrote the original code, and ASIX never added their copyrights?


> > +
> > +
> > +#define PASS_SCT      0
>
> O'Rly?
> This shouldn't even be possible to disable.
😊 Hey I am just the messenger.
I will enable it and see if anything breaks, and if so, will remove the #define

> > +//#if defined(_MSC_VER)           /* Handle Microsoft VC++ compiler
> specifics. */
> > +#define DBG_ENTER()             DEBUG (( 0xffffffff, "Entering "
> __FUNCTION__ "\n" )) ///<  Display routine entry
> > +#define DBG_EXIT()              DEBUG (( 0xffffffff, "Exiting " __FUNCTION__
> "\n" ))  ///<  Display routine exit
> > +#define DBG_EXIT_DEC(Status)    DEBUG (( 0xffffffff, "Exiting "
> __FUNCTION__ ", Status: %d\n", Status ))      ///<  Display routine exit with
> decimal value
> > +#define DBG_EXIT_HEX(Status)    DEBUG (( 0xffffffff, "Exiting "
> __FUNCTION__ ", Status: 0x%08x\n", Status ))  ///<  Display routine exit with
> hex value
> > +#define DBG_EXIT_STATUS(Status) DEBUG (( 0xffffffff, "Exiting "
> __FUNCTION__ ", Status: %r\n", Status ))      ///<  Display routine exit with
> status value
> > +#define DBG_EXIT_TF(Status)     DEBUG (( 0xffffffff, "Exiting "
> __FUNCTION__ ", returning %s\n", (FALSE == Status) ? L"FALSE" : L"TRUE" ))
> ///<  Display routine with TRUE/FALSE value
> > +//#else   //  _MSC_VER
> > +//#define DBG_ENTER()               ///<  Display routine entry
> > +//#define DBG_EXIT()                ///<  Display routine exit
> > +//#define DBG_EXIT_DEC(Status)      ///<  Display routine exit with decimal
> value
> > +//#define DBG_EXIT_HEX(Status)      ///<  Display routine exit with hex
> value
> > +//#define DBG_EXIT_STATUS(Status)   ///<  Display routine exit with
> status value
> > +//#define DBG_EXIT_TF(Status)       ///<  Display routine with TRUE/FALSE
> value
>
> I don't see these used at all (regardless of toolchain flavour)?
> Can they be dropped?
>
I actually see these debug macros used in both drivers.
I can fix the #if macro above to use a #define instead of the toolchain.




> > +
> > +
> > +/*Freddy*/
> > +#define RXC_RH1M          0x0100  ///<  Rx header 1
> > +#define RXC_RH2M          0x0200  ///<  Rx header 2
> > +#define RXC_RH3M          0x0400  ///<  Rx header 3
> > +/*Freddy*/
>
> Unused.
>

I only see RXC_RH1M  used once in the other driver. Will remove the other macros and the "Freddy" comments



> > +
> > +
> > +#if PASS_SCT
>
> Drop conditional.
>
> > +EFI_STATUS
> > +EFIAPI
> > +GetControllerName (
> > +  IN  EFI_COMPONENT_NAME_PROTOCOL * pThis,
> > +  IN  EFI_HANDLE ControllerHandle,
> > +  IN OPTIONAL EFI_HANDLE ChildHandle,
> > +  IN  CHAR8 * pLanguage,
> > +  OUT CHAR16 ** ppControllerName
> > +  )
> > +{
> > +
> > +  EFI_STATUS                  Status;
> > +  EFI_USB_IO_PROTOCOL         *UsbIoProtocol;
> > +
> > +  //
> > +  // This is a device driver, so ChildHandle must be NULL.
> > +  //
> > +  if (ChildHandle != NULL) {
> > +    return EFI_UNSUPPORTED;
> > +  }
> > +
> > +  //
> > +  // Check Controller's handle
> > +  //
> > +  Status = gBS->OpenProtocol (
> > +                  ControllerHandle,
> > +                  &gEfiUsbIoProtocolGuid,
> > +                  (VOID **) &UsbIoProtocol,
> > +                  gDriverBinding.DriverBindingHandle,
> > +                  ControllerHandle,
> > +                  EFI_OPEN_PROTOCOL_BY_DRIVER
> > +                  );
> > +  if (!EFI_ERROR (Status)) {
> > +    gBS->CloseProtocol (
> > +           ControllerHandle,
> > +           &gEfiUsbIoProtocolGuid,
> > +           gDriverBinding.DriverBindingHandle,
> > +           ControllerHandle
> > +           );
> > +    return EFI_UNSUPPORTED;
> > +  }
> > +
> > +  if (Status != EFI_ALREADY_STARTED) {
> > +    return EFI_UNSUPPORTED;
> > +  }
> > +
> > +  Status =  LookupUnicodeString2 (
> > +           pLanguage,
> > +            pThis->SupportedLanguages,
> > +           mControllerNameTable,
> > +           ppControllerName,
> > +           (BOOLEAN)(pThis == &gComponentName)
> > +           );
> > +
> > +  return  Status;
> > +}
> > +#else
>
> Umm? I was expecting the "else" side to be something that contained an
> empty stub. What's the problem this variant is trying to solve?
>
> /
>     Leif
>
Will enable PASS_SCT and see if we can live with that, and drop the other variant
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.