[{"id":1770163,"web_url":"http://patchwork.ozlabs.org/comment/1770163/","msgid":"","list_archive_url":null,"date":"2017-09-18T13:32:46","subject":"Re: [PATCH 4/4] watchdog: aspeed: Move init to arch_initcall","submitter":{"id":21889,"url":"http://patchwork.ozlabs.org/api/people/21889/","name":"Guenter Roeck","email":"linux@roeck-us.net"},"content":"On 09/17/2017 10:49 PM, Andrew Jeffery wrote:\n> Probing at device_initcall time lead to perverse cases where the\n> watchdog was probed after, say, I2C, which then leaves a potentially\n> running watchdog at the mercy of I2C device behaviour and bus\n> conditions.\n> \n> Load the watchdog driver early to ensure that the kernel is patting it\n> well before initialising peripherals.\n> \nBut you are doing a bit more. You are making it bool, and you are enabling it\nby default. Both isn't needed for the intended goal (arch_initcall is converted\nto module_initcall if a driver is built as module). Your change focuses on\nand optimizes the case where the watchdog is already running. That may not\nalways be the case, and there may be systems where the driver is not loaded\non purpose.\n\nGuenter\n\n> Signed-off-by: Andrew Jeffery \n> ---\n> drivers/watchdog/Kconfig | 6 ++----\n> drivers/watchdog/aspeed_wdt.c | 7 ++++++-\n> 2 files changed, 8 insertions(+), 5 deletions(-)\n> \n> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig\n> index a1b92ebe74b6..6103185983ed 100644\n> --- a/drivers/watchdog/Kconfig\n> +++ b/drivers/watchdog/Kconfig\n> @@ -741,8 +741,9 @@ config RENESAS_RZAWDT\n> \t Renesas RZ/A SoCs. These watchdogs can be used to reset a system.\n> \n> config ASPEED_WATCHDOG\n> -\ttristate \"Aspeed BMC watchdog support\"\n> +\tbool \"Aspeed BMC watchdog support\"\n> \tdepends on ARCH_ASPEED || COMPILE_TEST\n> +\tdefault y if ARCH_ASPEED\n> \tselect WATCHDOG_CORE\n> \thelp\n> \t Say Y here to include support for the watchdog timer\n> @@ -750,9 +751,6 @@ config ASPEED_WATCHDOG\n> \n> \t This driver is required to reboot the SoC.\n> \n> -\t To compile this driver as a module, choose M here: the\n> -\t module will be called aspeed_wdt.\n> -\n> config ZX2967_WATCHDOG\n> \ttristate \"ZTE zx2967 SoCs watchdog support\"\n> \tdepends on ARCH_ZX\n> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c\n> index 99bc6fbd8852..679c35abadc4 100644\n> --- a/drivers/watchdog/aspeed_wdt.c\n> +++ b/drivers/watchdog/aspeed_wdt.c\n> @@ -313,7 +313,12 @@ static struct platform_driver aspeed_watchdog_driver = {\n> \t\t.of_match_table = of_match_ptr(aspeed_wdt_of_table),\n> \t},\n> };\n> -module_platform_driver(aspeed_watchdog_driver);\n> +\n> +static int __init aspeed_wdt_init(void)\n> +{\n> +\treturn platform_driver_register(&aspeed_watchdog_driver);\n> +}\n> +arch_initcall(aspeed_wdt_init);\n> \n> MODULE_DESCRIPTION(\"Aspeed Watchdog Driver\");\n> MODULE_LICENSE(\"GPL\");\n>","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xwn582pXVz9s3w\n\tfor ;\n\tMon, 18 Sep 2017 23:33:16 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xwn5814yzzDrq3\n\tfor ;\n\tMon, 18 Sep 2017 23:33:16 +1000 (AEST)","from mail-pg0-x241.google.com (mail-pg0-x241.google.com\n\t[IPv6:2607:f8b0:400e:c05::241])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits)) (No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xwn4f17BbzDsG0;\n\tMon, 18 Sep 2017 23:32:49 +1000 (AEST)","by mail-pg0-x241.google.com with SMTP id u18so133649pgo.1;\n\tMon, 18 Sep 2017 06:32:49 -0700 (PDT)","from server.roeck-us.net\n\t(108-223-40-66.lightspeed.sntcca.sbcglobal.net. [108.223.40.66])\n\tby smtp.gmail.com with ESMTPSA id\n\t78sm14884825pfk.70.2017.09.18.06.32.46\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tMon, 18 Sep 2017 06:32:47 -0700 (PDT)"],"Authentication-Results":["ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"VJyN84Kb\"; dkim-atps=neutral","lists.ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"VJyN84Kb\"; dkim-atps=neutral","ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=gmail.com\n\t(client-ip=2607:f8b0:400e:c05::241; helo=mail-pg0-x241.google.com;\n\tenvelope-from=groeck7@gmail.com; receiver=)","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"VJyN84Kb\"; dkim-atps=neutral"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=sender:subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=swSiAH4srO/AxPB/M40t5f1Ge8RzQVyXz44/3MjlZp4=;\n\tb=VJyN84KbmE0vbJW4en9NKkZ0GB4xR1D/Bdx7WuotM2/EXDRteiTCyBVPJIQgwMFESc\n\t1gk4lE/hsLEqn00pwXysgvZTM4e+4aW1E5dTmpaILZZrSRJ2NRg53wdx7sB1IyBT7AdJ\n\t6GAr87FU5Oc6TO/c9fPqkLcrPw3X0hvQNvCqTsglSYCOkLTSEokm9rbfNbb3crdT/tPj\n\tF+Sdy7x+pXnyYPslF4blLEG1dj1o4QEgV81RrXiLh4mgJeHAWhVgRsqWKhN/oWCKmsy5\n\t7IEfVJu+4mzTgcYkox3XS+rd/5+UsNIxBjMUSvwexeBsYIGvFlVYPf62Z8GeRyvtySSx\n\tIgIQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:sender:subject:to:cc:references:from:message-id\n\t:date:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=swSiAH4srO/AxPB/M40t5f1Ge8RzQVyXz44/3MjlZp4=;\n\tb=GX6de+LaahWbamCOrh6QB+vljM+G8sjhb4T4KkLlb9FpX8tRnJanaZi/barDoqp7go\n\t1EDkOztS2KCxIapc+cLs562qHtxINe4ovdsgkmh2b5OrMXc41TCMW4jMAxfUsNfG60Wg\n\toS07a+ihMce9Iw/jaVULUotBIbA1xyPk3QWt2yht7pcvTk4oeLtC6OB/nLx50/25zor7\n\tVqgwcunUjQBUDEH85pXLceT/mJ2wsh5VM/5ScnwIwMoknTxcxS2t+9/lUljrjL6oJLE6\n\tfpvwrauYlAz1/DZCSyVR3Lk8j+htnbAK8r9Im85FRvqPqgLqK//FESk2QtowaqG6Pj9+\n\tl7gw==","X-Gm-Message-State":"AHPjjUj4k5uFS0kgApPDAnaV2VTbf7rhMjmZCTDmnb6mep1zjlHnfZR8\n\tEHUAzPdVc0VDmw==","X-Google-Smtp-Source":"ADKCNb5VUe7PXfsEmgS1NVL7SfbkZoRI2MVrHRJy4oGD4WHCZzY2QvETsrUmrZw1/BYiDsOEiNiUmg==","X-Received":"by 10.99.130.198 with SMTP id\n\tw189mr31512969pgd.175.1505741568107; \n\tMon, 18 Sep 2017 06:32:48 -0700 (PDT)","Subject":"Re: [PATCH 4/4] watchdog: aspeed: Move init to arch_initcall","To":"Andrew Jeffery , linux-watchdog@vger.kernel.org","References":"<20170918054905.16470-1-andrew@aj.id.au>\n\t<20170918054905.16470-5-andrew@aj.id.au>","From":"Guenter Roeck ","Message-ID":"","Date":"Mon, 18 Sep 2017 06:32:46 -0700","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170918054905.16470-5-andrew@aj.id.au>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"ryan_chen@aspeedtech.com, linux-aspeed@lists.ozlabs.org,\n\topenbmc@lists.ozlabs.org, linux-kernel@vger.kernel.org, wim@iguana.be","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}},{"id":1770619,"web_url":"http://patchwork.ozlabs.org/comment/1770619/","msgid":"<1505788097.4080.22.camel@aj.id.au>","list_archive_url":null,"date":"2017-09-19T02:28:17","subject":"Re: [PATCH 4/4] watchdog: aspeed: Move init to arch_initcall","submitter":{"id":68332,"url":"http://patchwork.ozlabs.org/api/people/68332/","name":"Andrew Jeffery","email":"andrew@aj.id.au"},"content":"On Mon, 2017-09-18 at 06:32 -0700, Guenter Roeck wrote:\n> On 09/17/2017 10:49 PM, Andrew Jeffery wrote:\n> > Probing at device_initcall time lead to perverse cases where the\n> > watchdog was probed after, say, I2C, which then leaves a potentially\n> > running watchdog at the mercy of I2C device behaviour and bus\n> > conditions.\n> > \n> > Load the watchdog driver early to ensure that the kernel is patting it\n> > well before initialising peripherals.\n> > \n> \n> But you are doing a bit more. \n\nTrue.\n\n> You are making it bool, and you are enabling it\n> by default. Both isn't needed for the intended goal (arch_initcall is converted\n> to module_initcall if a driver is built as module).\n\nOkay, my understanding on this small point was fuzzy and was something \nI should have chased up. Thanks for the feedback.\n\n> Your change focuses on\n> and optimizes the case where the watchdog is already running. That may not\n> always be the case, and there may be systems where the driver is not loaded\n> on purpose.\n\nIs this a general comment, or regarding a specific Aspeed BMC-based\nsystem? Currently selecting ARCH_ASPEED will force select WATCHDOG and\nASPEED_WATCHDOG. Thus \"default y if ARCH_ASPEED\" doesn't actually\nchange the resulting configuration, but removes the need for the\nexplicit select in arch/arm/mach-aspeed/Kconfig. I don't know what's\nbetter, but I felt that if we moved from tristate to boolean then\nchanging the default in the watchdog Kconfig was the right way to go.\n\nThe way to reboot the Aspeed SoC is to trip the watchdog. Perhaps\nwanting to build without the driver isn't unreasonable, but it would\nproduce a system that you couldn't reboot. However, that's independent\nof whether or not to build as a module so it can remain a tristate. I\nthink I'll just drop the Kconfig changes and add the exit handler. Sing\nout if I should do anything different.\n\nCheers,\n\nAndrew\n\n> \n> Guenter\n> \n> > > > Signed-off-by: Andrew Jeffery \n> > ---\n> >   drivers/watchdog/Kconfig      | 6 ++----\n> >   drivers/watchdog/aspeed_wdt.c | 7 ++++++-\n> >   2 files changed, 8 insertions(+), 5 deletions(-)\n> > \n> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig\n> > index a1b92ebe74b6..6103185983ed 100644\n> > --- a/drivers/watchdog/Kconfig\n> > +++ b/drivers/watchdog/Kconfig\n> > @@ -741,8 +741,9 @@ config RENESAS_RZAWDT\n> > > >   \t  Renesas RZ/A SoCs. These watchdogs can be used to reset a system.\n> >   \n> >   config ASPEED_WATCHDOG\n> > > > -\ttristate \"Aspeed BMC watchdog support\"\n> > > > +\tbool \"Aspeed BMC watchdog support\"\n> > > >   \tdepends on ARCH_ASPEED || COMPILE_TEST\n> > > > +\tdefault y if ARCH_ASPEED\n> > > >   \tselect WATCHDOG_CORE\n> > > >   \thelp\n> > > >   \t  Say Y here to include support for the watchdog timer\n> > @@ -750,9 +751,6 @@ config ASPEED_WATCHDOG\n> >   \n> > > >   \t  This driver is required to reboot the SoC.\n> >   \n> > > > -\t  To compile this driver as a module, choose M here: the\n> > > > -\t  module will be called aspeed_wdt.\n> > -\n> >   config ZX2967_WATCHDOG\n> > > >   \ttristate \"ZTE zx2967 SoCs watchdog support\"\n> > > >   \tdepends on ARCH_ZX\n> > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c\n> > index 99bc6fbd8852..679c35abadc4 100644\n> > --- a/drivers/watchdog/aspeed_wdt.c\n> > +++ b/drivers/watchdog/aspeed_wdt.c\n> > @@ -313,7 +313,12 @@ static struct platform_driver aspeed_watchdog_driver = {\n> > > >   \t\t.of_match_table = of_match_ptr(aspeed_wdt_of_table),\n> > > >   \t},\n> >   };\n> > -module_platform_driver(aspeed_watchdog_driver);\n> > +\n> > +static int __init aspeed_wdt_init(void)\n> > +{\n> > > > +\treturn platform_driver_register(&aspeed_watchdog_driver);\n> > +}\n> > +arch_initcall(aspeed_wdt_init);\n> >   \n> >   MODULE_DESCRIPTION(\"Aspeed Watchdog Driver\");\n> >   MODULE_LICENSE(\"GPL\");\n> > \n> \n>","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xx6Hy4YV9z9s4s\n\tfor ;\n\tTue, 19 Sep 2017 12:28:46 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xx6Hy3GpXzDqJq\n\tfor ;\n\tTue, 19 Sep 2017 12:28:46 +1000 (AEST)","from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com\n\t[66.111.4.25])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xx6Hh12SrzDqF3;\n\tTue, 19 Sep 2017 12:28:31 +1000 (AEST)","from compute4.internal (compute4.nyi.internal [10.202.2.44])\n\tby mailout.nyi.internal (Postfix) with ESMTP id E09F720A96;\n\tMon, 18 Sep 2017 22:28:26 -0400 (EDT)","from frontend1 ([10.202.2.160])\n\tby compute4.internal (MEProxy); Mon, 18 Sep 2017 22:28:26 -0400","from keelia16 (unknown [203.0.153.9])\n\tby mail.messagingengine.com (Postfix) with ESMTPA id 7983C7F9D2;\n\tMon, 18 Sep 2017 22:28:23 -0400 (EDT)"],"Authentication-Results":["ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"QHDzCG3l\";\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"NWW9vjgu\"; \n\tdkim-atps=neutral","lists.ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"QHDzCG3l\";\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"NWW9vjgu\"; \n\tdkim-atps=neutral","ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=aj.id.au\n\t(client-ip=66.111.4.25; helo=out1-smtp.messagingengine.com;\n\tenvelope-from=andrew@aj.id.au; receiver=)","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"QHDzCG3l\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com\n\theader.b=\"NWW9vjgu\"; dkim-atps=neutral"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=aj.id.au; h=cc\n\t:content-type:date:from:in-reply-to:message-id:mime-version\n\t:references:subject:to:x-me-sender:x-me-sender:x-sasl-enc\n\t:x-sasl-enc; s=fm1; bh=PwzwDUjSVcHzSdHUUVpSRiAA5AusBr3XKlSjS+vHB\n\tjE=; b=QHDzCG3lDtbKp9hP1exQX2yFUX6SklMqPvE6S7IIKTuFleMj4HAHrasnL\n\tjllsyun3G6p6/O4wD21bO4mMyHe0Y0cX5b9MkXb7Tq40pG7LlLq/JrFnLu1O88Gp\n\tIgl5TThCxZM935iu13l1IdDSAzgjTWxgswwPzQemTDW+/oR3Eb7z8w9BkG5WKpnC\n\tSm50TcF1h0m4h/N2W8hcvVpytDXWFQqZAQvjqZD7Y5N5whVWuVFpVMkP4+/rigkq\n\tkOwTj2hgWqW39u3cv8fp0P+6Zd4QcSjZhxKvqkawjWnD0356D+ABy700MsGI0ouZ\n\taaUHG/yS5bVl4RNLOj+LGsBDu6BMg==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=\n\tmessagingengine.com; h=cc:content-type:date:from:in-reply-to\n\t:message-id:mime-version:references:subject:to:x-me-sender\n\t:x-me-sender:x-sasl-enc:x-sasl-enc; s=fm1; bh=PwzwDUjSVcHzSdHUUV\n\tpSRiAA5AusBr3XKlSjS+vHBjE=; b=NWW9vjguC6l/Rw1GrSniRTYrluFk2rU+Nk\n\tuKERgasiWkaeXYL6tLwVgFX/z7Fab1+JnV0t2adMw4dVwX5ImT0ef8oazBku4n8Q\n\tIQ4oWu/3tVBA9o5AWriXOiWGYtDif5PBRZxQxgmJkoRD5QTtvcAsjRWNMzlIk8j6\n\tOGwDnFJfN9KbgK7yZYW5LnTolA37ANAnv3dmKHOuHkXZikk4sflo6NPpCK5sV8CX\n\tWlTgO5qJTPQJO4dRIiYVgA6ijhlTNKlH3Wq2aJSSbB5/EahxOBWM2bIbl+9x3eKZ\n\t+F8si8o4hEBmom8S1wurYgy/HM9jxz+Oenb3A+ubYhGcxcMSG5bA=="],"X-ME-Sender":"","X-Sasl-enc":"1tjoVil0IQSTQmEdQmw7+W+qg89Iu1X5h7shPoMqngdd 1505788105","Message-ID":"<1505788097.4080.22.camel@aj.id.au>","Subject":"Re: [PATCH 4/4] watchdog: aspeed: Move init to arch_initcall","From":"Andrew Jeffery ","To":"Guenter Roeck , linux-watchdog@vger.kernel.org","Date":"Tue, 19 Sep 2017 11:58:17 +0930","In-Reply-To":"","References":"<20170918054905.16470-1-andrew@aj.id.au>\n\t<20170918054905.16470-5-andrew@aj.id.au>\n\t","Content-Type":"multipart/signed; micalg=\"pgp-sha512\";\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"=-9nFddokobLpOtoUJGtV2\"","X-Mailer":"Evolution 3.22.6-1ubuntu1 ","Mime-Version":"1.0","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"ryan_chen@aspeedtech.com, linux-aspeed@lists.ozlabs.org,\n\topenbmc@lists.ozlabs.org, linux-kernel@vger.kernel.org, wim@iguana.be","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}}]