Re: [Qemu-devel] [PATCH 0/7] serial: add serial_chr_nonnull()

[Peter Maydell]

On 31 August 2017 at 04:52, Philippe Mathieu-Daudé <address@hidden> 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