linux-kernel - Re: [PATCH 07/11] Watchdog: DA9052 watchdog support v1

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 14 Jul 2011 12:29:28 +0200
From:	Wim Van Sebroeck <wim@...ana.be>
To:	ashishj3 <ashish.jangam@...tcummins.com>
Cc:	linux-kernel@...r.kernel.org, linux-watchdog@...r.kernel.org,
	Dajun <dajun.chen@...semi.com>, linaro-dev@...ts.linaro.org
Subject: Re: [PATCH 07/11] Watchdog: DA9052 watchdog support v1

Hi Ashish, Dajun,

> > This driver adds support for the watchdog functionality provided by the Dialog
> > Semiconductor DA9052 PMIC chip.
> > 
> > Signed-off-by: David Dajun Chen <dchen@...semi.com>
> > Signed-off-by: Ashish Jangam <ashish.jangam@...tcummins.com>
> > ---
> If there are no comments then can you ACK this patch?

I remain with my comment of Wed 26 Jan 2011:
> The ioctl WDIOC_SETOPTIONS call is not in line with the watchdog API.
> the supported options are:
> #define WDIOS_DISABLECARD       0x0001  /* Turn off the watchdog timer */
> #define WDIOS_ENABLECARD        0x0002  /* Turn on the watchdog timer */
> #define WDIOS_TEMPPANIC         0x0004  /* Kernel panic on temperature trip */
> Your options are:
> #define DA9052_STROBING_FILTER_ENABLE   0x0001
> #define DA9052_STROBING_FILTER_DISABLE  0x0002
> #define DA9052_SET_STROBING_MODE_MANUAL 0x0004
> #define DA9052_SET_STROBING_MODE_AUTO   0x0008
> 
> Please explain what you want to do.

FYI: this is how the code looks now:
+       case WDIOC_SETOPTIONS:
+               if (get_user(new_value, p))
+                       return -EFAULT;
+               if (new_value == DA9052_ENABLE || new_value == DA9052_DISABLE)
+                       da9052_sm_set_strobing_filter(wdt, new_value);
+               else
+                       wdt->pwdt->sm_strobe_mode_flag = new_value;
+               return 0;

Since the code is still not in line with the watchdog API and since I did not get
an answer yet on what you (or Dajun) want to do, there is definitely NO ACK yet.
If there is a good reasoning behind your needs, we might conclude that this is indeed
worthwhile and extend the API in a proper way. But as it is now, it's not correct.

Kind regards,
Wim.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/