On Mon, Jan 22, 2018 at 3:48 AM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 13 January 2018 at 11:34, Jean-Christophe Dubois <j...@tribudubois.net>
> wrote:
>> The actual imx_eth_enable_rx() function is buggy.
>>
>> It updates s->regs[ENET_RDAR] after calling qemu_flush_queued_packets().
>>
>> qemu_flush_queued_packets() is going to call imx_XXX_receive() which itself
>> is going to call imx_eth_enable_rx().
>>
>> By updating s->regs[ENET_RDAR] after calling qemu_flush_queued_packets()
>> we end up updating the register with an outdated value which might
>> lead to disabling the receive function in the i.MX FEC/ENET device.
>>
>> This patch change the place where the register update is done so that the
>> register value stays up to date and the receive function can keep
>> running.
>>
>> Reported-by: Fyleo <fyle...@gmail.com>
>> Tested-by: Fyleo <fyle...@gmail.com>
>> Signed-off-by: Jean-Christophe Dubois <j...@tribudubois.net>
>> ---
>
> Andrey, do you have an opinion on this patch, since you've been
> looking at i.MX code recently?
>
The rationale makes sense to me and patch looks like a good cleanup in
general, so FWIW:
Reviewed-by: Andrey Smirnov <andrew.smir...@gmail.com>
I also gave it a spin against my i.MX7 changes with doing basic things
like ping and scp of 1GB file, so I can give my:
Tested-by: Andrey Smirnov <andrew.smir...@gmail.com>
for that.
Thanks,
Andrey Smirnov
>> Change since v1:
>> 1. Rebase the patch on the updated master branch
>>
>> hw/net/imx_fec.c | 8 ++------
>> 1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
>> index 4fb48f62ba..9506f9b69f 100644
>> --- a/hw/net/imx_fec.c
>> +++ b/hw/net/imx_fec.c
>> @@ -595,19 +595,16 @@ static void imx_eth_do_tx(IMXFECState *s, uint32_t
>> index)
>> static void imx_eth_enable_rx(IMXFECState *s, bool flush)
>> {
>> IMXFECBufDesc bd;
>> - bool rx_ring_full;
>>
>> imx_fec_read_bd(&bd, s->rx_descriptor);
>>
>> - rx_ring_full = !(bd.flags & ENET_BD_E);
>> + s->regs[ENET_RDAR] = (bd.flags & ENET_BD_E) ? ENET_RDAR_RDAR : 0;
>>
>> - if (rx_ring_full) {
>> + if (!s->regs[ENET_RDAR]) {
>> FEC_PRINTF("RX buffer full\n");
>> } else if (flush) {
>> qemu_flush_queued_packets(qemu_get_queue(s->nic));
>> }
>> -
>> - s->regs[ENET_RDAR] = rx_ring_full ? 0 : ENET_RDAR_RDAR;
>> }
>>
>> static void imx_eth_reset(DeviceState *d)
>> @@ -866,7 +863,6 @@ static void imx_eth_write(void *opaque, hwaddr offset,
>> uint64_t value,
>> case ENET_RDAR:
>> if (s->regs[ENET_ECR] & ENET_ECR_ETHEREN) {
>> if (!s->regs[index]) {
>> - s->regs[index] = ENET_RDAR_RDAR;
>> imx_eth_enable_rx(s, true);
>> }
>> } else {
>> --
>> 2.14.1
>
> Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
>
> thanks
> -- PMM
