Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 14 Aug 2020 08:07:40 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     madhuparnabhowmik10@gmail.com
Cc:     wim@linux-watchdog.org, linux-watchdog@vger.kernel.org,
        linux-kernel@vger.kernel.org, andrianov@ispras.ru,
        ldv-project@linuxtesting.org
Subject: Re: [PATCH] drivers: watchdog: pc87413_wdt: Fix Race condition bug
Message-ID: <20200814150740.GA254327@roeck-us.net>
References: <20200813125451.19118-1-madhuparnabhowmik10@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200813125451.19118-1-madhuparnabhowmik10@gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Aug 13, 2020 at 06:24:51PM +0530, madhuparnabhowmik10@gmail.com wrote:
> From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> 
> After misc_register the open() callback can be called.
> However the base address (swc_base_addr) is set after misc_register()
> in init.
> As a result, if open callback is called before pc87413_get_swc_base_addr()
> then in the following call chain: pc87413_open() -> pc87413_refresh() ->
> pc87413_swc_bank3() : The value of swc_base_addr will be -1.
> Therefore, do misc_register() after pc87413_get_swc_base_addr().
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>

Another candidate for removal. The entire driver is at the very least
questionable: It hard enables the watchdog after registering it, making it
mandatory to open it within one minute or the system will reboot. Also,
the driver doesn't even check if the hardware even exists; it just assumes
that it does. And then it reconfigures that potentially non-existing
hardware to use a specific chip pin as wdt output, as if that would be
useful if that pin is not wired up. Worst case that driver may actually
break something if it is loaded on an arbitrary system.

I really don't see the point of trying to patch it up unless there are users
left who can confirm that it even works on existing hardware, and then I'd
prefer to have it fixed for good and not just patched up.

Thanks,
Guenter

> ---
>  drivers/watchdog/pc87413_wdt.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/pc87413_wdt.c b/drivers/watchdog/pc87413_wdt.c
> index 73fbfc99083b..ad8b8af2bdc0 100644
> --- a/drivers/watchdog/pc87413_wdt.c
> +++ b/drivers/watchdog/pc87413_wdt.c
> @@ -512,6 +512,10 @@ static int __init pc87413_init(void)
>  	if (ret != 0)
>  		pr_err("cannot register reboot notifier (err=%d)\n", ret);
>  
> +	pc87413_select_wdt_out();
> +	pc87413_enable_swc();
> +	pc87413_get_swc_base_addr();
> +
>  	ret = misc_register(&pc87413_miscdev);
>  	if (ret != 0) {
>  		pr_err("cannot register miscdev on minor=%d (err=%d)\n",
> @@ -520,10 +524,6 @@ static int __init pc87413_init(void)
>  	}
>  	pr_info("initialized. timeout=%d min\n", timeout);
>  
> -	pc87413_select_wdt_out();
> -	pc87413_enable_swc();
> -	pc87413_get_swc_base_addr();
> -
>  	if (!request_region(swc_base_addr, 0x20, MODNAME)) {
>  		pr_err("cannot request SWC region at 0x%x\n", swc_base_addr);
>  		ret = -EBUSY;