On Fri, Dec 3, 2021 at 1:42 PM Philippe Mathieu-Daudé <
f4bug@amsat.org> wrote:
On 12/3/21 22:27, Patrick Venture wrote:
> The rx_active boolean change to true should always trigger a try_read
> call that flushes the queue.
>
> Signed-off-by: Patrick Venture <venture@google.com>
> ---
> hw/net/npcm7xx_emc.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/hw/net/npcm7xx_emc.c b/hw/net/npcm7xx_emc.c
> index 7c892f820f..97522e6388 100644
> --- a/hw/net/npcm7xx_emc.c
> +++ b/hw/net/npcm7xx_emc.c
> @@ -581,13 +581,6 @@ static ssize_t emc_receive(NetClientState *nc, const uint8_t *buf, size_t len1)
> return len;
> }
>
> -static void emc_try_receive_next_packet(NPCM7xxEMCState *emc)
> -{
> - if (emc_can_receive(qemu_get_queue(emc->nic))) {
> - qemu_flush_queued_packets(qemu_get_queue(emc->nic));
> - }
> -}
What about modifying as emc_flush_rx() or emc_receive_and_flush()
helper instead?
static void emc_flush_rx(NPCM7xxEMCState *emc)
{
emc->rx_active = true;
qemu_flush_queued_packets(qemu_get_queue(emc->nic));
}
I'm ok with that idea, although I'm less fond that it _hides_ the rx_active=true. There is an emc_halt_rx that hides rx_active=false, so one could argue it's not an issue. Looking at ftgmac100, it mostly just calls the qemu_flush_queued_packets inline where it needs it. So given the prior art, I'm more inclined to leave this as a two-line pair, versus collapsing it into a method. Mostly because I don't anticipate this call being made from any other places, so it's not a "growing" device. The method originally was emc_try_receive_next_packet, which didn't do anything more than a no-op check and the queue_flush. The new method would move the rx_active setting from the call that deliberately controls it (the register change) into a subordinate method...
Beyond all that, I think it's fine either way. Feel free to push back and I'll make the change.