[3/3] efi_selftest: Recieve the packets until the receive buffer is empty

Message ID 163178242457.65790.16820317670072849950.stgit@localhost
State Accepted
Commit 28fc87ee3ae8730fc556757cff05480b5cf94381
Headers show
Series
  • efi_selftest: Update SIMPLE_NETWORK_PROTOCOL selftest
Related show

Commit Message

Masami Hiramatsu Sept. 16, 2021, 8:53 a.m.
Repeatedly receive the packets until the receive buffer is empty.
If the buffer is empty, EFI_SIMPLE_NETWORK_PROTOCOL::Receive()
returns EFI_NOT_READY. We don't need to use the wait_for_event()
every time.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>

---
 lib/efi_selftest/efi_selftest_snp.c |   67 +++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 30 deletions(-)

Comments

Heinrich Schuchardt Sept. 16, 2021, 9:29 a.m. | #1
On 9/16/21 10:53 AM, Masami Hiramatsu wrote:
> Repeatedly receive the packets until the receive buffer is empty.

> If the buffer is empty, EFI_SIMPLE_NETWORK_PROTOCOL::Receive()

> returns EFI_NOT_READY. We don't need to use the wait_for_event()

> every time.


Why do you want to make the change?
Was anything not working with the prior version?

Best regards

Heinrich

>

> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>

> ---

>   lib/efi_selftest/efi_selftest_snp.c |   67 +++++++++++++++++++----------------

>   1 file changed, 37 insertions(+), 30 deletions(-)

>

> diff --git a/lib/efi_selftest/efi_selftest_snp.c b/lib/efi_selftest/efi_selftest_snp.c

> index c5366c872c..818cbfcacd 100644

> --- a/lib/efi_selftest/efi_selftest_snp.c

> +++ b/lib/efi_selftest/efi_selftest_snp.c

> @@ -362,39 +362,46 @@ static int execute(void)

>   			continue;

>   		}

>   		/*

> -		 * Receive packet

> +		 * Receive packets until buffer is empty

>   		 */

> -		buffer_size = sizeof(buffer);

> -		ret = net->receive(net, NULL, &buffer_size, &buffer,

> -				   &srcaddr, &destaddr, NULL);

> -		if (ret != EFI_SUCCESS) {

> -			efi_st_error("Failed to receive packet");

> -			return EFI_ST_FAILURE;

> +		for (;;) {

> +			buffer_size = sizeof(buffer);

> +			ret = net->receive(net, NULL, &buffer_size, &buffer,

> +					   &srcaddr, &destaddr, NULL);

> +			if (ret == EFI_NOT_READY) {

> +				/* The received buffer is empty. */

> +				break;

> +			}

> +

> +			if (ret != EFI_SUCCESS) {

> +				efi_st_error("Failed to receive packet");

> +				return EFI_ST_FAILURE;

> +			}

> +			/*

> +			 * Check the packet is meant for this system.

> +			 * Unfortunately QEMU ignores the broadcast flag.

> +			 * So we have to check for broadcasts too.

> +			 */

> +			if (memcmp(&destaddr, &net->mode->current_address, ARP_HLEN) &&

> +			    memcmp(&destaddr, BROADCAST_MAC, ARP_HLEN))

> +				continue;

> +			/*

> +			 * Check this is a DHCP reply

> +			 */

> +			if (buffer.p.eth_hdr.et_protlen != ntohs(PROT_IP) ||

> +			    buffer.p.ip_udp.ip_hl_v != 0x45 ||

> +			    buffer.p.ip_udp.ip_p != IPPROTO_UDP ||

> +			    buffer.p.ip_udp.udp_src != ntohs(67) ||

> +			    buffer.p.ip_udp.udp_dst != ntohs(68) ||

> +			    buffer.p.dhcp_hdr.op != BOOTREPLY)

> +				continue;

> +			/*

> +			 * We successfully received a DHCP reply.

> +			 */

> +			goto received;

>   		}

> -		/*

> -		 * Check the packet is meant for this system.

> -		 * Unfortunately QEMU ignores the broadcast flag.

> -		 * So we have to check for broadcasts too.

> -		 */

> -		if (memcmp(&destaddr, &net->mode->current_address, ARP_HLEN) &&

> -		    memcmp(&destaddr, BROADCAST_MAC, ARP_HLEN))

> -			continue;

> -		/*

> -		 * Check this is a DHCP reply

> -		 */

> -		if (buffer.p.eth_hdr.et_protlen != ntohs(PROT_IP) ||

> -		    buffer.p.ip_udp.ip_hl_v != 0x45 ||

> -		    buffer.p.ip_udp.ip_p != IPPROTO_UDP ||

> -		    buffer.p.ip_udp.udp_src != ntohs(67) ||

> -		    buffer.p.ip_udp.udp_dst != ntohs(68) ||

> -		    buffer.p.dhcp_hdr.op != BOOTREPLY)

> -			continue;

> -		/*

> -		 * We successfully received a DHCP reply.

> -		 */

> -		break;

>   	}

