linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
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
Date: Sun, 16 Aug 2020 12:54:41 +0530	[thread overview]
Message-ID: <20200816072441.GA13871@madhuparna-HP-Notebook> (raw)
In-Reply-To: <20200814150740.GA254327@roeck-us.net>

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;

      reply	other threads:[~2020-08-16  7:24 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-13 12:54 [PATCH] drivers: watchdog: pc87413_wdt: Fix Race condition bug madhuparnabhowmik10
2020-08-14 15:07 ` Guenter Roeck
2020-08-16  7:24   ` Madhuparna Bhowmik [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200816072441.GA13871@madhuparna-HP-Notebook \
    --to=madhuparnabhowmik10@gmail.com \
    --cc=andrianov@ispras.ru \
    --cc=ldv-project@linuxtesting.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=wim@linux-watchdog.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).