* [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).