> -

> +received:

>   	/*

>   	 * Write a log message.

>   	 */

>
Masami Hiramatsu Sept. 16, 2021, 10:05 a.m. | #2
Hi Heinrich,

2021年9月16日(木) 18:29 Heinrich Schuchardt <xypron.glpk@gmx.de>:
>

>

>

> On 9/16/21 10:53 AM, Masami Hiramatsu wrote:

> > Repeatedly receive the packets until the receive buffer is empty.

> > If the buffer is empty, EFI_SIMPLE_NETWORK_PROTOCOL::Receive()

> > returns EFI_NOT_READY. We don't need to use the wait_for_event()

> > every time.

>

> Why do you want to make the change?

> Was anything not working with the prior version?


Actually, if [2/3] is merged, our problem will be solved. :-)
So this is optional for me.

This is a kind of optimization. If we run this on the busy network, the
interface can receive more than 2 packets until the wait_for_event()
returns. This will avoid calling wait_for_event() in such case, and
also it can test the net->receive() returns EFI_NOT_READY correctly.

Thank you,


>

> Best regards

>

> Heinrich

>

> >

> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>

> > ---

> >   lib/efi_selftest/efi_selftest_snp.c |   67 +++++++++++++++++++----------------

> >   1 file changed, 37 insertions(+), 30 deletions(-)

> >

> > diff --git a/lib/efi_selftest/efi_selftest_snp.c b/lib/efi_selftest/efi_selftest_snp.c

> > index c5366c872c..818cbfcacd 100644

> > --- a/lib/efi_selftest/efi_selftest_snp.c

> > +++ b/lib/efi_selftest/efi_selftest_snp.c

> > @@ -362,39 +362,46 @@ static int execute(void)

> >                       continue;

> >               }

> >               /*

> > -              * Receive packet

> > +              * Receive packets until buffer is empty

> >                */

> > -             buffer_size = sizeof(buffer);

> > -             ret = net->receive(net, NULL, &buffer_size, &buffer,

> > -                                &srcaddr, &destaddr, NULL);

> > -             if (ret != EFI_SUCCESS) {

> > -                     efi_st_error("Failed to receive packet");

> > -                     return EFI_ST_FAILURE;

> > +             for (;;) {

> > +                     buffer_size = sizeof(buffer);

> > +                     ret = net->receive(net, NULL, &buffer_size, &buffer,

> > +                                        &srcaddr, &destaddr, NULL);

> > +                     if (ret == EFI_NOT_READY) {

> > +                             /* The received buffer is empty. */

> > +                             break;

> > +                     }

> > +

> > +                     if (ret != EFI_SUCCESS) {

> > +                             efi_st_error("Failed to receive packet");

> > +                             return EFI_ST_FAILURE;

> > +                     }

> > +                     /*

> > +                      * Check the packet is meant for this system.

> > +                      * Unfortunately QEMU ignores the broadcast flag.

> > +                      * So we have to check for broadcasts too.

> > +                      */

> > +                     if (memcmp(&destaddr, &net->mode->current_address, ARP_HLEN) &&

> > +                         memcmp(&destaddr, BROADCAST_MAC, ARP_HLEN))

> > +                             continue;

> > +                     /*

> > +                      * Check this is a DHCP reply

> > +                      */

> > +                     if (buffer.p.eth_hdr.et_protlen != ntohs(PROT_IP) ||

> > +                         buffer.p.ip_udp.ip_hl_v != 0x45 ||

> > +                         buffer.p.ip_udp.ip_p != IPPROTO_UDP ||

> > +                         buffer.p.ip_udp.udp_src != ntohs(67) ||

> > +                         buffer.p.ip_udp.udp_dst != ntohs(68) ||

> > +                         buffer.p.dhcp_hdr.op != BOOTREPLY)

> > +                             continue;

> > +                     /*

> > +                      * We successfully received a DHCP reply.

> > +                      */

> > +                     goto received;

> >               }

> > -             /*

> > -              * Check the packet is meant for this system.

> > -              * Unfortunately QEMU ignores the broadcast flag.

> > -              * So we have to check for broadcasts too.

> > -              */

> > -             if (memcmp(&destaddr, &net->mode->current_address, ARP_HLEN) &&

> > -                 memcmp(&destaddr, BROADCAST_MAC, ARP_HLEN))

> > -                     continue;

> > -             /*

> > -              * Check this is a DHCP reply

> > -              */

> > -             if (buffer.p.eth_hdr.et_protlen != ntohs(PROT_IP) ||

> > -                 buffer.p.ip_udp.ip_hl_v != 0x45 ||

> > -                 buffer.p.ip_udp.ip_p != IPPROTO_UDP ||

> > -                 buffer.p.ip_udp.udp_src != ntohs(67) ||

> > -                 buffer.p.ip_udp.udp_dst != ntohs(68) ||

> > -                 buffer.p.dhcp_hdr.op != BOOTREPLY)

> > -                     continue;

> > -             /*

> > -              * We successfully received a DHCP reply.

> > -              */

> > -             break;

> >       }

