Re: [edk2-devel] [PATCH 2/3] UsbNetworkPkg/UsbCdcEcm: Add USB Cdc ECM devices support

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

On 10/3/22 03:27, RichardHo [何明忠] via groups.io wrote:
> diff --git a/UsbNetworkPkg/UsbCdcEcm/UsbCdcEcm.c b/UsbNetworkPkg/UsbCdcEcm/UsbCdcEcm.c
> +  UsbEthDriver->Signature                          = USB_ETHERNET_SIGNATURE;
> +  UsbEthDriver->NumOfInterface                     = Interface.InterfaceNumber;
> +  UsbEthDriver->UsbCdcDataHandle                   = UsbCdcDataHandle;
> +  UsbEthDriver->UsbIo                              = UsbIo;
> +  UsbEthDriver->UsbEth.UsbEthReceive               = UsbEthReceive;
> +  UsbEthDriver->UsbEth.UsbEthTransmit              = UsbEthTransmit;
> +  UsbEthDriver->UsbEth.UsbEthInterrupt             = UsbEthInterrupt;
> +  UsbEthDriver->UsbEth.UsbEthMacAddress            = GetUsbEthMacAddress;
> +  UsbEthDriver->UsbEth.UsbEthMaxBulkSize           = UsbEthBulkSize;
> +  UsbEthDriver->UsbEth.UsbHeaderFunDescriptor      = GetUsbHeaderFunDescriptor;
> +  UsbEthDriver->UsbEth.UsbUnionFunDescriptor       = GetUsbUnionFunDescriptor;
> +  UsbEthDriver->UsbEth.UsbEthFunDescriptor         = GetUsbEthFunDescriptor;
> +  UsbEthDriver->UsbEth.SetUsbEthMcastFilter        = SetUsbEthMcastFilter;
> +  UsbEthDriver->UsbEth.SetUsbEthPowerPatternFilter = SetUsbEthPowerFilter
> +  UsbEthDriver->UsbEth.GetUsbEthPoewrPatternFilter = GetUsbEthPowerFilter;

Typo of "Power".


 > diff --git a/UsbNetworkPkg/UsbCdcEcm/UsbEcmFunction.c 
b/UsbNetworkPkg/UsbCdcEcm/UsbEcmFunction.c
 > new file mode 100644
 > index 0000000000..ae1afc512a
 > --- /dev/null
 > +++ b/UsbNetworkPkg/UsbCdcEcm/UsbEcmFunction.c

