Re: [edk2-devel] [PATCH] UsbNetworkPkg: add USB network devices support

["Rebecca Cran" <quic_rcran@quicinc.com>]

On 9/1/22 23:24, RichardHo [何明忠] via groups.io wrote:
> diff --git a/UsbNetworkPkg/Config/UsbNetworkPkgComponentsDxe.inc.dsc b/UsbNetworkPkg/Config/UsbNetworkPkgComponentsDxe.inc.dsc
> new file mode 100644
> index 0000000000..74b5a3c1b3
> --- /dev/null
> +++ b/UsbNetworkPkg/Config/UsbNetworkPkgComponentsDxe.inc.dsc
> @@ -0,0 +1,20 @@
> +## @file
> +# List of Core Components.
> +#
> +# Copyright (c) 2022, American Megatrends International LLC. All rights reserved.<BR>
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +##
> +
> +  UsbNetworkPkg/NetworkCommon/NetworkCommon.inf
> +
> +!if gUsbNetworkPkgTokenSpaceGuid.UsbCdcEcmSupport
> +  UsbNetworkPkg/UsbCdcEcm/UsbCdcEcm.inf
> +!endif
> +
> +!if gUsbNetworkPkgTokenSpaceGuid.UsbCdcNcmSupport
> +  UsbNetworkPkg/UsbCdcNcm/UsbCdcNcm.inf
> +!endif
> +
> +!if gUsbNetworkPkgTokenSpaceGuid.UsbRndisSupport
> +  UsbNetworkPkg/UsbRndis/UsbRndis.inf
> +!endif

Since some of the functions have the same name and aren't static, should 
there be code to avoid allowing more than one protocol to be enabled at 
the same time?


> diff --git a/UsbNetworkPkg/Include/Protocol/UsbEthernetProtocol.h b/UsbNetworkPkg/Include/Protocol/UsbEthernetProtocol.h
> new file mode 100644
> index 0000000000..1c1a450920
> --- /dev/null
> +++ b/UsbNetworkPkg/Include/Protocol/UsbEthernetProtocol.h

> +// Request Type Codes for USB Ethernet
> +#define USB_ETHERNET_GET_REQ_TYPE  0xA1
> +#define USB_ETHRTNET_SET_REQ_TYPE  0x21

Typo.

> +// The USB_ETHERNET_PROTOCOL provides some basic USB Ethernet device relevant
> +// descriptor and specific requests.
> +struct _USB_ETHERNET_PROTOCOL {
> +  USB_ETHERNET_UNDI                                   UsbEthUndi;
> +  // for calling the UNDI child functions
> +  USB_ETHERNET_INITIALIZE                             UsbEthInitialize;
> +  USB_ETHERNET_STATISTICS                             UsbEthStatistics;
> +  USB_ETHERNET_RECEIVE                                UsbEthReceive;
> +  USB_ETHERNET_TRANSMIT                               UsbEthTransmit;
> +  USB_ETHERNET_INTERRUPT                              UsbEthInterrupt;
> +  USB_GET_ETH_MAC_ADDRESS                             UsbEthMacAddress;
> +  USB_ETH_MAX_BULK_SIZE                               UsbEthMaxBulkSize;
> +  USB_HEADER_FUNCTIONAL_DESCRIPTOR                    UsbHeaderFunDescriptor;
> +  USB_UNION_FUNCTIONAL_DESCRIPTOR                     UsbUnionFunDescriptor;
> +  USB_ETHERNET_FUNCTIONAL_DESCRIPTOR                  UsbEthFunDescriptor;
> +  USB_ETHERNET_SET_ETH_MULTICAST_FILTERS              SetUsbEthMcastFilter;
> +  USB_ETHERNET_SET_ETH_POWER_MANAGE_PATTERN_FILTER    SetUsbEthPowerPatternFilter;
> +  USB_ETHERNET_GET_ETH_POWER_MANAGE_PATTERN_FILTER    GetUsbEthPoewrPatternFilter;

Typo.

> diff --git a/UsbNetworkPkg/NetworkCommon/PxeFunction.c b/UsbNetworkPkg/NetworkCommon/PxeFunction.c
> new file mode 100644
> index 0000000000..f6505f7018
> --- /dev/null
> +++ b/UsbNetworkPkg/NetworkCommon/PxeFunction.c

> +/**
> +  Set PXE receive filter.
> +
> +  @param[in]  Nic         A pointer to the Network interface controller data.
> +  @param[in]  SetFilter   PXE receive filter
> +  @param[in]  CpbAddr     Command Parameter Block Address
> +  @param[in]  CpbSize     Command Parameter Block Size
> +
> +**/
> +UINT16
> +Setfilter (
> +  IN  NIC_DATA  *Nic,
> +  IN  UINT16    SetFilter,
> +  IN  UINT64    CpbAddr,
> +  IN  UINT32    CpbSize
> +  )

Following naming conventions, this should probably be "SetFilter", with 
an uppercase 'F'?

> +**/
> +UINT16
> +Receive (
> +  IN PXE_CDB       *Cdb,
> +  IN OUT NIC_DATA  *Nic,
> +  IN UINT64        CpbAddr,
> +  IN OUT UINT64    DbAddr
> +  )
> +{
> +  EFI_STATUS       Status;
> +  UINTN            Index;
> +  UINT16           StatCode  = PXE_STATCODE_NO_DATA;
> +  PXE_FRAME_TYPE   FrameType = PXE_FRAME_TYPE_NONE;
> +  PXE_CPB_RECEIVE  *Cpb;
> +  PXE_DB_RECEIVE   *Db;
> +  UINT8            *BulkInData;
> +  UINTN            DataLength = (UINTN)Nic->MaxSegmentSize;
> +  EthernetHeader   *Header;
> +
> +  Cpb = (PXE_CPB_RECEIVE *)(UINTN)CpbAddr;
> +  Db  = (PXE_DB_RECEIVE *)(UINTN)DbAddr;
> +
> +  Status = gBS->AllocatePool (EfiBootServicesData, DataLength, (VOID **)&BulkInData);

Allocating and freeing memory on every poll (i.e. 10ms) doesn't seem 
like a good idea. Could we allocate it during initialize instead?

-- 
Rebecca Cran