linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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
Date: Fri, 14 Aug 2020 08:07:40 -0700	[thread overview]
Message-ID: <20200814150740.GA254327@roeck-us.net> (raw)
In-Reply-To: <20200813125451.19118-1-madhuparnabhowmik10@gmail.com>

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;

  reply	other threads:[~2020-08-14 15:07 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 [this message]
2020-08-16  7:24   ` Madhuparna Bhowmik

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=20200814150740.GA254327@roeck-us.net \
    --to=linux@roeck-us.net \
    --cc=andrianov@ispras.ru \
    --cc=ldv-project@linuxtesting.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=madhuparnabhowmik10@gmail.com \
    --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).