> > -

> > +received:

> >       /*

> >        * Write a log message.

> >        */

> >




--
Masami Hiramatsu

Patch

diff --git a/lib/efi_selftest/efi_selftest_snp.c b/lib/efi_selftest/efi_selftest_snp.c
index c5366c872c..818cbfcacd 100644
--- a/lib/efi_selftest/efi_selftest_snp.c
+++ b/lib/efi_selftest/efi_selftest_snp.c
@@ -362,39 +362,46 @@  static int execute(void)
 			continue;
 		}
 		/*
-		 * Receive packet
+		 * Receive packets until buffer is empty
 		 */
-		buffer_size = sizeof(buffer);
-		ret = net->receive(net, NULL, &buffer_size, &buffer,
-				   &srcaddr, &destaddr, NULL);
-		if (ret != EFI_SUCCESS) {
-			efi_st_error("Failed to receive packet");
-			return EFI_ST_FAILURE;
+		for (;;) {
+			buffer_size = sizeof(buffer);
+			ret = net->receive(net, NULL, &buffer_size, &buffer,
+					   &srcaddr, &destaddr, NULL);
+			if (ret == EFI_NOT_READY) {
+				/* The received buffer is empty. */
+				break;
+			}
+
+			if (ret != EFI_SUCCESS) {
+				efi_st_error("Failed to receive packet");
+				return EFI_ST_FAILURE;
+			}
+			/*
+			 * Check the packet is meant for this system.
+			 * Unfortunately QEMU ignores the broadcast flag.
+			 * So we have to check for broadcasts too.
+			 */
+			if (memcmp(&destaddr, &net->mode->current_address, ARP_HLEN) &&
+			    memcmp(&destaddr, BROADCAST_MAC, ARP_HLEN))
+				continue;
+			/*
+			 * Check this is a DHCP reply
+			 */
+			if (buffer.p.eth_hdr.et_protlen != ntohs(PROT_IP) ||
+			    buffer.p.ip_udp.ip_hl_v != 0x45 ||
+			    buffer.p.ip_udp.ip_p != IPPROTO_UDP ||
+			    buffer.p.ip_udp.udp_src != ntohs(67) ||
+			    buffer.p.ip_udp.udp_dst != ntohs(68) ||
+			    buffer.p.dhcp_hdr.op != BOOTREPLY)
+				continue;
+			/*
+			 * We successfully received a DHCP reply.
+			 */
+			goto received;
 		}
-		/*
-		 * Check the packet is meant for this system.
-		 * Unfortunately QEMU ignores the broadcast flag.
-		 * So we have to check for broadcasts too.
-		 */
-		if (memcmp(&destaddr, &net->mode->current_address, ARP_HLEN) &&
-		    memcmp(&destaddr, BROADCAST_MAC, ARP_HLEN))
-			continue;
-		/*
-		 * Check this is a DHCP reply
-		 */
-		if (buffer.p.eth_hdr.et_protlen != ntohs(PROT_IP) ||
-		    buffer.p.ip_udp.ip_hl_v != 0x45 ||
-		    buffer.p.ip_udp.ip_p != IPPROTO_UDP ||
-		    buffer.p.ip_udp.udp_src != ntohs(67) ||
-		    buffer.p.ip_udp.udp_dst != ntohs(68) ||
-		    buffer.p.dhcp_hdr.op != BOOTREPLY)
-			continue;
-		/*
-		 * We successfully received a DHCP reply.
-		 */
-		break;
 	}
-
+received:
 	/*
 	 * Write a log message.
 	 */