[{"id":1763423,"web_url":"http://patchwork.ozlabs.org/comment/1763423/","msgid":"","list_archive_url":null,"date":"2017-09-05T14:56:16","subject":"Re: [Qemu-devel] [PATCH 0/7] serial: add serial_chr_nonnull()","submitter":{"id":5111,"url":"http://patchwork.ozlabs.org/api/people/5111/","name":"Peter Maydell","email":"peter.maydell@linaro.org"},"content":"On 31 August 2017 at 04:52, Philippe Mathieu-Daudé wrote:\n> Hi,\n>\n> This series add the serial_chr_nonnull() which connect to the \"null\" chardev\n> backend if none is provided.\n>\n> Inspired by Peter's suggestion:\n> http://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg05987.html\n> which also refers to this issue:\n> http://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg03546.html\n>\n> Some boards still check serial_hds[x] non null before calling serial_mm_init(),\n> this check could now be removed on the SoC which always have an UART mapped.\n\nI think this is definitely an area we need to sort out,\nso thanks for sending this patchset. I have two major comments\non it:\n\n(1) I don't think we should be using serial_chr_nonnull() in the\nboard and SoC code (your patches 3 and 4 here). I think the design\nprinciple we should be aiming for is:\n * devices which accept a Chardev should cope with being passed\n a NULL pointer, and so their users never need to care\n\nserial_mm_init() is basically a device (though it has never been\nQOMified), so it is correct that it should change to handle NULL.\n\nPatch 4 (malta) : we should just delete the entire for() loop,\nideally, but this runs into awkwardness because serial_hds_isa_init()\nthen won't create uart 0 and 1. Maybe leave it alone for the moment.\n\nIn patch 5, exynos4210_uart_create() is code that uses a device,\nnot code that implements it, so it should be able to simplify to\njust doing chr = serial_hds[channel]; .\n(there is scope for further cleanup here because there are only\n4 callsites for this utility function, which really ought to\njust always take a Chardev* and not support the 'or pass an\ninteger channel number' stuff. I'd leave that for a different\npatchset though.)\n\nPatches 6 and 7 look OK.\n\n\nOn to point (2): is creating a dummy null chardev the right\nway to arrange for devices to handle a NULL chardev pointer?\nMost of the qemu_chr_fe_*() functions have checks for\n\"if we are passed a CharBackend with a NULL Chardev pointer\ndo nothing\", which suggests that the design intent here is\nthat char devices should be able to just work without having\nto special case NULL. If that can generally be done without\nchar device authors having to think too hard then I feel\nlike we should continue with that approach. It's only\nif it looks like that's too error-prone that the \"create\na dummy null chardev\" approach makes sense, I think (and in\nthat case maybe we can drop all those NULL pointer checks\nin the fe functions?)\n\nWhat is going wrong in the serial_mm_init() case that's\ncausing it to fail when the chardev pointer is NULL ?\n\nthanks\n-- PMM","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=linaro.org header.i=@linaro.org\n\theader.b=\"fTtkx2jg\"; dkim-atps=neutral"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xmqZ72pZsz9sNq\n\tfor ;\n\tWed, 6 Sep 2017 00:57:19 +1000 (AEST)","from localhost ([::1]:59466 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dpFHx-0006D1-EC\n\tfor incoming@patchwork.ozlabs.org; Tue, 05 Sep 2017 10:57:17 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:60604)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dpFHU-00069H-4z\n\tfor qemu-devel@nongnu.org; Tue, 05 Sep 2017 10:56:53 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dpFHK-0005Pm-LM\n\tfor qemu-devel@nongnu.org; Tue, 05 Sep 2017 10:56:48 -0400","from mail-wr0-x22f.google.com ([2a00:1450:400c:c0c::22f]:38252)\n\tby eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16)\n\t(Exim 4.71) (envelope-from )\n\tid 1dpFHK-0005PS-Df\n\tfor qemu-devel@nongnu.org; Tue, 05 Sep 2017 10:56:38 -0400","by mail-wr0-x22f.google.com with SMTP id 108so9287381wra.5\n\tfor ; Tue, 05 Sep 2017 07:56:38 -0700 (PDT)","by 10.223.159.68 with HTTP; Tue, 5 Sep 2017 07:56:16 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=Y8MfjsCjqlP0oq3jS6TuLvZOhP1RPLXLKtRmbDEJKV4=;\n\tb=fTtkx2jgC+gmFrI67tMRoX4jhOcLHLs47V9zKOzI0eBSo8gvylxbbwhletQRTRr9Iz\n\tSmga72jEd6oDQcg19NPe1NsfOsaO5p5PqDHNurSOVybjxO/U046uX6KcXK9HI1q7P07I\n\tpKD5sFoAACSMwBIV4gRaSZRF2TMiSgp85ZRKg=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=Y8MfjsCjqlP0oq3jS6TuLvZOhP1RPLXLKtRmbDEJKV4=;\n\tb=DV+1KkQo0nE25lu71FGgmf4+Ppx3NXgWAnYYP6Ob1rrlDvVgwzg/JuMLnI9wJM8ynm\n\taFSF7JAhVkVTaFSyR7WVXlvViyIWI6DSXHp+8wnBi47sj8WpxMPQitTDixp7+F5xAQzQ\n\t/wywu61SY0MuZ1q4TJ9dL2wR8rBhji8t9BZQv0YvnpQACeASDilCKL0IE/bVKm5aWzLg\n\txW/8/pQ0B2MVWGxPE7aE4Nz7gscwgHJcLS0ZtzqJ7eKtu1HwWkP7rBzp4Q1m4C3qcHa2\n\tq/ygYP0rzKLBKd+CFlW+pCWJRGuE+2LVU1z/4oFA4ZSf7knEJAEZKF+8zd54h8TgK2uh\n\tuCCA==","X-Gm-Message-State":"AHPjjUj8ixAGt0U/R38mUKoPKiIPgKUXPKxcDUiIaIzmPKy3X37FnZnK\n\tkKnOhD9QaN2dCa3TL1fZNIZYu9nry7SE","X-Google-Smtp-Source":"ADKCNb70JFVXWZKHfJs6fvyL9ygITZSPjNCD9zvOGKGdMvft7Hsz5rZcoXSursQPvcjtLS40pkJAtWCUPd5TcknQTFI=","X-Received":"by 10.223.163.71 with SMTP id d7mr2597482wrb.161.1504623397155; \n\tTue, 05 Sep 2017 07:56:37 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<20170831035306.29170-1-f4bug@amsat.org>","References":"<20170831035306.29170-1-f4bug@amsat.org>","From":"Peter Maydell ","Date":"Tue, 5 Sep 2017 15:56:16 +0100","Message-ID":"","To":"=?utf-8?q?Philippe_Mathieu-Daud=C3=A9?= ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","X-detected-operating-system":"by eggs.gnu.org: Genre and OS details not\n\trecognized.","X-Received-From":"2a00:1450:400c:c0c::22f","Subject":"Re: [Qemu-devel] [PATCH 0/7] serial: add serial_chr_nonnull()","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Thomas Huth , \"Michael S. Tsirkin\" ,\n\tQEMU Developers , =?utf-8?q?Marc-Andr=C3=A9_Lu?=\n\t=?utf-8?q?reau?= ,\n\tPaolo Bonzini ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]