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

["RichardHo [何明忠]" <richardho@ami.com>]

Hi Rebecca,

Thanks for your response.

#1. The NCM, ECM, and RNDIS feature could enable at the same time.

#2. UsbEthernetProtocol.h, PxeFunction.c the TYPO will be fixed next.

#3 Receive buffer will be allocate during driver initialize.

Thanks,
Richard

-----Original Message-----
From: Rebecca Cran <rebecca@quicinc.com>
Sent: 2022年11月27日 2:56 AM
To: devel@edk2.groups.io; Richard Ho (何明忠) <RichardHo@ami.com>
Cc: Andrew Fish <afish@apple.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Michael D Kinney <michael.d.kinney@intel.com>; Michael Kubacki <michael.kubacki@microsoft.com>; Zhiguang Liu <zhiguang.liu@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Tony Lo (羅金松) <TonyLo@ami.com>
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH] UsbNetworkPkg: add USB network devices support


**CAUTION: The e-mail below is from an external source. Please exercise caution before opening attachments, clicking links, or following guidance.**

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
-The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.