> +/**
> +  Get USB Ethernet IO endpoint and USB CDC data IO endpoint.
> +
> +  @param[in]      UsbIo         A pointer to the EFI_USB_IO_PROTOCOL instance.
> +  @param[in, out] UsbEthDriver  A pointer to the USB_ETHERNET_DRIVER instance.

There shouldn't be a space between "in," and "out"

> +/**
> +  This function is used to manage a USB device with the bulk transfer pipe. The endpoint is Bulk in.
> +
> +  @param[in]      Cdb           A pointer to the command descriptor block.
> +  @param[in]      This          A pointer to the EDKII_USB_ETHERNET_PROTOCOL instance.
> +  @param[in, out] Packet        A pointer to the buffer of data that will be transmitted to USB
> +                                device or received from USB device.
> +  @param[in, out] PacketLength  A pointer to the PacketLength.
> +
> +  @retval EFI_SUCCESS           The bulk transfer has been successfully executed.
> +  @retval EFI_DEVICE_ERROR      The transfer failed. The transfer status is returned in status.
> +  @retval EFI_INVALID_PARAMETE  One or more parameters are invalid.

Missing 'R' - should be EFI_INVALID_PARAMETER.

> +  @retval EFI_OUT_OF_RESOURCES  The request could not be submitted due to a lack of resources.
> +  @retval EFI_TIMEOUT           The control transfer fails due to timeout.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +UsbEthReceive (
> +  IN     PXE_CDB                      *Cdb,
> +  IN     EDKII_USB_ETHERNET_PROTOCOL  *This,
> +  IN OUT VOID                         *Packet,
> +  IN OUT UINTN                        *PacketLength
> +  )
> +{
> +  EFI_STATUS           Status;
> +  USB_ETHERNET_DRIVER  *UsbEthDriver;
> +  EFI_USB_IO_PROTOCOL  *UsbIo;
> +  UINT32               TransStatus;
> +
> +  UsbEthDriver = USB_ETHERNET_DEV_FROM_THIS (This);
> +
> +  Status = gBS->HandleProtocol (
> +                  UsbEthDriver->UsbCdcDataHandle,
> +                  &gEfiUsbIoProtocolGuid,
> +                  (VOID **)&UsbIo
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (UsbEthDriver->BulkInEndpoint == 0) {
> +    GetEndpoint (UsbIo, UsbEthDriver);
> +  }
> +
> +  Status = UsbIo->UsbBulkTransfer (
> +                    UsbIo,
> +                    UsbEthDriver->BulkInEndpoint,
> +                    Packet,
> +                    PacketLength,
> +                    USB_ETHERNET_BULK_TIMEOUT,
> +                    &TransStatus
> +                    );

Have you made any changes to the USB network stack? I'm finding this 
slows down the system substantially, to the point where it can often 
look like it's hung. I'm wondering if we maybe need to change the 
timeout type to microseconds?

> +  return Status;
> +}
> +
> +/**
> +  This function is used to manage a USB device with the bulk transfer pipe. The endpoint is Bulk out.
> +
> +  @param[in]      Cdb           A pointer to the command descriptor block.
> +  @param[in]      This          A pointer to the EDKII_USB_ETHERNET_PROTOCOL instance.
> +  @param[in]      Packet        A pointer to the buffer of data that will be transmitted to USB
> +                                device or received from USB device.
> +  @param[in, out] PacketLength  A pointer to the PacketLength.
> +
> +  @retval EFI_SUCCESS           The bulk transfer has been successfully executed.
> +  @retval EFI_DEVICE_ERROR      The transfer failed. The transfer status is returned in status.
> +  @retval EFI_INVALID_PARAMETE  One or more parameters are invalid.

Missing 'R' - should be EFI_INVALID_PARAMETER.

> +/**
> +  This request sets the Ethernet device multicast filters as specified in the
> +  sequential list of 48 bit Ethernet multicast addresses.
> +
> +  @param[in]  This                   A pointer to the EDKII_USB_ETHERNET_PROTOCOL instance.
> +  @param[in]  Value                  Number of filters.
> +  @param[in]  McastAddr              A pointer to the value of the multicast addresses.
> +
> +  @retval EFI_SUCCESS           The request executed successfully.
> +  @retval EFI_TIMEOUT           A timeout occurred executing the request.
> +  @retval EFI_DEVICE_ERROR      The request failed due to a device error.
> +  @retval EFI_INVALID_PARAMETER One of the parameters has an invalid value.
> +  @retval EFI_UNSUPPORTED       Not supported.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +SetUsbEthMcastFilter (
> +  IN EDKII_USB_ETHERNET_PROTOCOL  *This,
> +  IN UINT16                       Value,
> +  IN VOID                         *McastAddr
> +  )
> +{
> +  EFI_STATUS                   Status;
> +  EFI_USB_DEVICE_REQUEST       Request;
> +  UINT32                       TransStatus;
> +  USB_ETHERNET_FUN_DESCRIPTOR  UsbEthFunDescriptor;
> +  USB_ETHERNET_DRIVER          *UsbEthDriver;
> +
> +  UsbEthDriver = USB_ETHERNET_DEV_FROM_THIS (This);
> +
> +  Status = This->UsbEthFunDescriptor (This, &UsbEthFunDescriptor);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if ((UsbEthFunDescriptor.NumberMcFilters << 1) == 0) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  Request.RequestType = USB_ETHRTNET_SET_REQ_TYPE;

Typo - should be "USB_ETHERNET_SET_REQ_TYPE".

> +  Request.Request     = SET_ETH_MULTICAST_FILTERS_REQ;
> +  Request.Value       = Value;
> +  Request.Index       = UsbEthDriver->NumOfInterface;
> +  Request.Length      = Value * 6;
> +
> +  return UsbEthDriver->UsbIo->UsbControlTransfer (
> +                                UsbEthDriver->UsbIo,
> +                                &Request,
> +                                EfiUsbDataOut,
> +                                USB_ETHERNET_TRANSFER_TIMEOUT,
> +                                McastAddr,
> +                                Request.Length,
> +                                &TransStatus
> +                                );
> +}
> +
> +/**
> +  This request sets up the specified Ethernet power management pattern filter as
> +  described in the data structure.
> +
> +  @param[in]  This                  A pointer to the EDKII_USB_ETHERNET_PROTOCOL instance.
> +  @param[in]  Value                 Number of filters.
> +  @param[in]  Length                Size of the power management pattern filter data.
> +  @param[in]  PatternFilter         A pointer to the power management pattern filter structure.
> +
> +  @retval EFI_SUCCESS           The request executed successfully.
> +  @retval EFI_TIMEOUT           A timeout occurred executing the request.
> +  @retval EFI_DEVICE_ERROR      The request failed due to a device error.
> +  @retval EFI_INVALID_PARAMETER One of the parameters has an invalid value.
> +  @retval EFI_UNSUPPORTED       Not supported.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +SetUsbEthPowerFilter (
> +  IN EDKII_USB_ETHERNET_PROTOCOL  *This,
> +  IN UINT16                       Value,
> +  IN UINT16                       Length,
> +  IN VOID                         *PatternFilter
> +  )
> +{
> +  EFI_USB_DEVICE_REQUEST  Request;
> +  UINT32                  TransStatus;
> +  USB_ETHERNET_DRIVER     *UsbEthDriver;
> +
> +  UsbEthDriver = USB_ETHERNET_DEV_FROM_THIS (This);
> +
> +  Request.RequestType = USB_ETHRTNET_SET_REQ_TYPE;

Typo - should be "USB_ETHERNET_SET_REQ_TYPE".

> +/**
> +  This request is used to configure device Ethernet packet filter settings.
> +
> +  @param[in]  This              A pointer to the EDKII_USB_ETHERNET_PROTOCOL instance.
> +  @param[in]  Value             Packet Filter Bitmap.
> +
> +  @retval EFI_SUCCESS           The request executed successfully.
> +  @retval EFI_TIMEOUT           A timeout occurred executing the request.
> +  @retval EFI_DEVICE_ERROR      The request failed due to a device error.
> +  @retval EFI_INVALID_PARAMETER One of the parameters has an invalid value.
> +  @retval EFI_UNSUPPORTED       Not supported.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +SetUsbEthPacketFilter (
> +  IN EDKII_USB_ETHERNET_PROTOCOL  *This,
> +  IN UINT16                       Value
> +  )
> +{
> +  EFI_USB_DEVICE_REQUEST  Request;
> +  UINT32                  TransStatus;
> +  USB_ETHERNET_DRIVER     *UsbEthDriver;
> +  UINT16                  CommandFilter = 0;
> +
> +  UsbEthDriver = USB_ETHERNET_DEV_FROM_THIS (This);
> +
> +  ConvertFilter (Value, &CommandFilter);
> +
> +  Request.RequestType = USB_ETHRTNET_SET_REQ_TYPE;

Typo.

-- 
Rebecca Cran