* [PATCH] efi_loader: Set EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT again in efi_net_receive()
@ 2021-09-14 2:06 Masami Hiramatsu
2021-09-14 15:45 ` Heinrich Schuchardt
0 siblings, 1 reply; 4+ messages in thread
From: Masami Hiramatsu @ 2021-09-14 2:06 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Kazuhiko Sakamoto, Masami Hiramatsu, Jassi Brar,
Ilias Apalodimas, u-boot
From: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
Since 'this->int_status' is cleared by efi_net_get_status(), if user
does wait_for_event(wait_for_packet) and efi_net_get_status() loop
and there are several received packets on the buffer, the second
efi_net_get_status() does not return EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT
even if the 'wait_for_packet' is ready.
This happens on "efiboot selftest" with noisy network.
(The network device can receive both of other packets and dhcp discover
packet in a short time.)
Set EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT again when we found any
packet still on the receive buffer.
Signed-off-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
lib/efi_loader/efi_net.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
index 69276b275d..9ca1e0c354 100644
--- a/lib/efi_loader/efi_net.c
+++ b/lib/efi_loader/efi_net.c
@@ -640,10 +640,19 @@ static efi_status_t EFIAPI efi_net_receive
*buffer_size = receive_lengths[rx_packet_idx];
rx_packet_idx = (rx_packet_idx + 1) % ETH_PACKETS_BATCH_RECV;
rx_packet_num--;
- if (rx_packet_num)
+ if (rx_packet_num) {
+ /*
+ * Since 'this->int_status' is cleared by efi_net_get_status(),
+ * if user does wait_for_event(wait_for_packet) and
+ * efi_net_get_status() again, EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT
+ * is not set even if 'wait_for_packet' is ready.
+ * Set EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT again here.
+ */
+ this->int_status |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
wait_for_packet->is_signaled = true;
- else
+ } else {
this->int_status &= ~EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
+ }
out:
return EFI_EXIT(ret);
}
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] efi_loader: Set EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT again in efi_net_receive()
2021-09-14 2:06 [PATCH] efi_loader: Set EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT again in efi_net_receive() Masami Hiramatsu
@ 2021-09-14 15:45 ` Heinrich Schuchardt
2021-09-15 1:31 ` Masami Hiramatsu
0 siblings, 1 reply; 4+ messages in thread
From: Heinrich Schuchardt @ 2021-09-14 15:45 UTC (permalink / raw)
To: Masami Hiramatsu; +Cc: Kazuhiko Sakamoto, Jassi Brar, Ilias Apalodimas, u-boot
On 9/14/21 4:06 AM, Masami Hiramatsu wrote:
> From: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
>
> Since 'this->int_status' is cleared by efi_net_get_status(), if user
Is it correct to clear int_status unconditionally in efi_net_get_status()?
Shouldn't two consecutive calls to EFI_SIMPLE_NETWORK.GetStatus() return
the same result?
Best regards
Heinrich
> does wait_for_event(wait_for_packet) and efi_net_get_status() loop
> and there are several received packets on the buffer, the second
> efi_net_get_status() does not return EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT
> even if the 'wait_for_packet' is ready.
>
> This happens on "efiboot selftest" with noisy network.
> (The network device can receive both of other packets and dhcp discover
> packet in a short time.)
>
> Set EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT again when we found any
> packet still on the receive buffer.
>
> Signed-off-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> ---
> lib/efi_loader/efi_net.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
> index 69276b275d..9ca1e0c354 100644
> --- a/lib/efi_loader/efi_net.c
> +++ b/lib/efi_loader/efi_net.c
> @@ -640,10 +640,19 @@ static efi_status_t EFIAPI efi_net_receive
> *buffer_size = receive_lengths[rx_packet_idx];
> rx_packet_idx = (rx_packet_idx + 1) % ETH_PACKETS_BATCH_RECV;
> rx_packet_num--;
> - if (rx_packet_num)
> + if (rx_packet_num) {
> + /*
> + * Since 'this->int_status' is cleared by efi_net_get_status(),
> + * if user does wait_for_event(wait_for_packet) and
> + * efi_net_get_status() again, EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT
> + * is not set even if 'wait_for_packet' is ready.
> + * Set EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT again here.
> + */
> + this->int_status |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
> wait_for_packet->is_signaled = true;
> - else
> + } else {
> this->int_status &= ~EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
> + }
> out:
> return EFI_EXIT(ret);
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] efi_loader: Set EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT again in efi_net_receive()
2021-09-14 15:45 ` Heinrich Schuchardt
@ 2021-09-15 1:31 ` Masami Hiramatsu
2021-09-15 10:15 ` Masami Hiramatsu
0 siblings, 1 reply; 4+ messages in thread
From: Masami Hiramatsu @ 2021-09-15 1:31 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Kazuhiko Sakamoto, Jassi Brar, Ilias Apalodimas, U-Boot Mailing List
Hi Heinrich,
This is obscure in the specification (I can not see any detailed
description about EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT).
If EFI_SIMPLE_NETWORK.GetStatus() returns only the hardware interrupt
state, it is an useless interface, because it doesn't sync to the
status of the packet buffer and wait_for_event() result.
If EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT is set only if the rx
interrupts (this is not the rx interrupt in U-Boot anyway...) is
coming, the efi selftest code must not use the net->get_status() for
checking the packet is received, or call net->receive() until the
packet buffer is empty(EFI_NOT_READY) (and also, it has to ensure the
packet buffer is empty before calling wait_for_event()).
So, I think the correct test code will be something like this;
submit_dhcp_discover()
for (;;) {
wait_for_event(net)
net->get_status() // and check receive interrupt
while (net->receive() != EFI_NOT_READY) {
// check dhcp reply
}
}
Thank you,
2021年9月15日(水) 0:45 Heinrich Schuchardt <xypron.glpk@gmx.de>:
>
> On 9/14/21 4:06 AM, Masami Hiramatsu wrote:
> > From: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
> >
> > Since 'this->int_status' is cleared by efi_net_get_status(), if user
>
> Is it correct to clear int_status unconditionally in efi_net_get_status()?
>
> Shouldn't two consecutive calls to EFI_SIMPLE_NETWORK.GetStatus() return
> the same result?
>
> Best regards
>
> Heinrich
>
> > does wait_for_event(wait_for_packet) and efi_net_get_status() loop
> > and there are several received packets on the buffer, the second
> > efi_net_get_status() does not return EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT
> > even if the 'wait_for_packet' is ready.
> >
> > This happens on "efiboot selftest" with noisy network.
> > (The network device can receive both of other packets and dhcp discover
> > packet in a short time.)
> >
> > Set EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT again when we found any
> > packet still on the receive buffer.
> >
> > Signed-off-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > ---
> > lib/efi_loader/efi_net.c | 13 +++++++++++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
> > index 69276b275d..9ca1e0c354 100644
> > --- a/lib/efi_loader/efi_net.c
> > +++ b/lib/efi_loader/efi_net.c
> > @@ -640,10 +640,19 @@ static efi_status_t EFIAPI efi_net_receive
> > *buffer_size = receive_lengths[rx_packet_idx];
> > rx_packet_idx = (rx_packet_idx + 1) % ETH_PACKETS_BATCH_RECV;
> > rx_packet_num--;
> > - if (rx_packet_num)
> > + if (rx_packet_num) {
> > + /*
> > + * Since 'this->int_status' is cleared by efi_net_get_status(),
> > + * if user does wait_for_event(wait_for_packet) and
> > + * efi_net_get_status() again, EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT
> > + * is not set even if 'wait_for_packet' is ready.
> > + * Set EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT again here.
> > + */
> > + this->int_status |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
> > wait_for_packet->is_signaled = true;
> > - else
> > + } else {
> > this->int_status &= ~EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
> > + }
> > out:
> > return EFI_EXIT(ret);
> > }
> >
>
--
Masami Hiramatsu
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] efi_loader: Set EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT again in efi_net_receive()
2021-09-15 1:31 ` Masami Hiramatsu
@ 2021-09-15 10:15 ` Masami Hiramatsu
0 siblings, 0 replies; 4+ messages in thread
From: Masami Hiramatsu @ 2021-09-15 10:15 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Kazuhiko Sakamoto, Jassi Brar, Ilias Apalodimas, U-Boot Mailing List
Hi Heinrich,
2021年9月15日(水) 10:31 Masami Hiramatsu <masami.hiramatsu@linaro.org>:
>
> Hi Heinrich,
>
> This is obscure in the specification (I can not see any detailed
> description about EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT).
OK, I checked the UEFI specification v2.9 again.
The main purpose of EFI_SIMPLE_NETWORK.GetStatus() seems to update the
EFI_SIMPLE_NETWORK_MODE::MediaPresent, in other words, it checks link
status.
And InterruptStatus of EFI_SIMPLE_NETWORK.GetStatus() means "the
currently active interrupts", not "packet to be received".
Thus, I think it should be tested for checking the link status instead
of checking receive packets in DHCP.
You also can see the EFI_SIMPLE_NETWORK_PROTOCOL::WaitForPacket
ensures that you can receive the packet (The spec says "Event used
with EFI_BOOT_SERVICES.WaitForEvent() to wait for a packet to be
received." )
So, it is enough to use the WaitForPacket in the packet receiving
loop, no need to use GetStatus(). Then, correct test should be
something like this.
----
net->get_status();
if (!net->mode.MediaPresent) {
error(no link up!)
return;
}
submit_dhcp_discover()
for (;;) {
wait_for_event(net)
while (net->receive() != EFI_NOT_READY) {
// check dhcp reply
}
}
----
For your information, as far as I can see, the interrupt bit is
cleared or not depends on the platform driver implementation in EDK2
too.
In many cases (e.g. virtio-net), they do not clear the original bits,
in another case, no interrupt bit supported (IntrruptStatus always be
0).
But I couldn't find the code which really cleared the interrupt bit...
So I think there is a difference between UEFI specification and
reference implementation (EDK2).
Thank you,
>
> Thank you,
>
> 2021年9月15日(水) 0:45 Heinrich Schuchardt <xypron.glpk@gmx.de>:
>
> >
> > On 9/14/21 4:06 AM, Masami Hiramatsu wrote:
> > > From: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
> > >
> > > Since 'this->int_status' is cleared by efi_net_get_status(), if user
> >
> > Is it correct to clear int_status unconditionally in efi_net_get_status()?
> >
> > Shouldn't two consecutive calls to EFI_SIMPLE_NETWORK.GetStatus() return
> > the same result?
> >
> > Best regards
> >
> > Heinrich
> >
> > > does wait_for_event(wait_for_packet) and efi_net_get_status() loop
> > > and there are several received packets on the buffer, the second
> > > efi_net_get_status() does not return EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT
> > > even if the 'wait_for_packet' is ready.
> > >
> > > This happens on "efiboot selftest" with noisy network.
> > > (The network device can receive both of other packets and dhcp discover
> > > packet in a short time.)
> > >
> > > Set EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT again when we found any
> > > packet still on the receive buffer.
> > >
> > > Signed-off-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
> > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > ---
> > > lib/efi_loader/efi_net.c | 13 +++++++++++--
> > > 1 file changed, 11 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
> > > index 69276b275d..9ca1e0c354 100644
> > > --- a/lib/efi_loader/efi_net.c
> > > +++ b/lib/efi_loader/efi_net.c
> > > @@ -640,10 +640,19 @@ static efi_status_t EFIAPI efi_net_receive
> > > *buffer_size = receive_lengths[rx_packet_idx];
> > > rx_packet_idx = (rx_packet_idx + 1) % ETH_PACKETS_BATCH_RECV;
> > > rx_packet_num--;
> > > - if (rx_packet_num)
> > > + if (rx_packet_num) {
> > > + /*
> > > + * Since 'this->int_status' is cleared by efi_net_get_status(),
> > > + * if user does wait_for_event(wait_for_packet) and
> > > + * efi_net_get_status() again, EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT
> > > + * is not set even if 'wait_for_packet' is ready.
> > > + * Set EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT again here.
> > > + */
> > > + this->int_status |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
> > > wait_for_packet->is_signaled = true;
> > > - else
> > > + } else {
> > > this->int_status &= ~EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
> > > + }
> > > out:
> > > return EFI_EXIT(ret);
> > > }
> > >
> >
>
>
> --
> Masami Hiramatsu
--
Masami Hiramatsu
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-09-15 10:15 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-14 2:06 [PATCH] efi_loader: Set EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT again in efi_net_receive() Masami Hiramatsu
2021-09-14 15:45 ` Heinrich Schuchardt
2021-09-15 1:31 ` Masami Hiramatsu
2021-09-15 10:15 ` Masami Hiramatsu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.