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;
From:   Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
Date:   Sun, 16 Aug 2020 12:54:41 +0530
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     madhuparnabhowmik10@gmail.com, 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: <20200816072441.GA13871@madhuparna-HP-Notebook>
References: <20200813125451.19118-1-madhuparnabhowmik10@gmail.com>
 <20200814150740.GA254327@roeck-us.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200814150740.GA254327@roeck-us.net>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Aug 14, 2020 at 08:07:40AM -0700, Guenter Roeck wrote:
> 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.
>
Sure makes sense. Thank you for reviewing.

Regards,
Madhuparna

> 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;