| Message ID | 163158520452.224107.7330406153367961192.stgit@localhost |
|---|---|
| State | New |
| Headers | show |
| Series | efi_loader: Set EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT again in efi_net_receive() | expand |
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); > } >
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
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
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); }