Re: [PATCH] hw/net: npcm7xx_emc fix missing queue_flush

[Patrick Venture <venture@google.com>]

[-- Attachment #1: Type: text/plain, Size: 3209 bytes --]

On Fri, Dec 3, 2021 at 1:54 PM Patrick Venture <venture@google.com> wrote:

>
>
> 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.
>

I figured why not :) And just made the change and sent out a v2.

>
>> >  static uint64_t npcm7xx_emc_read(void *opaque, hwaddr offset, unsigned
>> size)
>> >  {
>> >      NPCM7xxEMCState *emc = opaque;
>> > @@ -704,6 +697,7 @@ static void npcm7xx_emc_write(void *opaque, hwaddr
>> offset,
>> >          }
>> >          if (value & REG_MCMDR_RXON) {
>> >              emc->rx_active = true;
>> > +            qemu_flush_queued_packets(qemu_get_queue(emc->nic));
>> >          } else {
>> >              emc_halt_rx(emc, 0);
>> >          }
>> > @@ -740,7 +734,7 @@ static void npcm7xx_emc_write(void *opaque, hwaddr
>> offset,
>> >      case REG_RSDR:
>> >          if (emc->regs[REG_MCMDR] & REG_MCMDR_RXON) {
>> >              emc->rx_active = true;
>> > -            emc_try_receive_next_packet(emc);
>> > +            qemu_flush_queued_packets(qemu_get_queue(emc->nic));
>> >          }
>> >          break;
>> >      case REG_MIIDA:
>> >
>>
>>

[-- Attachment #2: Type: text/html, Size: 4588 bytes --]