On 31 August 2017 at 04:52, Philippe Mathieu-Daudé <f4...@amsat.org> wrote: > Hi, > > This series add the serial_chr_nonnull() which connect to the "null" chardev > backend if none is provided. > > Inspired by Peter's suggestion: > http://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg05987.html > which also refers to this issue: > http://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg03546.html > > Some boards still check serial_hds[x] non null before calling > serial_mm_init(), > this check could now be removed on the SoC which always have an UART mapped.
I think this is definitely an area we need to sort out, so thanks for sending this patchset. I have two major comments on it: (1) I don't think we should be using serial_chr_nonnull() in the board and SoC code (your patches 3 and 4 here). I think the design principle we should be aiming for is: * devices which accept a Chardev should cope with being passed a NULL pointer, and so their users never need to care serial_mm_init() is basically a device (though it has never been QOMified), so it is correct that it should change to handle NULL. Patch 4 (malta) : we should just delete the entire for() loop, ideally, but this runs into awkwardness because serial_hds_isa_init() then won't create uart 0 and 1. Maybe leave it alone for the moment. In patch 5, exynos4210_uart_create() is code that uses a device, not code that implements it, so it should be able to simplify to just doing chr = serial_hds[channel]; . (there is scope for further cleanup here because there are only 4 callsites for this utility function, which really ought to just always take a Chardev* and not support the 'or pass an integer channel number' stuff. I'd leave that for a different patchset though.) Patches 6 and 7 look OK. On to point (2): is creating a dummy null chardev the right way to arrange for devices to handle a NULL chardev pointer? Most of the qemu_chr_fe_*() functions have checks for "if we are passed a CharBackend with a NULL Chardev pointer do nothing", which suggests that the design intent here is that char devices should be able to just work without having to special case NULL. If that can generally be done without char device authors having to think too hard then I feel like we should continue with that approach. It's only if it looks like that's too error-prone that the "create a dummy null chardev" approach makes sense, I think (and in that case maybe we can drop all those NULL pointer checks in the fe functions?) What is going wrong in the serial_mm_init() case that's causing it to fail when the chardev pointer is NULL ? thanks -- PMM
