[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 0/8] dp8393x: fixes and improvements
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH v3 0/8] dp8393x: fixes and improvements |
Date: |
Sun, 11 Jul 2021 12:33:02 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 |
On 7/11/21 4:08 AM, Finn Thain wrote:
> On Sat, 10 Jul 2021, Philippe Mathieu-Daudé wrote:
>
>>
>> The last 2 patches are included for Mark, but I don't plan to merge
>>
>> them without Finn's Ack, and apparently they require more work.
>>
>
>
> I tested the patch series both with and without the last 2 patches. Both
> builds worked fine with my NetBSD/arc, Linux/mipsel and Linux/m68k guests.
>
> Tested-by: Finn Thain <fthain@linux-m68k.org>
Thank you very much :)
> I have no objection to patch 8/8 ("dp8393x: don't force 32-bit register
> access"). I asked Mark to explain why it was a bug fix (since it didn't
> change QEMU behaviour in my tests) but when I looked into it I found that
> he is quite right, the patch does fix a theoretical bug.
OK.
> My only objection to patch 7/8 ("dp8393x: Rewrite dp8393x_get() /
> dp8393x_put()") was that it could be churn.
>
> If I'm right that the big_endian flag should go away, commit b1600ff195
> ("hw/mips/jazz: specify correct endian for dp8393x device") has already
> taken mainline in the wrong direction and amounts to churn.
We might figure out with a BE guest image, the remove the endian flag.
I don't think the patch is worth removing, because it simplifies and
we'll only have to fix the endianess in 2 places, dp8393x_get/put. I
prefer to restrict the address space accesses there.
> I have the same reservations about patch 6/8 ("dp8393x: Store CRC using
> device configured endianess"). Perhaps that should be NOTFORMERGE too
> (even though it too a theoretical bug fix).
OK, dropped.
> Is there a good way to avoid using big_endian for storing the CRC and the
> other DMA operations?
Could be, but I'd rather see this fixed generically in the MemoryRegion
API, not in this particular device model.
> BTW, if you see "sn0: receive buffers exhausted" occasionally logged by
> the NetBSD 9.2 kernel, accompanied by packet loss, it's not a regression
> in QEMU. I first observed it last year when stress testing dp8393x with
> NetBSD 5.1. I believe this to be an old NetBSD sn driver bug because Linux
> is unaffected.
>
- Re: [PATCH v3 4/8] dp8393x: Store CAM registers as 16-bit, (continued)
- [PATCH v3 5/8] dp8393x: Migrate registers as array of uint16, Philippe Mathieu-Daudé, 2021/07/10
- [PATCH v3 6/8] dp8393x: Store CRC using device configured endianess, Philippe Mathieu-Daudé, 2021/07/10
- [NOTFORMERGE PATCH v3 7/8] dp8393x: Rewrite dp8393x_get() / dp8393x_put(), Philippe Mathieu-Daudé, 2021/07/10
- [NOTFORMERGE PATCH v3 8/8] dp8393x: don't force 32-bit register access, Philippe Mathieu-Daudé, 2021/07/10
- Re: [PATCH v3 0/8] dp8393x: fixes and improvements, Finn Thain, 2021/07/10
- Re: [PATCH v3 0/8] dp8393x: fixes and improvements,
Philippe Mathieu-Daudé <=