Skip to content

Commit 88a5f56

Browse files
committedJul 22, 2015
[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>
1 parent f903dda commit 88a5f56

File tree

2 files changed

+79
-80
lines changed

2 files changed

+79
-80
lines changed
 

‎src/include/ipxe/efi/efi_snp.h

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,9 @@
1818
#include <ipxe/efi/Protocol/HiiDatabase.h>
1919
#include <ipxe/efi/Protocol/LoadFile.h>
2020

21+
/** SNP transmit completion ring size */
22+
#define EFI_SNP_NUM_TX 32
23+
2124
/** An SNP device */
2225
struct efi_snp_device {
2326
/** List of SNP devices */
@@ -34,20 +37,16 @@ struct efi_snp_device {
3437
EFI_SIMPLE_NETWORK_MODE mode;
3538
/** Started flag */
3639
int started;
37-
/** Outstanding TX packet count (via "interrupt status")
38-
*
39-
* Used in order to generate TX completions.
40-
*/
41-
unsigned int tx_count_interrupts;
42-
/** Outstanding TX packet count (via "recycled tx buffers")
43-
*
44-
* Used in order to generate TX completions.
45-
*/
46-
unsigned int tx_count_txbufs;
47-
/** Outstanding RX packet count (via "interrupt status") */
48-
unsigned int rx_count_interrupts;
49-
/** Outstanding RX packet count (via WaitForPacket event) */
50-
unsigned int rx_count_events;
40+
/** Pending interrupt status */
41+
unsigned int interrupts;
42+
/** Transmit completion ring */
43+
VOID *tx[EFI_SNP_NUM_TX];
44+
/** Transmit completion ring producer counter */
45+
unsigned int tx_prod;
46+
/** Transmit completion ring consumer counter */
47+
unsigned int tx_cons;
48+
/** Receive queue */
49+
struct list_head rx;
5150
/** The network interface identifier */
5251
EFI_NETWORK_INTERFACE_IDENTIFIER_PROTOCOL nii;
5352
/** Component name protocol */

‎src/interface/efi/efi_snp.c

Lines changed: 66 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -97,29 +97,44 @@ static void efi_snp_set_mode ( struct efi_snp_device *snpdev ) {
9797
mode->MediaPresent = ( netdev_link_ok ( netdev ) ? TRUE : FALSE );
9898
}
9999

100+
/**
101+
* Flush transmit ring and receive queue
102+
*
103+
* @v snpdev SNP device
104+
*/
105+
static void efi_snp_flush ( struct efi_snp_device *snpdev ) {
106+
struct io_buffer *iobuf;
107+
struct io_buffer *tmp;
108+
109+
/* Reset transmit completion ring */
110+
snpdev->tx_prod = 0;
111+
snpdev->tx_cons = 0;
112+
113+
/* Discard any queued receive buffers */
114+
list_for_each_entry_safe ( iobuf, tmp, &snpdev->rx, list ) {
115+
list_del ( &iobuf->list );
116+
free_iob ( iobuf );
117+
}
118+
}
119+
100120
/**
101121
* Poll net device and count received packets
102122
*
103123
* @v snpdev SNP device
104124
*/
105125
static void efi_snp_poll ( struct efi_snp_device *snpdev ) {
126+
EFI_BOOT_SERVICES *bs = efi_systab->BootServices;
106127
struct io_buffer *iobuf;
107-
unsigned int before = 0;
108-
unsigned int after = 0;
109-
unsigned int arrived;
110128

111-
/* We have to report packet arrivals, and this is the easiest
112-
* way to fake it.
113-
*/
114-
list_for_each_entry ( iobuf, &snpdev->netdev->rx_queue, list )
115-
before++;
129+
/* Poll network device */
116130
netdev_poll ( snpdev->netdev );
117-
list_for_each_entry ( iobuf, &snpdev->netdev->rx_queue, list )
118-
after++;
119-
arrived = ( after - before );
120131

121-
snpdev->rx_count_interrupts += arrived;
122-
snpdev->rx_count_events += arrived;
132+
/* Retrieve any received packets */
133+
while ( ( iobuf = netdev_rx_dequeue ( snpdev->netdev ) ) ) {
134+
list_add_tail ( &iobuf->list, &snpdev->rx );
135+
snpdev->interrupts |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
136+
bs->SignalEvent ( &snpdev->snp.WaitForPacket );
137+
}
123138
}
124139

125140
/**
@@ -221,6 +236,7 @@ efi_snp_reset ( EFI_SIMPLE_NETWORK_PROTOCOL *snp, BOOLEAN ext_verify ) {
221236

222237
netdev_close ( snpdev->netdev );
223238
efi_snp_set_state ( snpdev );
239+
efi_snp_flush ( snpdev );
224240

225241
if ( ( rc = netdev_open ( snpdev->netdev ) ) != 0 ) {
226242
DBGC ( snpdev, "SNPDEV %p could not reopen %s: %s\n",
@@ -251,6 +267,7 @@ efi_snp_shutdown ( EFI_SIMPLE_NETWORK_PROTOCOL *snp ) {
251267

252268
netdev_close ( snpdev->netdev );
253269
efi_snp_set_state ( snpdev );
270+
efi_snp_flush ( snpdev );
254271

255272
return 0;
256273
}
@@ -446,20 +463,22 @@ efi_snp_nvdata ( EFI_SIMPLE_NETWORK_PROTOCOL *snp, BOOLEAN read,
446463
*
447464
* @v snp SNP interface
448465
* @v interrupts Interrupt status, or NULL
449-
* @v txbufs Recycled transmit buffer address, or NULL
466+
* @v txbuf Recycled transmit buffer address, or NULL
450467
* @ret efirc EFI status code
451468
*/
452469
static EFI_STATUS EFIAPI
453470
efi_snp_get_status ( EFI_SIMPLE_NETWORK_PROTOCOL *snp,
454-
UINT32 *interrupts, VOID **txbufs ) {
471+
UINT32 *interrupts, VOID **txbuf ) {
455472
struct efi_snp_device *snpdev =
456473
container_of ( snp, struct efi_snp_device, snp );
457474

458475
DBGC2 ( snpdev, "SNPDEV %p GET_STATUS", snpdev );
459476

460477
/* Fail if net device is currently claimed for use by iPXE */
461-
if ( efi_snp_claimed )
478+
if ( efi_snp_claimed ) {
479+
DBGC2 ( snpdev, "\n" );
462480
return EFI_NOT_READY;
481+
}
463482

464483
/* Poll the network device */
465484
efi_snp_poll ( snpdev );
@@ -468,47 +487,19 @@ efi_snp_get_status ( EFI_SIMPLE_NETWORK_PROTOCOL *snp,
468487
* to detect TX completions.
469488
*/
470489
if ( interrupts ) {
471-
*interrupts = 0;
472-
/* Report TX completions once queue is empty; this
473-
* avoids having to add hooks in the net device layer.
474-
*/
475-
if ( snpdev->tx_count_interrupts &&
476-
list_empty ( &snpdev->netdev->tx_queue ) ) {
477-
*interrupts |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
478-
snpdev->tx_count_interrupts--;
479-
}
480-
/* Report RX */
481-
if ( snpdev->rx_count_interrupts ) {
482-
*interrupts |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
483-
snpdev->rx_count_interrupts--;
484-
}
490+
*interrupts = snpdev->interrupts;
485491
DBGC2 ( snpdev, " INTS:%02x", *interrupts );
492+
snpdev->interrupts = 0;
486493
}
487494

488-
/* TX completions. It would be possible to design a more
489-
* idiotic scheme for this, but it would be a challenge.
490-
* According to the UEFI header file, txbufs will be filled in
491-
* with a list of "recycled transmit buffers" (i.e. completed
492-
* TX buffers). Observant readers may care to note that
493-
* *txbufs is a void pointer. Precisely how a list of
494-
* completed transmit buffers is meant to be represented as an
495-
* array of voids is left as an exercise for the reader.
496-
*
497-
* The only users of this interface (MnpDxe/MnpIo.c and
498-
* PxeBcDxe/Bc.c within the EFI dev kit) both just poll until
499-
* seeing a non-NULL result return in txbufs. This is valid
500-
* provided that they do not ever attempt to transmit more
501-
* than one packet concurrently (and that TX never times out).
502-
*/
503-
if ( txbufs ) {
504-
if ( snpdev->tx_count_txbufs &&
505-
list_empty ( &snpdev->netdev->tx_queue ) ) {
506-
*txbufs = "Which idiot designed this API?";
507-
snpdev->tx_count_txbufs--;
495+
/* TX completions */
496+
if ( txbuf ) {
497+
if ( snpdev->tx_prod != snpdev->tx_cons ) {
498+
*txbuf = snpdev->tx[snpdev->tx_cons++ % EFI_SNP_NUM_TX];
508499
} else {
509-
*txbufs = NULL;
500+
*txbuf = NULL;
510501
}
511-
DBGC2 ( snpdev, " TX:%s", ( *txbufs ? "some" : "none" ) );
502+
DBGC2 ( snpdev, " TX:%p", *txbuf );
512503
}
513504

514505
DBGC2 ( snpdev, "\n" );
@@ -537,6 +528,7 @@ efi_snp_transmit ( EFI_SIMPLE_NETWORK_PROTOCOL *snp,
537528
struct ll_protocol *ll_protocol = snpdev->netdev->ll_protocol;
538529
struct io_buffer *iobuf;
539530
size_t payload_len;
531+
unsigned int tx_fill;
540532
int rc;
541533

542534
DBGC2 ( snpdev, "SNPDEV %p TRANSMIT %p+%lx", snpdev, data,
@@ -624,12 +616,27 @@ efi_snp_transmit ( EFI_SIMPLE_NETWORK_PROTOCOL *snp,
624616
goto err_tx;
625617
}
626618

627-
/* Record transmission as outstanding */
628-
snpdev->tx_count_interrupts++;
629-
snpdev->tx_count_txbufs++;
619+
/* Record in transmit completion ring. If we run out of
620+
* space, report the failure even though we have already
621+
* transmitted the packet.
622+
*
623+
* This allows us to report completions only for packets for
624+
* which we had reported successfully initiating transmission,
625+
* while continuing to support clients that never poll for
626+
* transmit completions.
627+
*/
628+
tx_fill = ( snpdev->tx_prod - snpdev->tx_cons );
629+
if ( tx_fill >= EFI_SNP_NUM_TX ) {
630+
DBGC ( snpdev, "SNPDEV %p TX completion ring full\n", snpdev );
631+
rc = -ENOBUFS;
632+
goto err_ring_full;
633+
}
634+
snpdev->tx[ snpdev->tx_prod++ % EFI_SNP_NUM_TX ] = data;
635+
snpdev->interrupts |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
630636

631637
return 0;
632638

639+
err_ring_full:
633640
err_tx:
634641
err_ll_push:
635642
free_iob ( iobuf );
@@ -676,12 +683,13 @@ efi_snp_receive ( EFI_SIMPLE_NETWORK_PROTOCOL *snp,
676683
efi_snp_poll ( snpdev );
677684

678685
/* Dequeue a packet, if one is available */
679-
iobuf = netdev_rx_dequeue ( snpdev->netdev );
686+
iobuf = list_first_entry ( &snpdev->rx, struct io_buffer, list );
680687
if ( ! iobuf ) {
681688
DBGC2 ( snpdev, "\n" );
682689
rc = -EAGAIN;
683690
goto out_no_packet;
684691
}
692+
list_del ( &iobuf->list );
685693
DBGC2 ( snpdev, "+%zx\n", iob_len ( iobuf ) );
686694

687695
/* Return packet to caller */
@@ -721,9 +729,8 @@ efi_snp_receive ( EFI_SIMPLE_NETWORK_PROTOCOL *snp,
721729
* @v event Event
722730
* @v context Event context
723731
*/
724-
static VOID EFIAPI efi_snp_wait_for_packet ( EFI_EVENT event,
732+
static VOID EFIAPI efi_snp_wait_for_packet ( EFI_EVENT event __unused,
725733
VOID *context ) {
726-
EFI_BOOT_SERVICES *bs = efi_systab->BootServices;
727734
struct efi_snp_device *snpdev = context;
728735

729736
DBGCP ( snpdev, "SNPDEV %p WAIT_FOR_PACKET\n", snpdev );
@@ -738,14 +745,6 @@ static VOID EFIAPI efi_snp_wait_for_packet ( EFI_EVENT event,
738745

739746
/* Poll the network device */
740747
efi_snp_poll ( snpdev );
741-
742-
/* Fire event if packets have been received */
743-
if ( snpdev->rx_count_events != 0 ) {
744-
DBGC2 ( snpdev, "SNPDEV %p firing WaitForPacket event\n",
745-
snpdev );
746-
bs->SignalEvent ( event );
747-
snpdev->rx_count_events--;
748-
}
749748
}
750749

751750
/** SNP interface */
@@ -922,6 +921,7 @@ static int efi_snp_probe ( struct net_device *netdev ) {
922921
}
923922
snpdev->netdev = netdev_get ( netdev );
924923
snpdev->efidev = efidev;
924+
INIT_LIST_HEAD ( &snpdev->rx );
925925

926926
/* Sanity check */
927927
if ( netdev->ll_protocol->ll_addr_len > sizeof ( EFI_MAC_ADDRESS ) ) {

0 commit comments

Comments
 (0)
Please sign in to comment.