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