[efi] Fix receive and transmit completion reporting

Fix the TxBuf value filled in by GetStatus() to report the transmit
buffer address as required by the (now clarified) specification.

Simplify "interrupt" handling in GetStatus() to report only that one
or more packets have been transmitted or received; there is no need to
report one GetStatus() "interrupt" per packet.

Simplify receive handling to dequeue received packets immediately from
the network device into an internal list (thereby avoiding the hacks
previously used to determine when to report new packet arrivals).

Originally-fixed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Michael Brown <mcb30@ipxe.org>

src/include/ipxe/efi/efi_snp.h

@@ -18,6 +18,9 @@
 #include <ipxe/efi/Protocol/HiiDatabase.h>
 #include <ipxe/efi/Protocol/LoadFile.h>
 
+/** SNP transmit completion ring size */
+#define EFI_SNP_NUM_TX 32
+
 /** An SNP device */
 struct efi_snp_device {
 	/** List of SNP devices */
@@ -34,20 +37,16 @@ struct efi_snp_device {
 	EFI_SIMPLE_NETWORK_MODE mode;
 	/** Started flag */
 	int started;
-	/** Outstanding TX packet count (via "interrupt status")
-	 *
-	 * Used in order to generate TX completions.
-	 */
-	unsigned int tx_count_interrupts;
-	/** Outstanding TX packet count (via "recycled tx buffers")
-	 *
-	 * Used in order to generate TX completions.
-	 */
-	unsigned int tx_count_txbufs;
-	/** Outstanding RX packet count (via "interrupt status") */
-	unsigned int rx_count_interrupts;
-	/** Outstanding RX packet count (via WaitForPacket event) */
-	unsigned int rx_count_events;
+	/** Pending interrupt status */
+	unsigned int interrupts;
+	/** Transmit completion ring */
+	VOID *tx[EFI_SNP_NUM_TX];
+	/** Transmit completion ring producer counter */
+	unsigned int tx_prod;
+	/** Transmit completion ring consumer counter */
+	unsigned int tx_cons;
+	/** Receive queue */
+	struct list_head rx;
 	/** The network interface identifier */
 	EFI_NETWORK_INTERFACE_IDENTIFIER_PROTOCOL nii;
 	/** Component name protocol */

src/interface/efi/efi_snp.c

@@ -97,29 +97,44 @@ static void efi_snp_set_mode ( struct efi_snp_device *snpdev ) {
 	mode->MediaPresent = ( netdev_link_ok ( netdev ) ? TRUE : FALSE );
 }
 
+/**
+ * Flush transmit ring and receive queue
+ *
+ * @v snpdev		SNP device
+ */
+static void efi_snp_flush ( struct efi_snp_device *snpdev ) {
+	struct io_buffer *iobuf;
+	struct io_buffer *tmp;
+
+	/* Reset transmit completion ring */
+	snpdev->tx_prod = 0;
+	snpdev->tx_cons = 0;
+
+	/* Discard any queued receive buffers */
+	list_for_each_entry_safe ( iobuf, tmp, &snpdev->rx, list ) {
+		list_del ( &iobuf->list );
+		free_iob ( iobuf );
+	}
+}
+
 /**
  * Poll net device and count received packets
  *
  * @v snpdev		SNP device
  */
 static void efi_snp_poll ( struct efi_snp_device *snpdev ) {
+	EFI_BOOT_SERVICES *bs = efi_systab->BootServices;
 	struct io_buffer *iobuf;
-	unsigned int before = 0;
-	unsigned int after = 0;
-	unsigned int arrived;
 
-	/* We have to report packet arrivals, and this is the easiest
-	 * way to fake it.
-	 */
-	list_for_each_entry ( iobuf, &snpdev->netdev->rx_queue, list )
-		before++;
+	/* Poll network device */
 	netdev_poll ( snpdev->netdev );
-	list_for_each_entry ( iobuf, &snpdev->netdev->rx_queue, list )
-		after++;
-	arrived = ( after - before );
 
-	snpdev->rx_count_interrupts += arrived;
-	snpdev->rx_count_events += arrived;
+	/* Retrieve any received packets */
+	while ( ( iobuf = netdev_rx_dequeue ( snpdev->netdev ) ) ) {
+		list_add_tail ( &iobuf->list, &snpdev->rx );
+		snpdev->interrupts |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
+		bs->SignalEvent ( &snpdev->snp.WaitForPacket );
+	}
 }
 
 /**
@@ -221,6 +236,7 @@ efi_snp_reset ( EFI_SIMPLE_NETWORK_PROTOCOL *snp, BOOLEAN ext_verify ) {
 
 	netdev_close ( snpdev->netdev );
 	efi_snp_set_state ( snpdev );
+	efi_snp_flush ( snpdev );
 
 	if ( ( rc = netdev_open ( snpdev->netdev ) ) != 0 ) {
 		DBGC ( snpdev, "SNPDEV %p could not reopen %s: %s\n",
@@ -251,6 +267,7 @@ efi_snp_shutdown ( EFI_SIMPLE_NETWORK_PROTOCOL *snp ) {
 
 	netdev_close ( snpdev->netdev );
 	efi_snp_set_state ( snpdev );
+	efi_snp_flush ( snpdev );
 
 	return 0;
 }
@@ -446,20 +463,22 @@ efi_snp_nvdata ( EFI_SIMPLE_NETWORK_PROTOCOL *snp, BOOLEAN read,
  *
  * @v snp		SNP interface
  * @v interrupts	Interrupt status, or NULL
- * @v txbufs		Recycled transmit buffer address, or NULL
+ * @v txbuf		Recycled transmit buffer address, or NULL
  * @ret efirc		EFI status code
  */
 static EFI_STATUS EFIAPI
 efi_snp_get_status ( EFI_SIMPLE_NETWORK_PROTOCOL *snp,
-		     UINT32 *interrupts, VOID **txbufs ) {
+		     UINT32 *interrupts, VOID **txbuf ) {
 	struct efi_snp_device *snpdev =
 		container_of ( snp, struct efi_snp_device, snp );
 
 	DBGC2 ( snpdev, "SNPDEV %p GET_STATUS", snpdev );
 
 	/* Fail if net device is currently claimed for use by iPXE */
-	if ( efi_snp_claimed )
+	if ( efi_snp_claimed ) {
+		DBGC2 ( snpdev, "\n" );
 		return EFI_NOT_READY;
+	}
 
 	/* Poll the network device */
 	efi_snp_poll ( snpdev );
@@ -468,47 +487,19 @@ efi_snp_get_status ( EFI_SIMPLE_NETWORK_PROTOCOL *snp,
 	 * to detect TX completions.
 	 */
 	if ( interrupts ) {
-		*interrupts = 0;
-		/* Report TX completions once queue is empty; this
-		 * avoids having to add hooks in the net device layer.
-		 */
-		if ( snpdev->tx_count_interrupts &&
-		     list_empty ( &snpdev->netdev->tx_queue ) ) {
-			*interrupts |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
-			snpdev->tx_count_interrupts--;
-		}
-		/* Report RX */
-		if ( snpdev->rx_count_interrupts ) {
-			*interrupts |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
-			snpdev->rx_count_interrupts--;
-		}
+		*interrupts = snpdev->interrupts;
 		DBGC2 ( snpdev, " INTS:%02x", *interrupts );
+		snpdev->interrupts = 0;
 	}
 
-	/* TX completions.  It would be possible to design a more
-	 * idiotic scheme for this, but it would be a challenge.
-	 * According to the UEFI header file, txbufs will be filled in
-	 * with a list of "recycled transmit buffers" (i.e. completed
-	 * TX buffers).  Observant readers may care to note that
-	 * *txbufs is a void pointer.  Precisely how a list of
-	 * completed transmit buffers is meant to be represented as an
-	 * array of voids is left as an exercise for the reader.
-	 *
-	 * The only users of this interface (MnpDxe/MnpIo.c and
-	 * PxeBcDxe/Bc.c within the EFI dev kit) both just poll until
-	 * seeing a non-NULL result return in txbufs.  This is valid
-	 * provided that they do not ever attempt to transmit more
-	 * than one packet concurrently (and that TX never times out).
-	 */
-	if ( txbufs ) {
-		if ( snpdev->tx_count_txbufs &&
-		     list_empty ( &snpdev->netdev->tx_queue ) ) {
-			*txbufs = "Which idiot designed this API?";
-			snpdev->tx_count_txbufs--;
+	/* TX completions */
+	if ( txbuf ) {
+		if ( snpdev->tx_prod != snpdev->tx_cons ) {
+			*txbuf = snpdev->tx[snpdev->tx_cons++ % EFI_SNP_NUM_TX];
 		} else {
-			*txbufs = NULL;
+			*txbuf = NULL;
 		}
-		DBGC2 ( snpdev, " TX:%s", ( *txbufs ? "some" : "none" ) );
+		DBGC2 ( snpdev, " TX:%p", *txbuf );
 	}
 
 	DBGC2 ( snpdev, "\n" );
@@ -537,6 +528,7 @@ efi_snp_transmit ( EFI_SIMPLE_NETWORK_PROTOCOL *snp,
 	struct ll_protocol *ll_protocol = snpdev->netdev->ll_protocol;
 	struct io_buffer *iobuf;
 	size_t payload_len;
+	unsigned int tx_fill;
 	int rc;
 
 	DBGC2 ( snpdev, "SNPDEV %p TRANSMIT %p+%lx", snpdev, data,
@@ -624,12 +616,27 @@ efi_snp_transmit ( EFI_SIMPLE_NETWORK_PROTOCOL *snp,
 		goto err_tx;
 	}
 
-	/* Record transmission as outstanding */
-	snpdev->tx_count_interrupts++;
-	snpdev->tx_count_txbufs++;
+	/* Record in transmit completion ring.  If we run out of
+	 * space, report the failure even though we have already
+	 * transmitted the packet.
+	 *
+	 * This allows us to report completions only for packets for
+	 * which we had reported successfully initiating transmission,
+	 * while continuing to support clients that never poll for
+	 * transmit completions.
+	 */
+	tx_fill = ( snpdev->tx_prod - snpdev->tx_cons );
+	if ( tx_fill >= EFI_SNP_NUM_TX ) {
+		DBGC ( snpdev, "SNPDEV %p TX completion ring full\n", snpdev );
+		rc = -ENOBUFS;
+		goto err_ring_full;
+	}
+	snpdev->tx[ snpdev->tx_prod++ % EFI_SNP_NUM_TX ] = data;
+	snpdev->interrupts |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
 
 	return 0;
 
+ err_ring_full:
  err_tx:
  err_ll_push:
 	free_iob ( iobuf );
@@ -676,12 +683,13 @@ efi_snp_receive ( EFI_SIMPLE_NETWORK_PROTOCOL *snp,
 	efi_snp_poll ( snpdev );
 
 	/* Dequeue a packet, if one is available */
-	iobuf = netdev_rx_dequeue ( snpdev->netdev );
+	iobuf = list_first_entry ( &snpdev->rx, struct io_buffer, list );
 	if ( ! iobuf ) {
 		DBGC2 ( snpdev, "\n" );
 		rc = -EAGAIN;
 		goto out_no_packet;
 	}
+	list_del ( &iobuf->list );
 	DBGC2 ( snpdev, "+%zx\n", iob_len ( iobuf ) );
 
 	/* Return packet to caller */
@@ -721,9 +729,8 @@ efi_snp_receive ( EFI_SIMPLE_NETWORK_PROTOCOL *snp,
  * @v event		Event
  * @v context		Event context
  */
-static VOID EFIAPI efi_snp_wait_for_packet ( EFI_EVENT event,
+static VOID EFIAPI efi_snp_wait_for_packet ( EFI_EVENT event __unused,
 					     VOID *context ) {
-	EFI_BOOT_SERVICES *bs = efi_systab->BootServices;
 	struct efi_snp_device *snpdev = context;
 
 	DBGCP ( snpdev, "SNPDEV %p WAIT_FOR_PACKET\n", snpdev );
@@ -738,14 +745,6 @@ static VOID EFIAPI efi_snp_wait_for_packet ( EFI_EVENT event,
 
 	/* Poll the network device */
 	efi_snp_poll ( snpdev );
-
-	/* Fire event if packets have been received */
-	if ( snpdev->rx_count_events != 0 ) {
-		DBGC2 ( snpdev, "SNPDEV %p firing WaitForPacket event\n",
-			snpdev );
-		bs->SignalEvent ( event );
-		snpdev->rx_count_events--;
-	}
 }
 
 /** SNP interface */
@@ -922,6 +921,7 @@ static int efi_snp_probe ( struct net_device *netdev ) {
 	}
 	snpdev->netdev = netdev_get ( netdev );
 	snpdev->efidev = efidev;
+	INIT_LIST_HEAD ( &snpdev->rx );
 
 	/* Sanity check */
 	if ( netdev->ll_protocol->ll_addr_len > sizeof ( EFI_MAC_ADDRESS ) ) {