* [PATCH] hw/net: npcm7xx_emc fix missing queue_flush @ 2021-12-03 21:27 Patrick Venture 2021-12-03 21:42 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 4+ messages in thread From: Patrick Venture @ 2021-12-03 21:27 UTC (permalink / raw) To: peter.maydell; +Cc: qemu-arm, qemu-devel, hskinnemoen, kfting, Patrick Venture 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)); - } -} - 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: -- 2.34.1.400.ga245620fadb-goog ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] hw/net: npcm7xx_emc fix missing queue_flush 2021-12-03 21:27 [PATCH] hw/net: npcm7xx_emc fix missing queue_flush Patrick Venture @ 2021-12-03 21:42 ` Philippe Mathieu-Daudé 2021-12-03 21:54 ` Patrick Venture 0 siblings, 1 reply; 4+ messages in thread From: Philippe Mathieu-Daudé @ 2021-12-03 21:42 UTC (permalink / raw) To: Patrick Venture, peter.maydell; +Cc: kfting, qemu-arm, qemu-devel, hskinnemoen 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)); } > 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: > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] hw/net: npcm7xx_emc fix missing queue_flush 2021-12-03 21:42 ` Philippe Mathieu-Daudé @ 2021-12-03 21:54 ` Patrick Venture 2021-12-03 22:10 ` Patrick Venture 0 siblings, 1 reply; 4+ messages in thread From: Patrick Venture @ 2021-12-03 21:54 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: peter.maydell, qemu-arm, qemu-devel, hskinnemoen, KFTING [-- Attachment #1: Type: text/plain, Size: 2961 bytes --] 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. > > > 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: 3913 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] hw/net: npcm7xx_emc fix missing queue_flush 2021-12-03 21:54 ` Patrick Venture @ 2021-12-03 22:10 ` Patrick Venture 0 siblings, 0 replies; 4+ messages in thread From: Patrick Venture @ 2021-12-03 22:10 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: peter.maydell, qemu-arm, qemu-devel, hskinnemoen, KFTING [-- 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 --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-12-03 22:12 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-12-03 21:27 [PATCH] hw/net: npcm7xx_emc fix missing queue_flush Patrick Venture 2021-12-03 21:42 ` Philippe Mathieu-Daudé 2021-12-03 21:54 ` Patrick Venture 2021-12-03 22:10 ` Patrick Venture
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).