Re: [PATCH v3] lsilogic mpt fusion: mptctl: Fixed race condition around mptctl_id variable using mutexes
Hi all,
Please note I took the call chains in my secondmost previous message for
the race condition from mtpctl.c in kernel 4.18.20.
Thank you,
Mark
On Tue, 20 Aug 2019, Mark Balançian wrote:
Hello Mister Prakash, Calaby, and Subramani,
I also please request your reply to my previous message before the end of
this Thursday the latest, as I am partaking in an evaluation period from the
organization I am working for with a deadline very close to that time.
Thank you,
Mark
On 2019-08-20 7:46 a.m., Mark Balantzyan wrote:
Hi all,
The race condition in the mptctl driver I'm wishing to have confirmed is
evidenced by the pair of call chains:
compat_mpctl_ioctl -> compat_mpt_command -> mptctl_do_mpt_command which
calls mpt_get_msg_frame(mptctl_id, ioc)
and
__mptctl_ioctl -> mpt_fw_download -> mptctl_do_fw_download which calls
mpt_put_msg_frame(mptctl_id, iocp, mf) and calls
mpt_get_msg_frame(mptctl_id, iocp)
Since ioctl can be called at any time, accessing of mptctl_id occurs
concurrently between threads causing a race.
I realize in past messages I've tried to patch by surrounding all instances
of mptctl_id with mutexes but I'm focusing this time on one clear instance
of the race condition involving the variable mptctl_id, since Julian asks
what the exact race condition is with respect to the case.
Please let me know the confirmation or not confirmation of this race
possibility.
Thank you,
Mark
On Sun, 18 Aug 2019, Julian Calaby wrote:
Hi Mark,
On Thu, Aug 15, 2019 at 8:02 PM Mark Balantzyan
wrote:
Certain functions in the driver, such as mptctl_do_fw_download() and
mptctl_do_mpt_command(), rely on the instance of mptctl_id, which does
the
id-ing. There is race condition possible when these functions operate in
concurrency. Via, mutexes, the functions are mutually signalled to
cooperate.
Changelog v2
Lacked a version number but added properly terminated else condition at
(former) line 2300.
Changelog v3
Fixes "return -EAGAIN" lines which were erroneously tabbed as if to be
guarded
by "if" conditions lying above them.
Signed-off-by: Mark Balantzyan
---
Changelog should be down here after the "---"
drivers/message/fusion/mptctl.c | 43 +
1 file changed, 33 insertions(+), 10 deletions(-)
diff --git a/drivers/message/fusion/mptctl.c
b/drivers/message/fusion/mptctl.c
index 4470630d..3270843c 100644
--- a/drivers/message/fusion/mptctl.c
+++ b/drivers/message/fusion/mptctl.c
@@ -816,12 +816,15 @@ mptctl_do_fw_download(int ioc, char __user *ufwbuf,
size_t fwlen)
/* Valid device. Get a message frame and construct the
FW download message.
*/
+ mutex_lock(&mpctl_mutex);
if ((mf = mpt_get_msg_frame(mptctl_id, iocp)) == NULL)
- return -EAGAIN;
+ mutex_unlock(&mpctl_mutex);
+ return -EAGAIN;
Are you sure this is right?
1. We're now exiting early with -EAGAIN regardless of the result of
mpt_get_msg_frame()
2. If the result of mpt_get_msg_frame() is not NULL, we don't unlock the
mutex
Do you mean something like:
- - - - - -
mutex_lock(&mpctl_mutex);
mf = mpt_get_msg_frame(mptctl_id, iocp);
mutex_unlock(&mpctl_mutex);
if (mf == NULL) {
- - - - - -
@@ -1889,8 +1894,10 @@ mptctl_do_mpt_command (struct mpt_ioctl_command
karg, void __user *mfPtr)
/* Get a free request frame and save the message context.
*/
+ mutex_lock(&mpctl_mutex);
if ((mf = mpt_get_msg_frame(mptctl_id, ioc)) == NULL)
- return -EAGAIN;
+ mutex_unlock(&mpctl_mutex);
+ return -EAGAIN;
Same comments here.
@@ -2563,7 +2576,9 @@ mptctl_hp_hostinfo(unsigned long arg, unsigned int
data_size)
/*
* Gather ISTWI(Industry Standard Two Wire Interface) Data
*/
+ mutex_lock(&mpctl_mutex);
if ((mf = mpt_get_msg_frame(mptctl_id, ioc)) == NULL) {
+ mutex_unlock(&mpctl_mutex);
This line needs to be indented to match the line below, also we don't
unlock the mutex if mpt_get_msg_frame() is not NULL.
@@ -3010,9 +3027,11 @@ static int __init mptctl_init(void)
* Install our handler
*/
++where;
+ mutex_lock(&mpctl_mutex);
mptctl_id = mpt_register(mptctl_reply, MPTCTL_DRIVER,
"mptctl_reply");
if (!mptctl_id || mptctl_id >= MPT_MAX_PROTOCOL_DRIVERS) {
+ mutex_unlock(&mpctl_mutex);
Why not use a local variable and only update the global variable if it's
valid?
printk(KERN_ERR MYNAM ": ERROR: Failed to register with
Fusion MPT base driver\n");
misc_deregister(&mptctl_miscdev);
err = -EBUSY;
@@ -3022,13 +3041,14 @@ static int __init mptctl_init(void)
Re: [PATCH v3] lsilogic mpt fusion: mptctl: Fixed race condition around mptctl_id variable using mutexes
Hi all,
The race condition in the mptctl driver I'm wishing to have
confirmed is evidenced by the pair of call chains:
compat_mpctl_ioctl -> compat_mpt_command -> mptctl_do_mpt_command which
calls mpt_get_msg_frame(mptctl_id, ioc)
and
__mptctl_ioctl -> mpt_fw_download -> mptctl_do_fw_download which calls
mpt_put_msg_frame(mptctl_id, iocp, mf) and calls
mpt_get_msg_frame(mptctl_id, iocp)
Since ioctl can be called at any time, accessing of mptctl_id occurs
concurrently between threads causing a race.
I realize in past messages I've tried to patch by surrounding all
instances of mptctl_id with mutexes but I'm focusing this time on one
clear instance of the race condition involving the variable mptctl_id,
since Julian asks what the exact race condition is with respect to the
case.
Please let me know the confirmation or not confirmation of this race
possibility.
Thank you,
Mark
On Sun, 18 Aug 2019, Julian Calaby wrote:
Hi Mark,
On Thu, Aug 15, 2019 at 8:02 PM Mark Balantzyan wrote:
Certain functions in the driver, such as mptctl_do_fw_download() and
mptctl_do_mpt_command(), rely on the instance of mptctl_id, which does the
id-ing. There is race condition possible when these functions operate in
concurrency. Via, mutexes, the functions are mutually signalled to cooperate.
Changelog v2
Lacked a version number but added properly terminated else condition at
(former) line 2300.
Changelog v3
Fixes "return -EAGAIN" lines which were erroneously tabbed as if to be guarded
by "if" conditions lying above them.
Signed-off-by: Mark Balantzyan
---
Changelog should be down here after the "---"
drivers/message/fusion/mptctl.c | 43 +
1 file changed, 33 insertions(+), 10 deletions(-)
diff --git a/drivers/message/fusion/mptctl.c b/drivers/message/fusion/mptctl.c
index 4470630d..3270843c 100644
--- a/drivers/message/fusion/mptctl.c
+++ b/drivers/message/fusion/mptctl.c
@@ -816,12 +816,15 @@ mptctl_do_fw_download(int ioc, char __user *ufwbuf,
size_t fwlen)
/* Valid device. Get a message frame and construct the FW
download message.
*/
+ mutex_lock(&mpctl_mutex);
if ((mf = mpt_get_msg_frame(mptctl_id, iocp)) == NULL)
- return -EAGAIN;
+ mutex_unlock(&mpctl_mutex);
+ return -EAGAIN;
Are you sure this is right?
1. We're now exiting early with -EAGAIN regardless of the result of
mpt_get_msg_frame()
2. If the result of mpt_get_msg_frame() is not NULL, we don't unlock the mutex
Do you mean something like:
- - - - - -
mutex_lock(&mpctl_mutex);
mf = mpt_get_msg_frame(mptctl_id, iocp);
mutex_unlock(&mpctl_mutex);
if (mf == NULL) {
- - - - - -
@@ -1889,8 +1894,10 @@ mptctl_do_mpt_command (struct mpt_ioctl_command karg,
void __user *mfPtr)
/* Get a free request frame and save the message context.
*/
+ mutex_lock(&mpctl_mutex);
if ((mf = mpt_get_msg_frame(mptctl_id, ioc)) == NULL)
-return -EAGAIN;
+ mutex_unlock(&mpctl_mutex);
+return -EAGAIN;
Same comments here.
@@ -2563,7 +2576,9 @@ mptctl_hp_hostinfo(unsigned long arg, unsigned int
data_size)
/*
* Gather ISTWI(Industry Standard Two Wire Interface) Data
*/
+ mutex_lock(&mpctl_mutex);
if ((mf = mpt_get_msg_frame(mptctl_id, ioc)) == NULL) {
+ mutex_unlock(&mpctl_mutex);
This line needs to be indented to match the line below, also we don't
unlock the mutex if mpt_get_msg_frame() is not NULL.
@@ -3010,9 +3027,11 @@ static int __init mptctl_init(void)
* Install our handler
*/
++where;
+ mutex_lock(&mpctl_mutex);
mptctl_id = mpt_register(mptctl_reply, MPTCTL_DRIVER,
"mptctl_reply");
if (!mptctl_id || mptctl_id >= MPT_MAX_PROTOCOL_DRIVERS) {
+ mutex_unlock(&mpctl_mutex);
Why not use a local variable and only update the global variable if it's valid?
printk(KERN_ERR MYNAM ": ERROR: Failed to register with Fusion MPT
base driver\n");
misc_deregister(&mptctl_miscdev);
err = -EBUSY;
@@ -3022,13 +3041,14 @@ static int __init mptctl_init(void)
mptctl_taskmgmt_id = mpt_register(mptctl_taskmgmt_reply, MPTCTL_DRIVER,
"mptctl_taskmgmt_reply");
if (!mptctl_taskmgmt_id || mptctl_taskmgmt_id >=
MPT_MAX_PROTOCOL_DRIVERS) {
+ mutex_unlock(&mpctl_mutex);
Same comment here.
@@ -3044,13 +3064,14 @@ out_fail:
/*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
static void mptctl_exit(void)
{
+ mutex_lock(&mpctl_mutex);
misc_deregister(&mptctl_miscdev);
printk(KERN_INFO MYNAM ": Deregist
Re: [PATCH v3] lsilogic mpt fusion: mptctl: Fixed race condition around mptctl_id variable using mutexes
Hi Julian, all, I submitted a patch v4 following Julian's review. A function such as "mptctl_do_mpt_command" I don't think is a setup function and so the race condition (likelihood) remains. Again, this was mainly concerning the usage of "mptctl_id" variable in the driver. My objective was just to make it as safe as possible and improve it. Please accept my patch v4 should it suffice. Thank you, Mark
[PATCH v4] lsilogic mpt fusion: mptctl: Fixed race condition in mptctl.ko concerning mptctl_id variable among parallel functions variable among parallel functions
Certain functions in the driver, such as mptctl_do_fw_download() and
mptctl_do_mpt_command(), rely on the instance of mptctl_id, which does the
id-ing. There is race condition possible when these functions operate in
concurrency. Via, mutexes and global/local variable setting, the functions
are mutually signalled to cooperate.
Signed-off-by: Mark Balantzyan
---
Changelog v2
Lacked a version number but added properly terminated else condition at
(former) line 2300.
Changelog v3
Fixes "return -EAGAIN" lines which were erroneously tabbed as if to be guarded
by "if" conditions lying above them.
Changelog v4
Removes a pair of needless mutexes in init() function, allows for checking of
NULL
condition outside of race-fixing mutexes in functions mptctl_do_mpt_command(),
mptctl_do_fw_command, and mptctl_hp_hostinfo().
drivers/message/fusion/mptctl.c | 59 -
1 file changed, 43 insertions(+), 16 deletions(-)
diff --git a/drivers/message/fusion/mptctl.c b/drivers/message/fusion/mptctl.c
index 4470630d..21af0ee8 100644
--- a/drivers/message/fusion/mptctl.c
+++ b/drivers/message/fusion/mptctl.c
@@ -816,12 +816,17 @@ mptctl_do_fw_download(int ioc, char __user *ufwbuf,
size_t fwlen)
/* Valid device. Get a message frame and construct the FW
download message.
*/
- if ((mf = mpt_get_msg_frame(mptctl_id, iocp)) == NULL)
+ mutex_lock(&mpctl_mutex);
+ mf = mpt_get_msg_frame(mptctl_id, iocp);
+ mutex_unlock(&mpctl_mutex);
+ if (mf == NULL) {
return -EAGAIN;
+ }
}
-
+ mutex_lock(&mpctl_mutex);
dctlprintk(iocp, printk(MYIOC_s_DEBUG_FMT
"mptctl_do_fwdl called. mptctl_id = %xh.\n", iocp->name,
mptctl_id));
+ mutex_unlock(&mpctl_mutex);
dctlprintk(iocp, printk(MYIOC_s_DEBUG_FMT "DbG: kfwdl.bufp = %p\n",
iocp->name, ufwbuf));
dctlprintk(iocp, printk(MYIOC_s_DEBUG_FMT "DbG: kfwdl.fwlen = %d\n",
@@ -943,7 +948,9 @@ mptctl_do_fw_download(int ioc, char __user *ufwbuf, size_t
fwlen)
ReplyMsg = NULL;
SET_MGMT_MSG_CONTEXT(iocp->ioctl_cmds.msg_context, dlmsg->MsgContext);
INITIALIZE_MGMT_STATUS(iocp->ioctl_cmds.status)
+ mutex_lock(&mpctl_mutex);
mpt_put_msg_frame(mptctl_id, iocp, mf);
+ mutex_lock(&mpctl_mutex);
/* Now wait for the command to complete */
retry_wait:
@@ -1889,8 +1896,12 @@ mptctl_do_mpt_command (struct mpt_ioctl_command karg,
void __user *mfPtr)
/* Get a free request frame and save the message context.
*/
-if ((mf = mpt_get_msg_frame(mptctl_id, ioc)) == NULL)
-return -EAGAIN;
+ mutex_lock(&mpctl_mutex);
+ mf = mpt_get_msg_frame(mptctl_id, ioc);
+ mutex_unlock(&mpctl_mutex);
+ if (mf == NULL) {
+ return -EAGAIN;
+ }
hdr = (MPIHeader_t *) mf;
msgContext = le32_to_cpu(hdr->MsgContext);
@@ -2271,11 +2282,14 @@ mptctl_do_mpt_command (struct mpt_ioctl_command karg,
void __user *mfPtr)
DBG_DUMP_TM_REQUEST_FRAME(ioc, (u32 *)mf);
if ((ioc->facts.IOCCapabilities &
MPI_IOCFACTS_CAPABILITY_HIGH_PRI_Q) &&
- (ioc->facts.MsgVersion >= MPI_VERSION_01_05))
+ (ioc->facts.MsgVersion >= MPI_VERSION_01_05)) {
+ mutex_lock(&mpctl_mutex);
mpt_put_msg_frame_hi_pri(mptctl_id, ioc, mf);
- else {
- rc =mpt_send_handshake_request(mptctl_id, ioc,
- sizeof(SCSITaskMgmt_t), (u32*)mf, CAN_SLEEP);
+ mutex_unlock(&mpctl_mutex);
+ } else {
+ mutex_lock(&mpctl_mutex);
+ rc = mpt_send_handshake_request(mptctl_id, ioc,
sizeof(SCSITaskMgmt_t), (u32 *)mf, CAN_SLEEP);
+ mutex_unlock(&mpctl_mutex);
if (rc != 0) {
dfailprintk(ioc, printk(MYIOC_s_ERR_FMT
"send_handshake FAILED! (ioc %p, mf %p)\n",
@@ -2287,8 +2301,11 @@ mptctl_do_mpt_command (struct mpt_ioctl_command karg,
void __user *mfPtr)
}
}
- } else
+ } else {
+ mutex_lock(&mpctl_mutex);
mpt_put_msg_frame(mptctl_id, ioc, mf);
+ mutex_unlock(&mpctl_mutex);
+ }
/* Now wait for the command to complete */
timeout = (karg.timeout > 0) ? karg.timeout : MPT_IOCTL_DEFAULT_TIMEOUT;
@@ -2563,7 +2580,10 @@ mptctl_hp_hostinfo(unsigned long arg, unsigned int
data_size)
/*
* Gather ISTWI(Industry Standard Two Wire Interfa
[PATCH v3] lsilogic mpt fusion: mptctl: Fixed race condition around mptctl_id variable using mutexes
Certain functions in the driver, such as mptctl_do_fw_download() and
mptctl_do_mpt_command(), rely on the instance of mptctl_id, which does the
id-ing. There is race condition possible when these functions operate in
concurrency. Via, mutexes, the functions are mutually signalled to cooperate.
Changelog v2
Lacked a version number but added properly terminated else condition at
(former) line 2300.
Changelog v3
Fixes "return -EAGAIN" lines which were erroneously tabbed as if to be guarded
by "if" conditions lying above them.
Signed-off-by: Mark Balantzyan
---
drivers/message/fusion/mptctl.c | 43 +
1 file changed, 33 insertions(+), 10 deletions(-)
diff --git a/drivers/message/fusion/mptctl.c b/drivers/message/fusion/mptctl.c
index 4470630d..3270843c 100644
--- a/drivers/message/fusion/mptctl.c
+++ b/drivers/message/fusion/mptctl.c
@@ -816,12 +816,15 @@ mptctl_do_fw_download(int ioc, char __user *ufwbuf,
size_t fwlen)
/* Valid device. Get a message frame and construct the FW
download message.
*/
+ mutex_lock(&mpctl_mutex);
if ((mf = mpt_get_msg_frame(mptctl_id, iocp)) == NULL)
- return -EAGAIN;
+ mutex_unlock(&mpctl_mutex);
+ return -EAGAIN;
}
-
+ mutex_lock(&mpctl_mutex);
dctlprintk(iocp, printk(MYIOC_s_DEBUG_FMT
"mptctl_do_fwdl called. mptctl_id = %xh.\n", iocp->name,
mptctl_id));
+ mutex_unlock(&mpctl_mutex);
dctlprintk(iocp, printk(MYIOC_s_DEBUG_FMT "DbG: kfwdl.bufp = %p\n",
iocp->name, ufwbuf));
dctlprintk(iocp, printk(MYIOC_s_DEBUG_FMT "DbG: kfwdl.fwlen = %d\n",
@@ -943,7 +946,9 @@ mptctl_do_fw_download(int ioc, char __user *ufwbuf, size_t
fwlen)
ReplyMsg = NULL;
SET_MGMT_MSG_CONTEXT(iocp->ioctl_cmds.msg_context, dlmsg->MsgContext);
INITIALIZE_MGMT_STATUS(iocp->ioctl_cmds.status)
+ mutex_lock(&mpctl_mutex);
mpt_put_msg_frame(mptctl_id, iocp, mf);
+ mutex_lock(&mpctl_mutex);
/* Now wait for the command to complete */
retry_wait:
@@ -1889,8 +1894,10 @@ mptctl_do_mpt_command (struct mpt_ioctl_command karg,
void __user *mfPtr)
/* Get a free request frame and save the message context.
*/
+ mutex_lock(&mpctl_mutex);
if ((mf = mpt_get_msg_frame(mptctl_id, ioc)) == NULL)
-return -EAGAIN;
+ mutex_unlock(&mpctl_mutex);
+return -EAGAIN;
hdr = (MPIHeader_t *) mf;
msgContext = le32_to_cpu(hdr->MsgContext);
@@ -2271,11 +2278,14 @@ mptctl_do_mpt_command (struct mpt_ioctl_command karg,
void __user *mfPtr)
DBG_DUMP_TM_REQUEST_FRAME(ioc, (u32 *)mf);
if ((ioc->facts.IOCCapabilities &
MPI_IOCFACTS_CAPABILITY_HIGH_PRI_Q) &&
- (ioc->facts.MsgVersion >= MPI_VERSION_01_05))
+ (ioc->facts.MsgVersion >= MPI_VERSION_01_05)) {
+ mutex_lock(&mpctl_mutex);
mpt_put_msg_frame_hi_pri(mptctl_id, ioc, mf);
- else {
- rc =mpt_send_handshake_request(mptctl_id, ioc,
- sizeof(SCSITaskMgmt_t), (u32*)mf, CAN_SLEEP);
+ mutex_unlock(&mpctl_mutex);
+ } else {
+ mutex_lock(&mpctl_mutex);
+ rc = mpt_send_handshake_request(mptctl_id, ioc,
sizeof(SCSITaskMgmt_t), (u32 *)mf, CAN_SLEEP);
+ mutex_unlock(&mpctl_mutex);
if (rc != 0) {
dfailprintk(ioc, printk(MYIOC_s_ERR_FMT
"send_handshake FAILED! (ioc %p, mf %p)\n",
@@ -2287,8 +2297,11 @@ mptctl_do_mpt_command (struct mpt_ioctl_command karg,
void __user *mfPtr)
}
}
- } else
+ } else {
+ mutex_lock(&mpctl_mutex);
mpt_put_msg_frame(mptctl_id, ioc, mf);
+ mutex_unlock(&mpctl_mutex);
+ }
/* Now wait for the command to complete */
timeout = (karg.timeout > 0) ? karg.timeout : MPT_IOCTL_DEFAULT_TIMEOUT;
@@ -2563,7 +2576,9 @@ mptctl_hp_hostinfo(unsigned long arg, unsigned int
data_size)
/*
* Gather ISTWI(Industry Standard Two Wire Interface) Data
*/
+ mutex_lock(&mpctl_mutex);
if ((mf = mpt_get_msg_frame(mptctl_id, ioc)) == NULL) {
+ mutex_unlock(&mpctl_mutex);
dfailprintk(ioc, printk(MYIOC_s_WARN_FMT
"%s, no msg frames!!\n", ioc->name, __func__));
goto out;
@@ -2593,7 +2608,9 @@ mptctl_hp_hostinfo(unsigned long arg, unsigned int
[PATCH] lsilogic mpt fusion: mptctl: Fixed race condition around mptctl_id variable using mutexes
Certain functions in the driver, such as mptctl_do_fw_download() and
mptctl_do_mpt_command(), rely on the instance of mptctl_id, which does the
id-ing. There is race condition possible when these functions operate in
concurrency. Via, mutexes, the functions are mutually signalled to cooperate.
Signed-off-by: Mark Balantzyan
---
drivers/message/fusion/mptctl.c | 39 ++---
1 file changed, 31 insertions(+), 8 deletions(-)
diff --git a/drivers/message/fusion/mptctl.c b/drivers/message/fusion/mptctl.c
index 4470630d..f0b49a85 100644
--- a/drivers/message/fusion/mptctl.c
+++ b/drivers/message/fusion/mptctl.c
@@ -816,12 +816,15 @@ mptctl_do_fw_download(int ioc, char __user *ufwbuf,
size_t fwlen)
/* Valid device. Get a message frame and construct the FW
download message.
*/
+ mutex_lock(&mpctl_mutex);
if ((mf = mpt_get_msg_frame(mptctl_id, iocp)) == NULL)
+ mutex_unlock(&mpctl_mutex);
return -EAGAIN;
}
-
+ mutex_lock(&mpctl_mutex);
dctlprintk(iocp, printk(MYIOC_s_DEBUG_FMT
"mptctl_do_fwdl called. mptctl_id = %xh.\n", iocp->name,
mptctl_id));
+ mutex_unlock(&mpctl_mutex);
dctlprintk(iocp, printk(MYIOC_s_DEBUG_FMT "DbG: kfwdl.bufp = %p\n",
iocp->name, ufwbuf));
dctlprintk(iocp, printk(MYIOC_s_DEBUG_FMT "DbG: kfwdl.fwlen = %d\n",
@@ -943,7 +946,9 @@ mptctl_do_fw_download(int ioc, char __user *ufwbuf, size_t
fwlen)
ReplyMsg = NULL;
SET_MGMT_MSG_CONTEXT(iocp->ioctl_cmds.msg_context, dlmsg->MsgContext);
INITIALIZE_MGMT_STATUS(iocp->ioctl_cmds.status)
+ mutex_lock(&mpctl_mutex);
mpt_put_msg_frame(mptctl_id, iocp, mf);
+ mutex_lock(&mpctl_mutex);
/* Now wait for the command to complete */
retry_wait:
@@ -1889,7 +1894,9 @@ mptctl_do_mpt_command (struct mpt_ioctl_command karg,
void __user *mfPtr)
/* Get a free request frame and save the message context.
*/
+ mutex_lock(&mpctl_mutex);
if ((mf = mpt_get_msg_frame(mptctl_id, ioc)) == NULL)
+ mutex_unlock(&mpctl_mutex);
return -EAGAIN;
hdr = (MPIHeader_t *) mf;
@@ -2271,11 +2278,14 @@ mptctl_do_mpt_command (struct mpt_ioctl_command karg,
void __user *mfPtr)
DBG_DUMP_TM_REQUEST_FRAME(ioc, (u32 *)mf);
if ((ioc->facts.IOCCapabilities &
MPI_IOCFACTS_CAPABILITY_HIGH_PRI_Q) &&
- (ioc->facts.MsgVersion >= MPI_VERSION_01_05))
+ (ioc->facts.MsgVersion >= MPI_VERSION_01_05)) {
+ mutex_lock(&mpctl_mutex);
mpt_put_msg_frame_hi_pri(mptctl_id, ioc, mf);
- else {
- rc =mpt_send_handshake_request(mptctl_id, ioc,
- sizeof(SCSITaskMgmt_t), (u32*)mf, CAN_SLEEP);
+ mutex_unlock(&mpctl_mutex);
+ } else {
+ mutex_lock(&mpctl_mutex);
+ rc = mpt_send_handshake_request(mptctl_id, ioc,
sizeof(SCSITaskMgmt_t), (u32 *)mf, CAN_SLEEP);
+ mutex_unlock(&mpctl_mutex);
if (rc != 0) {
dfailprintk(ioc, printk(MYIOC_s_ERR_FMT
"send_handshake FAILED! (ioc %p, mf %p)\n",
@@ -2287,8 +2297,11 @@ mptctl_do_mpt_command (struct mpt_ioctl_command karg,
void __user *mfPtr)
}
}
- } else
+ } else {
+ mutex_lock(&mpctl_mutex);
mpt_put_msg_frame(mptctl_id, ioc, mf);
+ mutex_unlock(&mpctl_mutex);
+ }
/* Now wait for the command to complete */
timeout = (karg.timeout > 0) ? karg.timeout : MPT_IOCTL_DEFAULT_TIMEOUT;
@@ -2563,7 +2576,9 @@ mptctl_hp_hostinfo(unsigned long arg, unsigned int
data_size)
/*
* Gather ISTWI(Industry Standard Two Wire Interface) Data
*/
+ mutex_lock(&mpctl_mutex);
if ((mf = mpt_get_msg_frame(mptctl_id, ioc)) == NULL) {
+ mutex_unlock(&mpctl_mutex);
dfailprintk(ioc, printk(MYIOC_s_WARN_FMT
"%s, no msg frames!!\n", ioc->name, __func__));
goto out;
@@ -2593,7 +2608,9 @@ mptctl_hp_hostinfo(unsigned long arg, unsigned int
data_size)
SET_MGMT_MSG_CONTEXT(ioc->ioctl_cmds.msg_context,
IstwiRWRequest->MsgContext);
INITIALIZE_MGMT_STATUS(ioc->ioctl_cmds.status)
+ mutex_lock(&mpctl_mutex);
mpt_put_msg_frame(mptctl_id, ioc, mf);
+ mutex_unlock(&mpctl_mutex);
retry_wait:
timeleft = wait_for_completion_tim
[PATCH] lsilogic mpt fusion: mptctl: Fixed race condition around mptctl_id variable using mutexes
Certain functions in the driver, such as mptctl_do_fw_download() and
mptctl_do_mpt_command(), rely on the instance of mptctl_id, which does the
id-ing. There is race condition possible when these functions operate in
concurrency. Via, mutexes, the functions are mutually signalled to cooperate.
Signed-off-by: Mark Balantzyan
---
drivers/message/fusion/mptctl.c | 36 ++---
1 file changed, 29 insertions(+), 7 deletions(-)
diff --git a/drivers/message/fusion/mptctl.c b/drivers/message/fusion/mptctl.c
index 4470630d..58ce0fc0 100644
--- a/drivers/message/fusion/mptctl.c
+++ b/drivers/message/fusion/mptctl.c
@@ -816,12 +816,15 @@ mptctl_do_fw_download(int ioc, char __user *ufwbuf,
size_t fwlen)
/* Valid device. Get a message frame and construct the FW
download message.
*/
+ mutex_lock(&mpctl_mutex);
if ((mf = mpt_get_msg_frame(mptctl_id, iocp)) == NULL)
+ mutex_unlock(&mpctl_mutex);
return -EAGAIN;
}
-
+ mutex_lock(&mpctl_mutex);
dctlprintk(iocp, printk(MYIOC_s_DEBUG_FMT
"mptctl_do_fwdl called. mptctl_id = %xh.\n", iocp->name,
mptctl_id));
+ mutex_unlock(&mpctl_mutex);
dctlprintk(iocp, printk(MYIOC_s_DEBUG_FMT "DbG: kfwdl.bufp = %p\n",
iocp->name, ufwbuf));
dctlprintk(iocp, printk(MYIOC_s_DEBUG_FMT "DbG: kfwdl.fwlen = %d\n",
@@ -943,7 +946,9 @@ mptctl_do_fw_download(int ioc, char __user *ufwbuf, size_t
fwlen)
ReplyMsg = NULL;
SET_MGMT_MSG_CONTEXT(iocp->ioctl_cmds.msg_context, dlmsg->MsgContext);
INITIALIZE_MGMT_STATUS(iocp->ioctl_cmds.status)
+ mutex_lock(&mpctl_mutex);
mpt_put_msg_frame(mptctl_id, iocp, mf);
+ mutex_lock(&mpctl_mutex);
/* Now wait for the command to complete */
retry_wait:
@@ -1889,7 +1894,9 @@ mptctl_do_mpt_command (struct mpt_ioctl_command karg,
void __user *mfPtr)
/* Get a free request frame and save the message context.
*/
+ mutex_lock(&mpctl_mutex);
if ((mf = mpt_get_msg_frame(mptctl_id, ioc)) == NULL)
+ mutex_unlock(&mpctl_mutex);
return -EAGAIN;
hdr = (MPIHeader_t *) mf;
@@ -2271,11 +2278,14 @@ mptctl_do_mpt_command (struct mpt_ioctl_command karg,
void __user *mfPtr)
DBG_DUMP_TM_REQUEST_FRAME(ioc, (u32 *)mf);
if ((ioc->facts.IOCCapabilities &
MPI_IOCFACTS_CAPABILITY_HIGH_PRI_Q) &&
- (ioc->facts.MsgVersion >= MPI_VERSION_01_05))
+ (ioc->facts.MsgVersion >= MPI_VERSION_01_05)) {
+ mutex_lock(&mpctl_mutex);
mpt_put_msg_frame_hi_pri(mptctl_id, ioc, mf);
- else {
- rc =mpt_send_handshake_request(mptctl_id, ioc,
- sizeof(SCSITaskMgmt_t), (u32*)mf, CAN_SLEEP);
+ mutex_unlock(&mpctl_mutex);
+ } else {
+ mutex_lock(&mpctl_mutex);
+ rc = mpt_send_handshake_request(mptctl_id, ioc,
sizeof(SCSITaskMgmt_t), (u32 *)mf, CAN_SLEEP);
+ mutex_unlock(&mpctl_mutex);
if (rc != 0) {
dfailprintk(ioc, printk(MYIOC_s_ERR_FMT
"send_handshake FAILED! (ioc %p, mf %p)\n",
@@ -2288,7 +2298,9 @@ mptctl_do_mpt_command (struct mpt_ioctl_command karg,
void __user *mfPtr)
}
} else
+ mutex_lock(&mpctl_mutex);
mpt_put_msg_frame(mptctl_id, ioc, mf);
+ mutex_unlock(&mpctl_mutex);
/* Now wait for the command to complete */
timeout = (karg.timeout > 0) ? karg.timeout : MPT_IOCTL_DEFAULT_TIMEOUT;
@@ -2563,7 +2575,9 @@ mptctl_hp_hostinfo(unsigned long arg, unsigned int
data_size)
/*
* Gather ISTWI(Industry Standard Two Wire Interface) Data
*/
+ mutex_lock(&mpctl_mutex);
if ((mf = mpt_get_msg_frame(mptctl_id, ioc)) == NULL) {
+ mutex_unlock(&mpctl_mutex);
dfailprintk(ioc, printk(MYIOC_s_WARN_FMT
"%s, no msg frames!!\n", ioc->name, __func__));
goto out;
@@ -2593,7 +2607,9 @@ mptctl_hp_hostinfo(unsigned long arg, unsigned int
data_size)
SET_MGMT_MSG_CONTEXT(ioc->ioctl_cmds.msg_context,
IstwiRWRequest->MsgContext);
INITIALIZE_MGMT_STATUS(ioc->ioctl_cmds.status)
+ mutex_lock(&mpctl_mutex);
mpt_put_msg_frame(mptctl_id, ioc, mf);
+ mutex_unlock(&mpctl_mutex);
retry_wait:
timeleft = wait_for_completion_timeout(&ioc->ioctl_cmds.done,
@@ -3010,9 +3026,11 @@ static int __init mptctl_init(vo
[PATCH] lsilogic mpt fusion: mptctl: Fixed race condition around mptctl_id variable using mutexes
Certain functions in the driver, such as mptctl_do_fw_download() and
mptctl_do_mpt_command(), rely on the instance of mptctl_id, which does the
id-ing. There is race condition possible when these functions operate in
concurrency. Via, mutexes, the functions are mutually signalled to cooperate.
Signed-off-by: Mark Balantzyan
---
drivers/message/fusion/mptctl.c | 36 ++---
1 file changed, 29 insertions(+), 7 deletions(-)
diff --git a/drivers/message/fusion/mptctl.c b/drivers/message/fusion/mptctl.c
index 4470630d..58ce0fc0 100644
--- a/drivers/message/fusion/mptctl.c
+++ b/drivers/message/fusion/mptctl.c
@@ -816,12 +816,15 @@ mptctl_do_fw_download(int ioc, char __user *ufwbuf,
size_t fwlen)
/* Valid device. Get a message frame and construct the FW
download message.
*/
+ mutex_lock(&mpctl_mutex);
if ((mf = mpt_get_msg_frame(mptctl_id, iocp)) == NULL)
+ mutex_unlock(&mpctl_mutex);
return -EAGAIN;
}
-
+ mutex_lock(&mpctl_mutex);
dctlprintk(iocp, printk(MYIOC_s_DEBUG_FMT
"mptctl_do_fwdl called. mptctl_id = %xh.\n", iocp->name,
mptctl_id));
+ mutex_unlock(&mpctl_mutex);
dctlprintk(iocp, printk(MYIOC_s_DEBUG_FMT "DbG: kfwdl.bufp = %p\n",
iocp->name, ufwbuf));
dctlprintk(iocp, printk(MYIOC_s_DEBUG_FMT "DbG: kfwdl.fwlen = %d\n",
@@ -943,7 +946,9 @@ mptctl_do_fw_download(int ioc, char __user *ufwbuf, size_t
fwlen)
ReplyMsg = NULL;
SET_MGMT_MSG_CONTEXT(iocp->ioctl_cmds.msg_context, dlmsg->MsgContext);
INITIALIZE_MGMT_STATUS(iocp->ioctl_cmds.status)
+ mutex_lock(&mpctl_mutex);
mpt_put_msg_frame(mptctl_id, iocp, mf);
+ mutex_lock(&mpctl_mutex);
/* Now wait for the command to complete */
retry_wait:
@@ -1889,7 +1894,9 @@ mptctl_do_mpt_command (struct mpt_ioctl_command karg,
void __user *mfPtr)
/* Get a free request frame and save the message context.
*/
+ mutex_lock(&mpctl_mutex);
if ((mf = mpt_get_msg_frame(mptctl_id, ioc)) == NULL)
+ mutex_unlock(&mpctl_mutex);
return -EAGAIN;
hdr = (MPIHeader_t *) mf;
@@ -2271,11 +2278,14 @@ mptctl_do_mpt_command (struct mpt_ioctl_command karg,
void __user *mfPtr)
DBG_DUMP_TM_REQUEST_FRAME(ioc, (u32 *)mf);
if ((ioc->facts.IOCCapabilities &
MPI_IOCFACTS_CAPABILITY_HIGH_PRI_Q) &&
- (ioc->facts.MsgVersion >= MPI_VERSION_01_05))
+ (ioc->facts.MsgVersion >= MPI_VERSION_01_05)) {
+ mutex_lock(&mpctl_mutex);
mpt_put_msg_frame_hi_pri(mptctl_id, ioc, mf);
- else {
- rc =mpt_send_handshake_request(mptctl_id, ioc,
- sizeof(SCSITaskMgmt_t), (u32*)mf, CAN_SLEEP);
+ mutex_unlock(&mpctl_mutex);
+ } else {
+ mutex_lock(&mpctl_mutex);
+ rc = mpt_send_handshake_request(mptctl_id, ioc,
sizeof(SCSITaskMgmt_t), (u32 *)mf, CAN_SLEEP);
+ mutex_unlock(&mpctl_mutex);
if (rc != 0) {
dfailprintk(ioc, printk(MYIOC_s_ERR_FMT
"send_handshake FAILED! (ioc %p, mf %p)\n",
@@ -2288,7 +2298,9 @@ mptctl_do_mpt_command (struct mpt_ioctl_command karg,
void __user *mfPtr)
}
} else
+ mutex_lock(&mpctl_mutex);
mpt_put_msg_frame(mptctl_id, ioc, mf);
+ mutex_unlock(&mpctl_mutex);
/* Now wait for the command to complete */
timeout = (karg.timeout > 0) ? karg.timeout : MPT_IOCTL_DEFAULT_TIMEOUT;
@@ -2563,7 +2575,9 @@ mptctl_hp_hostinfo(unsigned long arg, unsigned int
data_size)
/*
* Gather ISTWI(Industry Standard Two Wire Interface) Data
*/
+ mutex_lock(&mpctl_mutex);
if ((mf = mpt_get_msg_frame(mptctl_id, ioc)) == NULL) {
+ mutex_unlock(&mpctl_mutex);
dfailprintk(ioc, printk(MYIOC_s_WARN_FMT
"%s, no msg frames!!\n", ioc->name, __func__));
goto out;
@@ -2593,7 +2607,9 @@ mptctl_hp_hostinfo(unsigned long arg, unsigned int
data_size)
SET_MGMT_MSG_CONTEXT(ioc->ioctl_cmds.msg_context,
IstwiRWRequest->MsgContext);
INITIALIZE_MGMT_STATUS(ioc->ioctl_cmds.status)
+ mutex_lock(&mpctl_mutex);
mpt_put_msg_frame(mptctl_id, ioc, mf);
+ mutex_unlock(&mpctl_mutex);
retry_wait:
timeleft = wait_for_completion_timeout(&ioc->ioctl_cmds.done,
@@ -3010,9 +3026,11 @@ static int __init mptctl_init(vo
Re: [PATCH v4] watchdog: alim1535: Rewriting of alim1535 driver to use watchdog subsystem
Dear Ondrej, As advised by another kernel maintainer, patches for antiquated drivers like these (this one which I test-built successfully) should hang around until someone with the hardware volunteers to test it. Therefore, I would provide the software and the individual would serve as the hardware tester with the provided software. So far, I've worked with Guenter Roeck to conform it to the watchdog subsystem. And now I'm letting it sit until someone with the hardware is able to test it. Thank you, Mark
Re: [PATCH v4] watchdog: alim1535: Rewriting of alim1535 to use watchdog subsystem
Please see: https://lkml.org/lkml/2019/8/2/6 Thank you.
Re: [PATCH v4] watchdog: alim1535: Rewriting of alim1535 to use watchdog subsystem
There was some sort of filesystem error that cause the wrong file to be saved. Hence, yes, possibly there was no difference betwee the v2 and the 'v4' (which should have been the 'v3'). Since the last one, though unchanged from v2, was a changelog-less 'v3', THIS is an actual 'v4'. Thank you, Mark
[PATCH v4] watchdog: alim1535: Rewriting of alim1535 driver to use watchdog subsystem
This patch rewrites the alim1535_wdt driver to use the watchdog subsystem.
By virtue of this, it also fixes a (theoretical) race condition between the
formerly arranged ali_timeout_bits and ali_settimer() interoperation.
Signed-off-by: Mark Balantzyan
---
drivers/watchdog/Kconfig| 1 +
drivers/watchdog/alim1535_wdt.c | 294 ++--
2 files changed, 50 insertions(+), 245 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9af07fd9..980b0c90 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -853,6 +853,7 @@ config ADVANTECH_WDT
config ALIM1535_WDT
tristate "ALi M1535 PMU Watchdog Timer"
+ select WATCHDOG_CORE
depends on X86 && PCI
---help---
This is the driver for the hardware watchdog on the ALi M1535 PMU.
diff --git a/drivers/watchdog/alim1535_wdt.c b/drivers/watchdog/alim1535_wdt.c
index 60f0c2eb..747ba12c 100644
--- a/drivers/watchdog/alim1535_wdt.c
+++ b/drivers/watchdog/alim1535_wdt.c
@@ -12,29 +12,20 @@
#include
#include
#include
-#include
#include
#include
-#include
-#include
-#include
-#include
#include
-#include
-#include
#define WATCHDOG_NAME "ALi_M1535"
#define WATCHDOG_TIMEOUT 60/* 60 sec default timeout */
/* internal variables */
-static unsigned long ali_is_open;
-static char ali_expect_release;
static struct pci_dev *ali_pci;
static u32 ali_timeout_bits; /* stores the computed timeout */
static DEFINE_SPINLOCK(ali_lock); /* Guards the hardware */
/* module parameters */
-static int timeout = WATCHDOG_TIMEOUT;
+static int timeout;
module_param(timeout, int, 0);
MODULE_PARM_DESC(timeout,
"Watchdog timeout in seconds. (0 < timeout < 18000, default="
@@ -53,18 +44,15 @@ MODULE_PARM_DESC(nowayout,
* configuration set.
*/
-static void ali_start(void)
+static int ali_start(struct watchdog_device *wdd)
{
u32 val;
- spin_lock(&ali_lock);
-
pci_read_config_dword(ali_pci, 0xCC, &val);
val &= ~0x3F; /* Mask count */
val |= (1 << 25) | ali_timeout_bits;
pci_write_config_dword(ali_pci, 0xCC, val);
-
- spin_unlock(&ali_lock);
+ return 0;
}
/*
@@ -73,225 +61,53 @@ static void ali_start(void)
* Stop the ALi watchdog countdown
*/
-static void ali_stop(void)
+static int ali_stop(struct watchdog_device *wdd)
{
u32 val;
- spin_lock(&ali_lock);
-
pci_read_config_dword(ali_pci, 0xCC, &val);
val &= ~0x3F; /* Mask count to zero (disabled) */
val &= ~(1 << 25); /* and for safety mask the reset enable */
pci_write_config_dword(ali_pci, 0xCC, val);
-
- spin_unlock(&ali_lock);
-}
-
-/*
- * ali_keepalive - send a keepalive to the watchdog
- *
- * Send a keepalive to the timer (actually we restart the timer).
- */
-
-static void ali_keepalive(void)
-{
- ali_start();
+ return 0;
}
/*
- * ali_settimer- compute the timer reload value
+ * ali_set_timeout - compute the timer reload value
* @t: time in seconds
*
* Computes the timeout values needed
*/
-static int ali_settimer(int t)
+static int ali_set_timeout(struct watchdog_device *wdd, unsigned int t)
{
- if (t < 0)
- return -EINVAL;
- else if (t < 60)
- ali_timeout_bits = t|(1 << 6);
- else if (t < 3600)
- ali_timeout_bits = (t / 60)|(1 << 7);
- else if (t < 18000)
- ali_timeout_bits = (t / 300)|(1 << 6)|(1 << 7);
- else
+ spin_lock(&ali_lock);
+ if (t < 0) {
+ spin_unlock(&ali_lock);
return -EINVAL;
-
- timeout = t;
- return 0;
-}
-
-/*
- * /dev/watchdog handling
- */
-
-/*
- * ali_write - writes to ALi watchdog
- * @file: file from VFS
- * @data: user address of data
- * @len: length of data
- * @ppos: pointer to the file offset
- *
- * Handle a write to the ALi watchdog. Writing to the file pings
- * the watchdog and resets it. Writing the magic 'V' sequence allows
- * the next close to turn off the watchdog.
- */
-
-static ssize_t ali_write(struct file *file, const char __user *data,
- size_t len, loff_t *ppos)
-{
- /* See if we got the magic character 'V' and reload the timer */
- if (len) {
- if (!nowayout) {
- size_t i;
-
- /* note: just in case someone wrote the
- magic character five months ago... */
- ali_expect_release = 0;
-
- /* scan to see whether or not we got
- the magic cha
Re: [PATCH v4] watchdog: alim1535: Rewriting of alim1535 to use watchdog subsystem
Take a second look. It is different. And I had to amend the title hence
the second send to include the subsystem, as you so duly tend to remind
me. It just so happened that I mistyped and skipped a version by...1.
Mark
On Thu, 1 Aug 2019, Guenter Roeck wrote:
On 8/1/19 8:26 PM, Mark Balantzyan wrote:
This patch rewrites the alim1535_wdt driver to use the watchdog subsystem.
By virtue of this, it also fixes a (theoretical) race condition between the
formerly arranged ali_timeout_bits and ali_settimer() interoperation.
Signed-off-by: Mark Balantzyan
This is v4. A minute ago I received v1. Earlier today I got v2, which I
commented on. I didn't see v3 of this patch. I don't see a change log.
At first glance, I don't see a difference to v2.
Sorry, this is a waste of my time. I won't review or comment further.
Guenter
---
drivers/watchdog/Kconfig| 1 +
drivers/watchdog/alim1535_wdt.c | 275 +---
2 files changed, 37 insertions(+), 239 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9af07fd9..980b0c90 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -853,6 +853,7 @@ config ADVANTECH_WDT
config ALIM1535_WDT
tristate "ALi M1535 PMU Watchdog Timer"
+ select WATCHDOG_CORE
depends on X86 && PCI
---help---
This is the driver for the hardware watchdog on the ALi M1535 PMU.
diff --git a/drivers/watchdog/alim1535_wdt.c
b/drivers/watchdog/alim1535_wdt.c
index 60f0c2eb..55648ba8 100644
--- a/drivers/watchdog/alim1535_wdt.c
+++ b/drivers/watchdog/alim1535_wdt.c
@@ -12,26 +12,18 @@
#include
#include
#include
-#include
#include
#include
-#include
-#include
-#include
-#include
-#include
#include
#include
+#include
#define WATCHDOG_NAME "ALi_M1535"
#define WATCHDOG_TIMEOUT 60 /* 60 sec default timeout */
/* internal variables */
-static unsigned long ali_is_open;
-static char ali_expect_release;
static struct pci_dev *ali_pci;
static u32 ali_timeout_bits; /* stores the computed
timeout */
-static DEFINE_SPINLOCK(ali_lock); /* Guards the hardware */
/* module parameters */
static int timeout = WATCHDOG_TIMEOUT;
@@ -53,18 +45,15 @@ MODULE_PARM_DESC(nowayout,
*configuration set.
*/
-static void ali_start(void)
+static int ali_start(struct watchdog_device *wdd)
{
u32 val;
- spin_lock(&ali_lock);
-
pci_read_config_dword(ali_pci, 0xCC, &val);
val &= ~0x3F; /* Mask count */
val |= (1 << 25) | ali_timeout_bits;
pci_write_config_dword(ali_pci, 0xCC, val);
-
- spin_unlock(&ali_lock);
+ return 0;
}
/*
@@ -73,18 +62,15 @@ static void ali_start(void)
*Stop the ALi watchdog countdown
*/
-static void ali_stop(void)
+static int ali_stop(struct watchdog_device *wdd)
{
u32 val;
- spin_lock(&ali_lock);
-
pci_read_config_dword(ali_pci, 0xCC, &val);
val &= ~0x3F; /* Mask count to zero (disabled) */
val &= ~(1 << 25);/* and for safety mask the reset enable */
pci_write_config_dword(ali_pci, 0xCC, val);
-
- spin_unlock(&ali_lock);
+ return 0;
}
/*
@@ -93,32 +79,24 @@ static void ali_stop(void)
*Send a keepalive to the timer (actually we restart the timer).
*/
-static void ali_keepalive(void)
+static int ali_keepalive(struct watchdog_device *wdd)
{
- ali_start();
+ ali_start(wdd);
+ return 0;
}
/*
- * ali_settimer- compute the timer reload value
+ * ali_set_timeout - compute the timer reload value
*@t: time in seconds
*
*Computes the timeout values needed
*/
-static int ali_settimer(int t)
+static int ali_set_timeout(struct watchdog_device *wdd, unsigned int t)
{
- if (t < 0)
- return -EINVAL;
- else if (t < 60)
- ali_timeout_bits = t|(1 << 6);
- else if (t < 3600)
- ali_timeout_bits = (t / 60)|(1 << 7);
- else if (t < 18000)
- ali_timeout_bits = (t / 300)|(1 << 6)|(1 << 7);
- else
- return -EINVAL;
-
- timeout = t;
+ wdd->max_timeout = 60;
+ wdd->min_timeout = 1;
+ wdd->timeout = t;
return 0;
}
@@ -126,172 +104,6 @@ static int ali_settimer(int t)
*/dev/watchdog handling
*/
-/*
- * ali_write - writes to ALi watchdog
- * @file: file from VFS
- * @data: user address of data
- * @len: length of data
- * @ppos: pointer to the file offset
- *
- * Handle a write to the ALi watchdog. Writing to the file pings
- * the watchdog and resets it. Writing the magic 'V' sequence allows
- * the next close to turn off the watchdog.
- */
-
-static ssize
[PATCH v4] watchdog: alim1535: Rewriting of alim1535 to use watchdog subsystem
This patch rewrites the alim1535_wdt driver to use the watchdog subsystem.
By virtue of this, it also fixes a (theoretical) race condition between the
formerly arranged ali_timeout_bits and ali_settimer() interoperation.
Signed-off-by: Mark Balantzyan
---
drivers/watchdog/Kconfig| 1 +
drivers/watchdog/alim1535_wdt.c | 275 +---
2 files changed, 37 insertions(+), 239 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9af07fd9..980b0c90 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -853,6 +853,7 @@ config ADVANTECH_WDT
config ALIM1535_WDT
tristate "ALi M1535 PMU Watchdog Timer"
+ select WATCHDOG_CORE
depends on X86 && PCI
---help---
This is the driver for the hardware watchdog on the ALi M1535 PMU.
diff --git a/drivers/watchdog/alim1535_wdt.c b/drivers/watchdog/alim1535_wdt.c
index 60f0c2eb..55648ba8 100644
--- a/drivers/watchdog/alim1535_wdt.c
+++ b/drivers/watchdog/alim1535_wdt.c
@@ -12,26 +12,18 @@
#include
#include
#include
-#include
#include
#include
-#include
-#include
-#include
-#include
-#include
#include
#include
+#include
#define WATCHDOG_NAME "ALi_M1535"
#define WATCHDOG_TIMEOUT 60/* 60 sec default timeout */
/* internal variables */
-static unsigned long ali_is_open;
-static char ali_expect_release;
static struct pci_dev *ali_pci;
static u32 ali_timeout_bits; /* stores the computed timeout */
-static DEFINE_SPINLOCK(ali_lock); /* Guards the hardware */
/* module parameters */
static int timeout = WATCHDOG_TIMEOUT;
@@ -53,18 +45,15 @@ MODULE_PARM_DESC(nowayout,
* configuration set.
*/
-static void ali_start(void)
+static int ali_start(struct watchdog_device *wdd)
{
u32 val;
- spin_lock(&ali_lock);
-
pci_read_config_dword(ali_pci, 0xCC, &val);
val &= ~0x3F; /* Mask count */
val |= (1 << 25) | ali_timeout_bits;
pci_write_config_dword(ali_pci, 0xCC, val);
-
- spin_unlock(&ali_lock);
+ return 0;
}
/*
@@ -73,18 +62,15 @@ static void ali_start(void)
* Stop the ALi watchdog countdown
*/
-static void ali_stop(void)
+static int ali_stop(struct watchdog_device *wdd)
{
u32 val;
- spin_lock(&ali_lock);
-
pci_read_config_dword(ali_pci, 0xCC, &val);
val &= ~0x3F; /* Mask count to zero (disabled) */
val &= ~(1 << 25); /* and for safety mask the reset enable */
pci_write_config_dword(ali_pci, 0xCC, val);
-
- spin_unlock(&ali_lock);
+ return 0;
}
/*
@@ -93,32 +79,24 @@ static void ali_stop(void)
* Send a keepalive to the timer (actually we restart the timer).
*/
-static void ali_keepalive(void)
+static int ali_keepalive(struct watchdog_device *wdd)
{
- ali_start();
+ ali_start(wdd);
+ return 0;
}
/*
- * ali_settimer- compute the timer reload value
+ * ali_set_timeout - compute the timer reload value
* @t: time in seconds
*
* Computes the timeout values needed
*/
-static int ali_settimer(int t)
+static int ali_set_timeout(struct watchdog_device *wdd, unsigned int t)
{
- if (t < 0)
- return -EINVAL;
- else if (t < 60)
- ali_timeout_bits = t|(1 << 6);
- else if (t < 3600)
- ali_timeout_bits = (t / 60)|(1 << 7);
- else if (t < 18000)
- ali_timeout_bits = (t / 300)|(1 << 6)|(1 << 7);
- else
- return -EINVAL;
-
- timeout = t;
+ wdd->max_timeout = 60;
+ wdd->min_timeout = 1;
+ wdd->timeout = t;
return 0;
}
@@ -126,172 +104,6 @@ static int ali_settimer(int t)
* /dev/watchdog handling
*/
-/*
- * ali_write - writes to ALi watchdog
- * @file: file from VFS
- * @data: user address of data
- * @len: length of data
- * @ppos: pointer to the file offset
- *
- * Handle a write to the ALi watchdog. Writing to the file pings
- * the watchdog and resets it. Writing the magic 'V' sequence allows
- * the next close to turn off the watchdog.
- */
-
-static ssize_t ali_write(struct file *file, const char __user *data,
- size_t len, loff_t *ppos)
-{
- /* See if we got the magic character 'V' and reload the timer */
- if (len) {
- if (!nowayout) {
- size_t i;
-
- /* note: just in case someone wrote the
- magic character five months ago... */
- ali_expect_release = 0;
-
- /* scan to see whether or not we got
- the magic character */
-
[PATCH] Rewriting of alim1535 to use watchdog subsystem
This patch rewrites the alim1535_wdt driver to use the watchdog subsystem.
By virtue of this, it also fixes a (theoretical) race condition between the
formerly arranged ali_timeout_bits and ali_settimer() interoperation.
Signed-off-by: Mark Balantzyan
---
drivers/watchdog/Kconfig| 1 +
drivers/watchdog/alim1535_wdt.c | 275 +---
2 files changed, 37 insertions(+), 239 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9af07fd9..980b0c90 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -853,6 +853,7 @@ config ADVANTECH_WDT
config ALIM1535_WDT
tristate "ALi M1535 PMU Watchdog Timer"
+ select WATCHDOG_CORE
depends on X86 && PCI
---help---
This is the driver for the hardware watchdog on the ALi M1535 PMU.
diff --git a/drivers/watchdog/alim1535_wdt.c b/drivers/watchdog/alim1535_wdt.c
index 60f0c2eb..55648ba8 100644
--- a/drivers/watchdog/alim1535_wdt.c
+++ b/drivers/watchdog/alim1535_wdt.c
@@ -12,26 +12,18 @@
#include
#include
#include
-#include
#include
#include
-#include
-#include
-#include
-#include
-#include
#include
#include
+#include
#define WATCHDOG_NAME "ALi_M1535"
#define WATCHDOG_TIMEOUT 60/* 60 sec default timeout */
/* internal variables */
-static unsigned long ali_is_open;
-static char ali_expect_release;
static struct pci_dev *ali_pci;
static u32 ali_timeout_bits; /* stores the computed timeout */
-static DEFINE_SPINLOCK(ali_lock); /* Guards the hardware */
/* module parameters */
static int timeout = WATCHDOG_TIMEOUT;
@@ -53,18 +45,15 @@ MODULE_PARM_DESC(nowayout,
* configuration set.
*/
-static void ali_start(void)
+static int ali_start(struct watchdog_device *wdd)
{
u32 val;
- spin_lock(&ali_lock);
-
pci_read_config_dword(ali_pci, 0xCC, &val);
val &= ~0x3F; /* Mask count */
val |= (1 << 25) | ali_timeout_bits;
pci_write_config_dword(ali_pci, 0xCC, val);
-
- spin_unlock(&ali_lock);
+ return 0;
}
/*
@@ -73,18 +62,15 @@ static void ali_start(void)
* Stop the ALi watchdog countdown
*/
-static void ali_stop(void)
+static int ali_stop(struct watchdog_device *wdd)
{
u32 val;
- spin_lock(&ali_lock);
-
pci_read_config_dword(ali_pci, 0xCC, &val);
val &= ~0x3F; /* Mask count to zero (disabled) */
val &= ~(1 << 25); /* and for safety mask the reset enable */
pci_write_config_dword(ali_pci, 0xCC, val);
-
- spin_unlock(&ali_lock);
+ return 0;
}
/*
@@ -93,32 +79,24 @@ static void ali_stop(void)
* Send a keepalive to the timer (actually we restart the timer).
*/
-static void ali_keepalive(void)
+static int ali_keepalive(struct watchdog_device *wdd)
{
- ali_start();
+ ali_start(wdd);
+ return 0;
}
/*
- * ali_settimer- compute the timer reload value
+ * ali_set_timeout - compute the timer reload value
* @t: time in seconds
*
* Computes the timeout values needed
*/
-static int ali_settimer(int t)
+static int ali_set_timeout(struct watchdog_device *wdd, unsigned int t)
{
- if (t < 0)
- return -EINVAL;
- else if (t < 60)
- ali_timeout_bits = t|(1 << 6);
- else if (t < 3600)
- ali_timeout_bits = (t / 60)|(1 << 7);
- else if (t < 18000)
- ali_timeout_bits = (t / 300)|(1 << 6)|(1 << 7);
- else
- return -EINVAL;
-
- timeout = t;
+ wdd->max_timeout = 60;
+ wdd->min_timeout = 1;
+ wdd->timeout = t;
return 0;
}
@@ -126,172 +104,6 @@ static int ali_settimer(int t)
* /dev/watchdog handling
*/
-/*
- * ali_write - writes to ALi watchdog
- * @file: file from VFS
- * @data: user address of data
- * @len: length of data
- * @ppos: pointer to the file offset
- *
- * Handle a write to the ALi watchdog. Writing to the file pings
- * the watchdog and resets it. Writing the magic 'V' sequence allows
- * the next close to turn off the watchdog.
- */
-
-static ssize_t ali_write(struct file *file, const char __user *data,
- size_t len, loff_t *ppos)
-{
- /* See if we got the magic character 'V' and reload the timer */
- if (len) {
- if (!nowayout) {
- size_t i;
-
- /* note: just in case someone wrote the
- magic character five months ago... */
- ali_expect_release = 0;
-
- /* scan to see whether or not we got
- the magic character */
-
[PATCH v2] watchdog: alim1535: Rewriting of alim1535 to use watchdog subsystem
This patch rewrites the alim1535_wdt driver to use the watchdog subsystem. By
virtue of this, it also fixes a potential race condition between
ali_timeout_bits and ali_settimer().
Signed-off-by: Mark Balantzyan
---
drivers/watchdog/Kconfig| 1 +
drivers/watchdog/alim1535_wdt.c | 275 +---
2 files changed, 37 insertions(+), 239 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9af07fd9..980b0c90 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -853,6 +853,7 @@ config ADVANTECH_WDT
config ALIM1535_WDT
tristate "ALi M1535 PMU Watchdog Timer"
+ select WATCHDOG_CORE
depends on X86 && PCI
---help---
This is the driver for the hardware watchdog on the ALi M1535 PMU.
diff --git a/drivers/watchdog/alim1535_wdt.c b/drivers/watchdog/alim1535_wdt.c
index 60f0c2eb..55648ba8 100644
--- a/drivers/watchdog/alim1535_wdt.c
+++ b/drivers/watchdog/alim1535_wdt.c
@@ -12,26 +12,18 @@
#include
#include
#include
-#include
#include
#include
-#include
-#include
-#include
-#include
-#include
#include
#include
+#include
#define WATCHDOG_NAME "ALi_M1535"
#define WATCHDOG_TIMEOUT 60/* 60 sec default timeout */
/* internal variables */
-static unsigned long ali_is_open;
-static char ali_expect_release;
static struct pci_dev *ali_pci;
static u32 ali_timeout_bits; /* stores the computed timeout */
-static DEFINE_SPINLOCK(ali_lock); /* Guards the hardware */
/* module parameters */
static int timeout = WATCHDOG_TIMEOUT;
@@ -53,18 +45,15 @@ MODULE_PARM_DESC(nowayout,
* configuration set.
*/
-static void ali_start(void)
+static int ali_start(struct watchdog_device *wdd)
{
u32 val;
- spin_lock(&ali_lock);
-
pci_read_config_dword(ali_pci, 0xCC, &val);
val &= ~0x3F; /* Mask count */
val |= (1 << 25) | ali_timeout_bits;
pci_write_config_dword(ali_pci, 0xCC, val);
-
- spin_unlock(&ali_lock);
+ return 0;
}
/*
@@ -73,18 +62,15 @@ static void ali_start(void)
* Stop the ALi watchdog countdown
*/
-static void ali_stop(void)
+static int ali_stop(struct watchdog_device *wdd)
{
u32 val;
- spin_lock(&ali_lock);
-
pci_read_config_dword(ali_pci, 0xCC, &val);
val &= ~0x3F; /* Mask count to zero (disabled) */
val &= ~(1 << 25); /* and for safety mask the reset enable */
pci_write_config_dword(ali_pci, 0xCC, val);
-
- spin_unlock(&ali_lock);
+ return 0;
}
/*
@@ -93,32 +79,24 @@ static void ali_stop(void)
* Send a keepalive to the timer (actually we restart the timer).
*/
-static void ali_keepalive(void)
+static int ali_keepalive(struct watchdog_device *wdd)
{
- ali_start();
+ ali_start(wdd);
+ return 0;
}
/*
- * ali_settimer- compute the timer reload value
+ * ali_set_timeout - compute the timer reload value
* @t: time in seconds
*
* Computes the timeout values needed
*/
-static int ali_settimer(int t)
+static int ali_set_timeout(struct watchdog_device *wdd, unsigned int t)
{
- if (t < 0)
- return -EINVAL;
- else if (t < 60)
- ali_timeout_bits = t|(1 << 6);
- else if (t < 3600)
- ali_timeout_bits = (t / 60)|(1 << 7);
- else if (t < 18000)
- ali_timeout_bits = (t / 300)|(1 << 6)|(1 << 7);
- else
- return -EINVAL;
-
- timeout = t;
+ wdd->max_timeout = 60;
+ wdd->min_timeout = 1;
+ wdd->timeout = t;
return 0;
}
@@ -126,172 +104,6 @@ static int ali_settimer(int t)
* /dev/watchdog handling
*/
-/*
- * ali_write - writes to ALi watchdog
- * @file: file from VFS
- * @data: user address of data
- * @len: length of data
- * @ppos: pointer to the file offset
- *
- * Handle a write to the ALi watchdog. Writing to the file pings
- * the watchdog and resets it. Writing the magic 'V' sequence allows
- * the next close to turn off the watchdog.
- */
-
-static ssize_t ali_write(struct file *file, const char __user *data,
- size_t len, loff_t *ppos)
-{
- /* See if we got the magic character 'V' and reload the timer */
- if (len) {
- if (!nowayout) {
- size_t i;
-
- /* note: just in case someone wrote the
- magic character five months ago... */
- ali_expect_release = 0;
-
- /* scan to see whether or not we got
- the magic character */
- for (i = 0; i != len; i++) {
-
Re: [PATCH] watchdog:alim1535_wdt: Fix data race in ali_settimer() concerning ali_timeout_bits variable.
Hi Guenter, all, I don't really understand this focus on fixing theoretic/irrelevant race conditions in drivers which no one uses anymore. Maybe someone can enlighten me ? In conjunction with linuxtesting.org and The Linux Foundation, I've been enlisted to test and work on helping to test tools they use for reliability testing of linux-based systems. In particular, two tools, RaceHound (whose command is 'lines2insns' and which isolates race conditions in kernel modules) and Klever, which is browser-based, are under continual development by the Center and I aim to help them improve their throughput by aiding in investigating where, in the automated nature particularly of Klever (requiring considerable configuration as well), there may areas to improve. Hence, yes, a large amount of our findings result in manifesting as theoretical and possible only, but relevant to improving the tools nonetheless. Hope that helps with the 'enlightening' :), regards, Mark
Re: [PATCH] media input infrastructure:tw686x: Fix of possibleinconsistent memory deallocation and/or race condition by implementation of custom video_device_release function in tw686x driver
Hi Hans, all, Sorry for the poor patching, I am a student and as you may tell still new to this system. At the time of the patching, I wasn't fully informed of all the requirements that go into such things, and am still learning. Would it be alright if I submit a report instead? In order to, I am (still, sorry) trying to understand the issue at hand. How in fact may the release() callback be overridden (by a tw686x-specific function) to free the dma memory and call video_device_release()? To my understanding at the time, this was merely a re-implementation of video_device_release with said requirements and subtraction of extra features from tw686x_video_free().. This release() callback is called by the V4L2 framework when the last user of the device closes its filehandle, so that's a good point to free all the memory. Doing it earlier (as the current code does) runs the risk that someone might still access that memory, and you don't want that. Yes, I definitely don't want that. :) Thank you, Mark
Re: [PATCH] watchdog:alim1535_wdt: Fix data race in ali_settimer() concerning ali_timeout_bits variable.
Hi Guenter, all, It's alright if you still don't wish to review my patch on alim1535_wdt, but my employer and I, using our race condition analysis tool, detected it to contain a race condition warning. I believe any possible issues could be resolved if it were rewritten to use the watchdog subsystem as you've previously stated. Now, I don't wish to bother you too much, but it seems I forgot to work mainly with my assigned mentor prior to submitting patches..sorry. So, after I have worked on rewriting the alim1535 driver into common watchdog subsystem with my mentor, may I submit it to you then? Thank you, Mark
[PATCH v4] watchdog: pc87413: Rewriting of pc87413_wdt driver to use watchdog subsystem
This patch rewrites the pc87413_wdt driver to use the watchdog subsystem. In
doing so, it also addresses a potential race condition owing from the
swc_base_addr variable being used before being set.
Signed-off-by: Mark Balantzyan
---
drivers/watchdog/Kconfig | 1 +
drivers/watchdog/pc87413_wdt.c | 346 ++---
2 files changed, 60 insertions(+), 287 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9af07fd9..84a7326d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1166,6 +1166,7 @@ config SCx200_WDT
config PC87413_WDT
tristate "NS PC87413 watchdog"
+ select WATCHDOG_CORE
depends on X86
---help---
This is the driver for the hardware watchdog on the PC87413 chipset
diff --git a/drivers/watchdog/pc87413_wdt.c b/drivers/watchdog/pc87413_wdt.c
index 06a892e3..1aa39c43 100644
--- a/drivers/watchdog/pc87413_wdt.c
+++ b/drivers/watchdog/pc87413_wdt.c
@@ -22,15 +22,9 @@
#include
#include
-#include
#include
#include
#include
-#include
-#include
-#include
-#include
-#include
#include
#include
#include
@@ -59,14 +53,9 @@ static int io = IO_DEFAULT;
static int swc_base_addr = -1;
static int timeout = DEFAULT_TIMEOUT; /* timeout value */
-static unsigned long timer_enabled;/* is the timer enabled? */
-
-static char expect_close; /* is the close expected? */
-
-static DEFINE_SPINLOCK(io_lock); /* to guard us from io races */
-
static bool nowayout = WATCHDOG_NOWAYOUT;
/* -- Low level function */
/* Select pins for Watchdog output */
@@ -149,8 +138,6 @@ static inline void pc87413_swc_bank3(void)
#endif
}
-/* Set watchdog timeout to x minutes */
-
static inline void pc87413_programm_wdto(char pc87413_time)
{
/* Step 5: Programm WDTO, Twd. */
@@ -160,6 +147,19 @@ static inline void pc87413_programm_wdto(char pc87413_time)
#endif
}
+/* Set watchdog timeout to x minutes */
+
+static int pc87413_set_timeout(struct watchdog_device *wdd, unsigned int t)
+{
+ /* Step 5: Program watchdog timeout */
+
+ wdd->min_timeout = 1;
+ wdd->max_timeout = 60;
+
+ wdd->timeout = t;
+ return 0;
+}
+
/* Enable WDEN */
static inline void pc87413_enable_wden(void)
@@ -216,281 +216,62 @@ static inline void pc87413_disable_sw_wd_trg(void)
/* -- Higher level functions */
-/* Enable the watchdog */
+/* Enable/start the watchdog */
-static void pc87413_enable(void)
+static int pc87413_start(struct watchdog_device *wdd)
{
- spin_lock(&io_lock);
-
pc87413_swc_bank3();
- pc87413_programm_wdto(timeout);
+ pc87413_set_timeout(wdd, wdd->timeout);
pc87413_enable_wden();
pc87413_enable_sw_wd_tren();
pc87413_enable_sw_wd_trg();
-
- spin_unlock(&io_lock);
+ return 0;
}
-/* Disable the watchdog */
+/* Disable/stop the watchdog */
-static void pc87413_disable(void)
+static int pc87413_stop(struct watchdog_device *wdd)
{
- spin_lock(&io_lock);
-
pc87413_swc_bank3();
pc87413_disable_sw_wd_tren();
pc87413_disable_sw_wd_trg();
pc87413_programm_wdto(0);
-
- spin_unlock(&io_lock);
+ pc87413_set_timeout(wdd, wdd->timeout);
+ return 0;
}
-/* Refresh the watchdog */
+/* Refresh/keepalive the watchdog */
-static void pc87413_refresh(void)
+static int pc87413_keepalive(struct watchdog_device *wdd)
{
- spin_lock(&io_lock);
-
pc87413_swc_bank3();
pc87413_disable_sw_wd_tren();
pc87413_disable_sw_wd_trg();
- pc87413_programm_wdto(timeout);
+ pc87413_set_timeout(wdd, wdd->timeout);
pc87413_enable_wden();
pc87413_enable_sw_wd_tren();
pc87413_enable_sw_wd_trg();
-
- spin_unlock(&io_lock);
-}
-
-/* -- File operations ---*/
-
-/**
- * pc87413_open:
- * @inode: inode of device
- * @file: file handle to device
- *
- */
-
-static int pc87413_open(struct inode *inode, struct file *file)
-{
- /* /dev/watchdog can only be opened once */
-
- if (test_and_set_bit(0, &timer_enabled))
- return -EBUSY;
-
- if (nowayout)
- __module_get(THIS_MODULE);
-
- /* Reload and activate timer */
- pc87413_refresh();
-
- pr_info("Watchdog enabled. Timeout set to %d minute(s).\n", timeout);
-
- return nonseekable_open(inode, file);
-}
-
-/**
- * pc87413_release:
- * @inode: inode to board
- * @file: file handle to board
- *
- * The watchdog has a configurable API. There is a religious dispute
- * between people who want their watchdog to be able to shut down and
- * those who want to be sure if the watchdog manager dies the machine
- * reboots. In the former case we
[PATCH v4] watchdog: pc87413: Rewriting of pc87413_wdt driver to use watchdog subsystem
This patch rewrites the pc87413_wdt driver to use the watchdog subsystem. In
doing so, it also addresses a potential race condition owing from the
swc_base_addr variable being used before being set.
Signed-off-by: Mark Balantzyan
---
drivers/watchdog/Kconfig | 1 +
drivers/watchdog/pc87413_wdt.c | 346 ++---
2 files changed, 60 insertions(+), 287 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9af07fd9..84a7326d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1166,6 +1166,7 @@ config SCx200_WDT
config PC87413_WDT
tristate "NS PC87413 watchdog"
+ select WATCHDOG_CORE
depends on X86
---help---
This is the driver for the hardware watchdog on the PC87413 chipset
diff --git a/drivers/watchdog/pc87413_wdt.c b/drivers/watchdog/pc87413_wdt.c
index 06a892e3..1aa39c43 100644
--- a/drivers/watchdog/pc87413_wdt.c
+++ b/drivers/watchdog/pc87413_wdt.c
@@ -22,15 +22,9 @@
#include
#include
-#include
#include
#include
#include
-#include
-#include
-#include
-#include
-#include
#include
#include
#include
@@ -59,14 +53,9 @@ static int io = IO_DEFAULT;
static int swc_base_addr = -1;
static int timeout = DEFAULT_TIMEOUT; /* timeout value */
-static unsigned long timer_enabled;/* is the timer enabled? */
-
-static char expect_close; /* is the close expected? */
-
-static DEFINE_SPINLOCK(io_lock); /* to guard us from io races */
-
static bool nowayout = WATCHDOG_NOWAYOUT;
+
/* -- Low level function */
/* Select pins for Watchdog output */
@@ -149,8 +138,6 @@ static inline void pc87413_swc_bank3(void)
#endif
}
-/* Set watchdog timeout to x minutes */
-
static inline void pc87413_programm_wdto(char pc87413_time)
{
/* Step 5: Programm WDTO, Twd. */
@@ -160,6 +147,19 @@ static inline void pc87413_programm_wdto(char pc87413_time)
#endif
}
+/* Set watchdog timeout to x minutes */
+
+static int pc87413_set_timeout(struct watchdog_device *wdd, unsigned int t)
+{
+ /* Step 5: Programm watchdog timeout */
+
+ wdd->min_timeout = 1;
+ wdd->max_timeout = 60;
+
+ wdd->timeout = t;
+ return 0;
+}
+
/* Enable WDEN */
static inline void pc87413_enable_wden(void)
@@ -216,281 +216,62 @@ static inline void pc87413_disable_sw_wd_trg(void)
/* -- Higher level functions */
-/* Enable the watchdog */
+/* Enable/start the watchdog */
-static void pc87413_enable(void)
+static int pc87413_start(struct watchdog_device *wdd)
{
- spin_lock(&io_lock);
-
pc87413_swc_bank3();
- pc87413_programm_wdto(timeout);
+ pc87413_set_timeout(wdd,wdd->timeout);
pc87413_enable_wden();
pc87413_enable_sw_wd_tren();
pc87413_enable_sw_wd_trg();
-
- spin_unlock(&io_lock);
+ return 0;
}
-/* Disable the watchdog */
+/* Disable/stop the watchdog */
-static void pc87413_disable(void)
+static int pc87413_stop(struct watchdog_device *wdd)
{
- spin_lock(&io_lock);
-
pc87413_swc_bank3();
pc87413_disable_sw_wd_tren();
pc87413_disable_sw_wd_trg();
pc87413_programm_wdto(0);
-
- spin_unlock(&io_lock);
+ pc87413_set_timeout(wdd,wdd->timeout);
+ return 0;
}
-/* Refresh the watchdog */
+/* Refresh/keepalive the watchdog */
-static void pc87413_refresh(void)
+static int pc87413_keepalive(struct watchdog_device *wdd)
{
- spin_lock(&io_lock);
-
pc87413_swc_bank3();
pc87413_disable_sw_wd_tren();
pc87413_disable_sw_wd_trg();
- pc87413_programm_wdto(timeout);
+ pc87413_set_timeout(wdd,wdd->timeout);
pc87413_enable_wden();
pc87413_enable_sw_wd_tren();
pc87413_enable_sw_wd_trg();
-
- spin_unlock(&io_lock);
-}
-
-/* -- File operations ---*/
-
-/**
- * pc87413_open:
- * @inode: inode of device
- * @file: file handle to device
- *
- */
-
-static int pc87413_open(struct inode *inode, struct file *file)
-{
- /* /dev/watchdog can only be opened once */
-
- if (test_and_set_bit(0, &timer_enabled))
- return -EBUSY;
-
- if (nowayout)
- __module_get(THIS_MODULE);
-
- /* Reload and activate timer */
- pc87413_refresh();
-
- pr_info("Watchdog enabled. Timeout set to %d minute(s).\n", timeout);
-
- return nonseekable_open(inode, file);
-}
-
-/**
- * pc87413_release:
- * @inode: inode to board
- * @file: file handle to board
- *
- * The watchdog has a configurable API. There is a religious dispute
- * between people who want their watchdog to be able to shut down and
- * those who want to be sure if the watchdog manager dies the machine
- * reboots. In the former
Re: [PATCH] watchdog:alim1535_wdt: Fix data race in ali_settimer() concerning ali_timeout_bits variable.
My employer (and yes, I am working for the Linux Foundation) has me working on analysing race condition warnings in the Linux kernel. They have a driver verification project running under the umbrella of the ELISA project involved in the research, investigation, experimentation, and establishment of linux kernel verification measures and tools. I actually do have assigned to me coaches and/or mentors that I have been corresponding with. They are aware of what is going on and are being cc'd to (most of) our emails. pc87413_wdt was detected by our race condition analysis tool as having warning. Even outside this work we've been doing, I've been trying to apply the reasoning of the race condition analysis tool to different kernel modules, as part of my menteeship. I hope you can respect that this is a process primarily for learning and experimentation. I'm sorry if I'm creating too much work for you at once. If so, let me know and I'll try to spread it out. Thank you, Mark
Re: [PATCH] watchdog:alim1535_wdt: Fix data race in ali_settimer() concerning ali_timeout_bits variable.
Hi Guenter, If it's not too much too ask, I also propose to rewrite alim1535_wdt to use the watchdog subsystem as I believe we are making progress toward the similar end in pc87413_wdt, as my evaluation ends in some weeks. Thank you, Mark
[PATCH v3] watchdog: pc87413: Rewriting of pc87413_wdt driver to use watchdog subsystem
This patch rewrites the pc87413_wdt driver to use the watchdog subsystem. In
doing so, it also addresses a potential race condition owing from the
swc_base_addr variable being used before being set.
Signed-off-by: Mark Balantzyan
---
drivers/watchdog/Kconfig | 1 +
drivers/watchdog/pc87413_wdt.c | 333 +
2 files changed, 47 insertions(+), 287 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9af07fd9..84a7326d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1166,6 +1166,7 @@ config SCx200_WDT
config PC87413_WDT
tristate "NS PC87413 watchdog"
+ select WATCHDOG_CORE
depends on X86
---help---
This is the driver for the hardware watchdog on the PC87413 chipset
diff --git a/drivers/watchdog/pc87413_wdt.c b/drivers/watchdog/pc87413_wdt.c
index 06a892e3..19d29cd8 100644
--- a/drivers/watchdog/pc87413_wdt.c
+++ b/drivers/watchdog/pc87413_wdt.c
@@ -22,15 +22,9 @@
#include
#include
-#include
#include
#include
#include
-#include
-#include
-#include
-#include
-#include
#include
#include
#include
@@ -60,13 +54,9 @@ static int swc_base_addr = -1;
static int timeout = DEFAULT_TIMEOUT; /* timeout value */
static unsigned long timer_enabled;/* is the timer enabled? */
-
-static char expect_close; /* is the close expected? */
-
-static DEFINE_SPINLOCK(io_lock); /* to guard us from io races */
-
static bool nowayout = WATCHDOG_NOWAYOUT;
+
/* -- Low level function */
/* Select pins for Watchdog output */
@@ -151,13 +141,15 @@ static inline void pc87413_swc_bank3(void)
/* Set watchdog timeout to x minutes */
-static inline void pc87413_programm_wdto(char pc87413_time)
+static int pc87413_set_timeout(struct watchdog_device *wdd, unsigned int t)
{
- /* Step 5: Programm WDTO, Twd. */
- outb_p(pc87413_time, swc_base_addr + WDTO);
-#ifdef DEBUG
- pr_info(DPFX "Set WDTO to %d minutes\n", pc87413_time);
-#endif
+ /* Step 5: Program watchdog timeout */
+
+ if (t < 1 || t > 60)/* arbitrary upper limit */
+ return -EINVAL;
+
+ timeout = t;
+ return 0;
}
/* Enable WDEN */
@@ -216,281 +208,61 @@ static inline void pc87413_disable_sw_wd_trg(void)
/* -- Higher level functions */
-/* Enable the watchdog */
+/* Enable/start the watchdog */
-static void pc87413_enable(void)
+static int pc87413_start(struct watchdog_device *wdd)
{
- spin_lock(&io_lock);
-
pc87413_swc_bank3();
- pc87413_programm_wdto(timeout);
+ pc87413_set_timeout(wdd, timeout);
pc87413_enable_wden();
pc87413_enable_sw_wd_tren();
pc87413_enable_sw_wd_trg();
-
- spin_unlock(&io_lock);
+ return 0;
}
-/* Disable the watchdog */
+/* Disable/stop the watchdog */
-static void pc87413_disable(void)
+static int pc87413_stop(struct watchdog_device *wdd)
{
- spin_lock(&io_lock);
-
pc87413_swc_bank3();
pc87413_disable_sw_wd_tren();
pc87413_disable_sw_wd_trg();
- pc87413_programm_wdto(0);
-
- spin_unlock(&io_lock);
+ pc87413_set_timeout(wdd, 0);
+ return 0;
}
-/* Refresh the watchdog */
+/* Refresh/keepalive the watchdog */
-static void pc87413_refresh(void)
+static int pc87413_keepalive(struct watchdog_device *wdd)
{
- spin_lock(&io_lock);
-
pc87413_swc_bank3();
pc87413_disable_sw_wd_tren();
pc87413_disable_sw_wd_trg();
- pc87413_programm_wdto(timeout);
+ pc87413_set_timeout(wdd, timeout);
pc87413_enable_wden();
pc87413_enable_sw_wd_tren();
pc87413_enable_sw_wd_trg();
-
- spin_unlock(&io_lock);
-}
-
-/* -- File operations ---*/
-
-/**
- * pc87413_open:
- * @inode: inode of device
- * @file: file handle to device
- *
- */
-
-static int pc87413_open(struct inode *inode, struct file *file)
-{
- /* /dev/watchdog can only be opened once */
-
- if (test_and_set_bit(0, &timer_enabled))
- return -EBUSY;
-
- if (nowayout)
- __module_get(THIS_MODULE);
-
- /* Reload and activate timer */
- pc87413_refresh();
-
- pr_info("Watchdog enabled. Timeout set to %d minute(s).\n", timeout);
-
- return nonseekable_open(inode, file);
-}
-
-/**
- * pc87413_release:
- * @inode: inode to board
- * @file: file handle to board
- *
- * The watchdog has a configurable API. There is a religious dispute
- * between people who want their watchdog to be able to shut down and
- * those who want to be sure if the watchdog manager dies the machine
- * reboots. In the former case we disable the counters, in the latter
- * case you have to open it again very soon
[PATCH v2] watchdog: pc87413: Rewriting of pc87413_wdt driver to use watchdog subsystem
This patch rewrites the pc87413_wdt driver to use the watchdog subsystem. In
doing so, it also addresses a potential race condition owing from the
swc_base_addr variable being used before being set.
Signed-off-by: Mark Balantzyan
---
drivers/watchdog/Kconfig | 1 +
drivers/watchdog/pc87413_wdt.c | 333 +
2 files changed, 47 insertions(+), 287 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9af07fd9..84a7326d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1166,6 +1166,7 @@ config SCx200_WDT
config PC87413_WDT
tristate "NS PC87413 watchdog"
+ select WATCHDOG_CORE
depends on X86
---help---
This is the driver for the hardware watchdog on the PC87413 chipset
diff --git a/drivers/watchdog/pc87413_wdt.c b/drivers/watchdog/pc87413_wdt.c
index 06a892e3..19d29cd8 100644
--- a/drivers/watchdog/pc87413_wdt.c
+++ b/drivers/watchdog/pc87413_wdt.c
@@ -22,15 +22,9 @@
#include
#include
-#include
#include
#include
#include
-#include
-#include
-#include
-#include
-#include
#include
#include
#include
@@ -60,13 +54,9 @@ static int swc_base_addr = -1;
static int timeout = DEFAULT_TIMEOUT; /* timeout value */
static unsigned long timer_enabled;/* is the timer enabled? */
-
-static char expect_close; /* is the close expected? */
-
-static DEFINE_SPINLOCK(io_lock); /* to guard us from io races */
-
static bool nowayout = WATCHDOG_NOWAYOUT;
+
/* -- Low level function */
/* Select pins for Watchdog output */
@@ -151,13 +141,15 @@ static inline void pc87413_swc_bank3(void)
/* Set watchdog timeout to x minutes */
-static inline void pc87413_programm_wdto(char pc87413_time)
+static int pc87413_set_timeout(struct watchdog_device *wdd, unsigned int t)
{
- /* Step 5: Programm WDTO, Twd. */
- outb_p(pc87413_time, swc_base_addr + WDTO);
-#ifdef DEBUG
- pr_info(DPFX "Set WDTO to %d minutes\n", pc87413_time);
-#endif
+ /* Step 5: Programm watchdog timeout */
+
+ if ((t < 1) || ( t > 60))/* arbitrary upper limit */
+ return -EINVAL;
+
+ timeout = t;
+ return 0;
}
/* Enable WDEN */
@@ -216,281 +208,61 @@ static inline void pc87413_disable_sw_wd_trg(void)
/* -- Higher level functions */
-/* Enable the watchdog */
+/* Enable/start the watchdog */
-static void pc87413_enable(void)
+static int pc87413_start(struct watchdog_device *wdd)
{
- spin_lock(&io_lock);
-
pc87413_swc_bank3();
- pc87413_programm_wdto(timeout);
+ pc87413_set_timeout(wdd,timeout);
pc87413_enable_wden();
pc87413_enable_sw_wd_tren();
pc87413_enable_sw_wd_trg();
-
- spin_unlock(&io_lock);
+ return 0;
}
-/* Disable the watchdog */
+/* Disable/stop the watchdog */
-static void pc87413_disable(void)
+static int pc87413_stop(struct watchdog_device *wdd)
{
- spin_lock(&io_lock);
-
pc87413_swc_bank3();
pc87413_disable_sw_wd_tren();
pc87413_disable_sw_wd_trg();
- pc87413_programm_wdto(0);
-
- spin_unlock(&io_lock);
+ pc87413_set_timeout(wdd,0);
+ return 0;
}
-/* Refresh the watchdog */
+/* Refresh/keepalive the watchdog */
-static void pc87413_refresh(void)
+static int pc87413_keepalive(struct watchdog_device *wdd)
{
- spin_lock(&io_lock);
-
pc87413_swc_bank3();
pc87413_disable_sw_wd_tren();
pc87413_disable_sw_wd_trg();
- pc87413_programm_wdto(timeout);
+ pc87413_set_timeout(wdd,timeout);
pc87413_enable_wden();
pc87413_enable_sw_wd_tren();
pc87413_enable_sw_wd_trg();
-
- spin_unlock(&io_lock);
-}
-
-/* -- File operations ---*/
-
-/**
- * pc87413_open:
- * @inode: inode of device
- * @file: file handle to device
- *
- */
-
-static int pc87413_open(struct inode *inode, struct file *file)
-{
- /* /dev/watchdog can only be opened once */
-
- if (test_and_set_bit(0, &timer_enabled))
- return -EBUSY;
-
- if (nowayout)
- __module_get(THIS_MODULE);
-
- /* Reload and activate timer */
- pc87413_refresh();
-
- pr_info("Watchdog enabled. Timeout set to %d minute(s).\n", timeout);
-
- return nonseekable_open(inode, file);
-}
-
-/**
- * pc87413_release:
- * @inode: inode to board
- * @file: file handle to board
- *
- * The watchdog has a configurable API. There is a religious dispute
- * between people who want their watchdog to be able to shut down and
- * those who want to be sure if the watchdog manager dies the machine
- * reboots. In the former case we disable the counters, in the latter
- * case you have to open it again v
Re: [PATCH] watchdog: pc87413: Rewriting of pc87413_wdt driver to use watchdog subsystem
Hi all, Guenter, Thank you for your email. Unfortunately, on my end, the indentation is straight and perhaps through protocol transfer there was stray modification. I've made the other changes as indicated that I'll submit in a v2 patch shortly. Is 'v2' permissible to include in the title in this case? Not sure, but this will BE a modification...in that case I should have been v-ing my patches since. No, the compiler did not complain about no 'return ret' as part of the label code block. The function does 'return 0;' at the end, by default. Thanks + regards, Mark
[PATCH] watchdog: pc87413: Rewriting of pc87413_wdt driver to use watchdog subsystem
This patch rewrites the pc87413_wdt driver to use the watchdog subsystem. In
doing so, it also addresses a potential race condition owing from the
swc_base_addr variable being used before being set.
Signed-off-by: Mark Balantzyan
---
drivers/watchdog/Kconfig | 1 +
drivers/watchdog/pc87413_wdt.c | 323 -
2 files changed, 40 insertions(+), 284 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9af07fd9..84a7326d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1166,6 +1166,7 @@ config SCx200_WDT
config PC87413_WDT
tristate "NS PC87413 watchdog"
+ select WATCHDOG_CORE
depends on X86
---help---
This is the driver for the hardware watchdog on the PC87413 chipset
diff --git a/drivers/watchdog/pc87413_wdt.c b/drivers/watchdog/pc87413_wdt.c
index 06a892e3..60e3cda6 100644
--- a/drivers/watchdog/pc87413_wdt.c
+++ b/drivers/watchdog/pc87413_wdt.c
@@ -22,15 +22,9 @@
#include
#include
-#include
#include
#include
#include
-#include
-#include
-#include
-#include
-#include
#include
#include
#include
@@ -60,13 +54,9 @@ static int swc_base_addr = -1;
static int timeout = DEFAULT_TIMEOUT; /* timeout value */
static unsigned long timer_enabled;/* is the timer enabled? */
-
-static char expect_close; /* is the close expected? */
-
-static DEFINE_SPINLOCK(io_lock); /* to guard us from io races */
-
static bool nowayout = WATCHDOG_NOWAYOUT;
+
/* -- Low level function */
/* Select pins for Watchdog output */
@@ -151,13 +141,15 @@ static inline void pc87413_swc_bank3(void)
/* Set watchdog timeout to x minutes */
-static inline void pc87413_programm_wdto(char pc87413_time)
+static int pc87413_set_timeout(struct watchdog_device *wdd, unsigned int t)
{
- /* Step 5: Programm WDTO, Twd. */
- outb_p(pc87413_time, swc_base_addr + WDTO);
-#ifdef DEBUG
- pr_info(DPFX "Set WDTO to %d minutes\n", pc87413_time);
-#endif
+ /* Step 5: Programm watchdog timeout */
+
+ if ((t < 1) || ( t > 60))/* arbitrary upper limit */
+ return -EINVAL;
+
+ timeout = t;
+ return 0;
}
/* Enable WDEN */
@@ -216,281 +208,61 @@ static inline void pc87413_disable_sw_wd_trg(void)
/* -- Higher level functions */
-/* Enable the watchdog */
+/* Enable/start the watchdog */
-static void pc87413_enable(void)
+static int pc87413_start(struct watchdog_device *wdd)
{
- spin_lock(&io_lock);
-
pc87413_swc_bank3();
- pc87413_programm_wdto(timeout);
+ pc87413_set_timeout(wdd,timeout);
pc87413_enable_wden();
pc87413_enable_sw_wd_tren();
pc87413_enable_sw_wd_trg();
-
- spin_unlock(&io_lock);
+ return 0;
}
-/* Disable the watchdog */
+/* Disable/stop the watchdog */
-static void pc87413_disable(void)
+static int pc87413_stop(struct watchdog_device *wdd)
{
- spin_lock(&io_lock);
-
pc87413_swc_bank3();
pc87413_disable_sw_wd_tren();
pc87413_disable_sw_wd_trg();
- pc87413_programm_wdto(0);
-
- spin_unlock(&io_lock);
+ pc87413_set_timeout(wdd,0);
+ return 0;
}
-/* Refresh the watchdog */
+/* Refresh/keepalive the watchdog */
-static void pc87413_refresh(void)
+static int pc87413_keepalive(struct watchdog_device *wdd)
{
- spin_lock(&io_lock);
-
pc87413_swc_bank3();
pc87413_disable_sw_wd_tren();
pc87413_disable_sw_wd_trg();
- pc87413_programm_wdto(timeout);
+ pc87413_set_timeout(wdd,timeout);
pc87413_enable_wden();
pc87413_enable_sw_wd_tren();
pc87413_enable_sw_wd_trg();
-
- spin_unlock(&io_lock);
-}
-
-/* -- File operations ---*/
-
-/**
- * pc87413_open:
- * @inode: inode of device
- * @file: file handle to device
- *
- */
-
-static int pc87413_open(struct inode *inode, struct file *file)
-{
- /* /dev/watchdog can only be opened once */
-
- if (test_and_set_bit(0, &timer_enabled))
- return -EBUSY;
-
- if (nowayout)
- __module_get(THIS_MODULE);
-
- /* Reload and activate timer */
- pc87413_refresh();
-
- pr_info("Watchdog enabled. Timeout set to %d minute(s).\n", timeout);
-
- return nonseekable_open(inode, file);
-}
-
-/**
- * pc87413_release:
- * @inode: inode to board
- * @file: file handle to board
- *
- * The watchdog has a configurable API. There is a religious dispute
- * between people who want their watchdog to be able to shut down and
- * those who want to be sure if the watchdog manager dies the machine
- * reboots. In the former case we disable the counters, in the latter
- * case you have to open it again v
[PATCH] Revision of pc87413_wdt driver to use watchdog subsystem
This patch rewrites the pc87413_wdt driver to use the watchdog subsystem. In
doing so, it also addresses a potential race condition owing from the
swc_base_addr variable being used before being set.
Signed-off-by: Mark Balantzyan
---
drivers/watchdog/Kconfig | 1 +
drivers/watchdog/pc87413_wdt.c | 294 +
2 files changed, 39 insertions(+), 256 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9af07fd9..84a7326d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1166,6 +1166,7 @@ config SCx200_WDT
config PC87413_WDT
tristate "NS PC87413 watchdog"
+ select WATCHDOG_CORE
depends on X86
---help---
This is the driver for the hardware watchdog on the PC87413 chipset
diff --git a/drivers/watchdog/pc87413_wdt.c b/drivers/watchdog/pc87413_wdt.c
index 06a892e3..d1d32771 100644
--- a/drivers/watchdog/pc87413_wdt.c
+++ b/drivers/watchdog/pc87413_wdt.c
@@ -22,12 +22,10 @@
#include
#include
-#include
#include
#include
#include
#include
-#include
#include
#include
#include
@@ -65,7 +63,6 @@ static char expect_close; /* is the close
expected? */
static DEFINE_SPINLOCK(io_lock); /* to guard us from io races */
-static bool nowayout = WATCHDOG_NOWAYOUT;
/* -- Low level function */
@@ -216,41 +213,32 @@ static inline void pc87413_disable_sw_wd_trg(void)
/* -- Higher level functions */
-/* Enable the watchdog */
+/* Enable/start the watchdog */
-static void pc87413_enable(void)
+static void pc87413_start(void)
{
- spin_lock(&io_lock);
-
pc87413_swc_bank3();
pc87413_programm_wdto(timeout);
pc87413_enable_wden();
pc87413_enable_sw_wd_tren();
pc87413_enable_sw_wd_trg();
- spin_unlock(&io_lock);
}
-/* Disable the watchdog */
+/* Disable/stop the watchdog */
-static void pc87413_disable(void)
+static void pc87413_stop(void)
{
- spin_lock(&io_lock);
-
pc87413_swc_bank3();
pc87413_disable_sw_wd_tren();
pc87413_disable_sw_wd_trg();
pc87413_programm_wdto(0);
-
- spin_unlock(&io_lock);
}
-/* Refresh the watchdog */
+/* Refresh/keepalive the watchdog */
-static void pc87413_refresh(void)
+static void pc87413_keepalive(struct watchdog_device *wdd)
{
- spin_lock(&io_lock);
-
pc87413_swc_bank3();
pc87413_disable_sw_wd_tren();
pc87413_disable_sw_wd_trg();
@@ -258,195 +246,11 @@ static void pc87413_refresh(void)
pc87413_enable_wden();
pc87413_enable_sw_wd_tren();
pc87413_enable_sw_wd_trg();
-
- spin_unlock(&io_lock);
-}
-
-/* -- File operations ---*/
-
-/**
- * pc87413_open:
- * @inode: inode of device
- * @file: file handle to device
- *
- */
-
-static int pc87413_open(struct inode *inode, struct file *file)
-{
- /* /dev/watchdog can only be opened once */
-
- if (test_and_set_bit(0, &timer_enabled))
- return -EBUSY;
-
- if (nowayout)
- __module_get(THIS_MODULE);
-
- /* Reload and activate timer */
- pc87413_refresh();
-
- pr_info("Watchdog enabled. Timeout set to %d minute(s).\n", timeout);
-
- return nonseekable_open(inode, file);
-}
-
-/**
- * pc87413_release:
- * @inode: inode to board
- * @file: file handle to board
- *
- * The watchdog has a configurable API. There is a religious dispute
- * between people who want their watchdog to be able to shut down and
- * those who want to be sure if the watchdog manager dies the machine
- * reboots. In the former case we disable the counters, in the latter
- * case you have to open it again very soon.
- */
-
-static int pc87413_release(struct inode *inode, struct file *file)
-{
- /* Shut off the timer. */
-
- if (expect_close == 42) {
- pc87413_disable();
- pr_info("Watchdog disabled, sleeping again...\n");
- } else {
- pr_crit("Unexpected close, not stopping watchdog!\n");
- pc87413_refresh();
- }
- clear_bit(0, &timer_enabled);
- expect_close = 0;
return 0;
}
-/**
- * pc87413_status:
- *
- * return, if the watchdog is enabled (timeout is set...)
- */
-
-
-static int pc87413_status(void)
-{
- return 0; /* currently not supported */
-}
-
-/**
- * pc87413_write:
- * @file: file handle to the watchdog
- * @data: data buffer to write
- * @len: length in bytes
- * @ppos: pointer to the position to write. No seeks allowed
- *
- * A write to a watchdog device is defined as a keepalive signal. Any
- * write of data will do, as we we don't define content meaning.
-
Re: [PATCH 1/4] watchdog device drivers:pc87413_wdt:Rewriting of pc87413_wdt driver to utilize common watchdog interface
Yes, each time I go through an edit I discover something I missed, apologies. Will be working on it for a bit, currently getting compile time errors. Just letting you know and it’s good we’re checking :-) Thank you, Mark
Re: [PATCH] watchdog device drivers:pc87413_wdt: Rewriting of pc87413_wdt driver to utilize common watchdog interface (fwd)
Hi all, Guenter, Sure, it'd be great to work on ib700, doing both, if we may. I feel it's worth a shot in case somebody out there has the hardware to test the pc87413_wdt driver, though I'm doing my best building the individual module and checking for compilation errors (as best I can). I just sent off via git send-email a quad-chain of patches for the driver. Thanks + regards, Mark
[PATCH 4/4] watchdog device drivers:pc87413_wdt: Continuing revision of conversion of pc87413_wdt to use common watchdog interface, removed undeclared identifiers
There is a potential for the variable swc_base_addr in the call chain of the
driver initialization function (init) to be used before initialization. This
brought up the need for, by rewriting the driver to use the common watchdog
interface, ensuring to have all resources in place. This patch addresses this
need by rewriting into common watchdog interface utilization for the driver.
Signed-off-by: Mark Balantzyan
---
drivers/watchdog/pc87413_wdt.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/watchdog/pc87413_wdt.c b/drivers/watchdog/pc87413_wdt.c
index bc6c4e19..6863145a 100644
--- a/drivers/watchdog/pc87413_wdt.c
+++ b/drivers/watchdog/pc87413_wdt.c
@@ -370,7 +370,6 @@ reboot_unreg:
static void __exit pc87413_exit(void)
{
watchdog_unregister_device(&pc87413wdt_wdd);
- unregister_reboot_notifier(&pc87413_notifier);
release_region(swc_base_addr, 0x20);
pr_info("watchdog component driver removed\n");
--
2.17.1
[PATCH 2/4] watchdog device drivers:pc87413_wdt:Rewriting of pc87413_wdt driver to utilize common watchdog interface, with removal of file access functions for correct functionality
There is a potential for the variable swc_base_addr in the call chain of the
driver initialization function (init) to be used before initialization. This
brought up the need for, by rewriting the driver to use the common watchdog
interface, ensuring to have all resources in place. This patch addresses this
need by rewriting into common watchdog interface utilization for the driver.
Signed-off-by: Mark Balantzyan
---
drivers/watchdog/pc87413_wdt.c | 235 +++--
1 file changed, 22 insertions(+), 213 deletions(-)
diff --git a/drivers/watchdog/pc87413_wdt.c b/drivers/watchdog/pc87413_wdt.c
index 4c330ee5..a9070a22 100644
--- a/drivers/watchdog/pc87413_wdt.c
+++ b/drivers/watchdog/pc87413_wdt.c
@@ -217,37 +217,28 @@ static inline void pc87413_disable_sw_wd_trg(void)
static void pc87413_start(void)
{
- spin_lock(&io_lock);
-
pc87413_swc_bank3();
pc87413_programm_wdto(timeout);
pc87413_enable_wden();
pc87413_enable_sw_wd_tren();
pc87413_enable_sw_wd_trg();
- spin_unlock(&io_lock);
}
/* Disable/stop the watchdog */
static void pc87413_stop(void)
{
- spin_lock(&io_lock);
-
pc87413_swc_bank3();
pc87413_disable_sw_wd_tren();
pc87413_disable_sw_wd_trg();
pc87413_programm_wdto(0);
-
- spin_unlock(&io_lock);
}
/* Refresh/keepalive the watchdog */
static void pc87413_keepalive(struct watchdog_device *wdd)
{
- spin_lock(&io_lock);
-
pc87413_swc_bank3();
pc87413_disable_sw_wd_tren();
pc87413_disable_sw_wd_trg();
@@ -255,192 +246,11 @@ static void pc87413_keepalive(struct watchdog_device
*wdd)
pc87413_enable_wden();
pc87413_enable_sw_wd_tren();
pc87413_enable_sw_wd_trg();
-
- spin_unlock(&io_lock);
-
return 0;
}
-/* -- File operations ---*/
-
-/**
- * pc87413_open:
- * @inode: inode of device
- * @file: file handle to device
- *
- */
-
-static int pc87413_open(struct inode *inode, struct file *file)
-{
- /* /dev/watchdog can only be opened once */
-
- if (test_and_set_bit(0, &timer_enabled))
- return -EBUSY;
- /* Reload and activate timer */
- pc87413_refresh();
-
- pr_info("Watchdog enabled. Timeout set to %d minute(s).\n", timeout);
-
- return nonseekable_open(inode, file);
-}
-
-/**
- * pc87413_release:
- * @inode: inode to board
- * @file: file handle to board
- *
- * The watchdog has a configurable API. There is a religious dispute
- * between people who want their watchdog to be able to shut down and
- * those who want to be sure if the watchdog manager dies the machine
- * reboots. In the former case we disable the counters, in the latter
- * case you have to open it again very soon.
- */
-
-static int pc87413_release(struct inode *inode, struct file *file)
-{
- /* Shut off the timer. */
-
- if (expect_close == 42) {
- pc87413_disable();
- pr_info("Watchdog disabled, sleeping again...\n");
- } else {
- pr_crit("Unexpected close, not stopping watchdog!\n");
- pc87413_refresh();
- }
- clear_bit(0, &timer_enabled);
- expect_close = 0;
- return 0;
-}
-
-/**
- * pc87413_status:
- *
- * return, if the watchdog is enabled (timeout is set...)
- */
-
-
-static int pc87413_status(void)
-{
- return 0; /* currently not supported */
-}
-
-/**
- * pc87413_write:
- * @data: data buffer to write
- * @len: length in bytes
- * @ppos: pointer to the position to write. No seeks allowed
- *
- * A write to a watchdog device is defined as a keepalive signal. Any
- * write of data will do, as we we don't define content meaning.
- */
-
-static ssize_t pc87413_write(struct file *file, const char __user *data,
-size_t len, loff_t *ppos)
-{
- /* See if we got the magic character 'V' and reload the timer */
- if (len) {
- size_t i;
-
- /* reset expect flag */
- expect_close = 0;
-
- /* scan to see whether or not we got the
- magic character */
- for (i = 0; i != len; i++) {
- char c;
- if (get_user(c, data + i))
- return -EFAULT;
- if (c == 'V')
- expect_close = 42;
- }
- }
-
- /* someone wrote to us, we should reload the timer */
- pc87413_refresh();
-
- return len;
-}
-
-/**
- * pc87413_ioctl:
- * @file: file handle to the device
- * @cmd: watchdog command
- * @arg: argument pointer
- *
- * The watchdog API defines a common set of functions for
[PATCH 1/4] watchdog device drivers:pc87413_wdt:Rewriting of pc87413_wdt driver to utilize common watchdog interface
There is a potential for the variable swc_base_addr in the call chain of the
driver initialization function (init) to be used before initialization. This
brought up the need for, by rewriting the driver to use the common watchdog
interface, ensuring to have all resources in place. This patch addresses this
need by rewriting into common watchdog interface utilization for the driver.
Signed-off-by: Mark Balantzyan
---
drivers/media/pci/tw686x/Kconfig | 1 +
drivers/watchdog/pc87413_wdt.c | 92 +++-
2 files changed, 45 insertions(+), 48 deletions(-)
diff --git a/drivers/media/pci/tw686x/Kconfig b/drivers/media/pci/tw686x/Kconfig
index da8bfee7..079d7464 100644
--- a/drivers/media/pci/tw686x/Kconfig
+++ b/drivers/media/pci/tw686x/Kconfig
@@ -5,6 +5,7 @@ config VIDEO_TW686X
select VIDEOBUF2_DMA_CONTIG
select VIDEOBUF2_DMA_SG
select SND_PCM
+ select WATCHDOG_CORE
help
Support for Intersil/Techwell TW686x-based frame grabber cards.
diff --git a/drivers/watchdog/pc87413_wdt.c b/drivers/watchdog/pc87413_wdt.c
index 06a892e3..4c330ee5 100644
--- a/drivers/watchdog/pc87413_wdt.c
+++ b/drivers/watchdog/pc87413_wdt.c
@@ -22,12 +22,10 @@
#include
#include
-#include
#include
#include
#include
#include
-#include
#include
#include
#include
@@ -65,7 +63,6 @@ static char expect_close; /* is the close
expected? */
static DEFINE_SPINLOCK(io_lock); /* to guard us from io races */
-static bool nowayout = WATCHDOG_NOWAYOUT;
/* -- Low level function */
@@ -216,9 +213,9 @@ static inline void pc87413_disable_sw_wd_trg(void)
/* -- Higher level functions */
-/* Enable the watchdog */
+/* Enable/start the watchdog */
-static void pc87413_enable(void)
+static void pc87413_start(void)
{
spin_lock(&io_lock);
@@ -231,9 +228,9 @@ static void pc87413_enable(void)
spin_unlock(&io_lock);
}
-/* Disable the watchdog */
+/* Disable/stop the watchdog */
-static void pc87413_disable(void)
+static void pc87413_stop(void)
{
spin_lock(&io_lock);
@@ -245,9 +242,9 @@ static void pc87413_disable(void)
spin_unlock(&io_lock);
}
-/* Refresh the watchdog */
+/* Refresh/keepalive the watchdog */
-static void pc87413_refresh(void)
+static void pc87413_keepalive(struct watchdog_device *wdd)
{
spin_lock(&io_lock);
@@ -260,6 +257,8 @@ static void pc87413_refresh(void)
pc87413_enable_sw_wd_trg();
spin_unlock(&io_lock);
+
+ return 0;
}
/* -- File operations ---*/
@@ -278,9 +277,6 @@ static int pc87413_open(struct inode *inode, struct file
*file)
if (test_and_set_bit(0, &timer_enabled))
return -EBUSY;
- if (nowayout)
- __module_get(THIS_MODULE);
-
/* Reload and activate timer */
pc87413_refresh();
@@ -331,7 +327,6 @@ static int pc87413_status(void)
/**
* pc87413_write:
- * @file: file handle to the watchdog
* @data: data buffer to write
* @len: length in bytes
* @ppos: pointer to the position to write. No seeks allowed
@@ -345,26 +340,25 @@ static ssize_t pc87413_write(struct file *file, const
char __user *data,
{
/* See if we got the magic character 'V' and reload the timer */
if (len) {
- if (!nowayout) {
- size_t i;
-
- /* reset expect flag */
- expect_close = 0;
-
- /* scan to see whether or not we got the
- magic character */
- for (i = 0; i != len; i++) {
- char c;
- if (get_user(c, data + i))
- return -EFAULT;
- if (c == 'V')
- expect_close = 42;
- }
+ size_t i;
+
+ /* reset expect flag */
+ expect_close = 0;
+
+ /* scan to see whether or not we got the
+ magic character */
+ for (i = 0; i != len; i++) {
+ char c;
+ if (get_user(c, data + i))
+ return -EFAULT;
+ if (c == 'V')
+ expect_close = 42;
}
+ }
/* someone wrote to us, we should reload the timer */
- pc87413_refresh();
- }
+ pc87413_refresh();
+
return len;
}
@@ -417,7 +411,7 @@ static long pc87413_ioctl(struct file *file, unsigned int
cmd,
retval = 0;
}
if (options & WDIOS_ENABLECARD) {
-
[PATCH 3/4] watchdog device drivers:pc87413_wdt: Tidying up conversion of pc87413_wdt driver to common watchdog interface, removal of some stray nowayout parameters
There is a potential for the variable swc_base_addr in the call chain of the driver initialization function (init) to be used before initialization. This brought up the need for, by rewriting the driver to use the common watchdog interface, ensuring to have all resources in place. This patch addresses this need by rewriting into common watchdog interface utilization for the driver. Signed-off-by: Mark Balantzyan --- drivers/watchdog/pc87413_wdt.c | 6 -- 1 file changed, 6 deletions(-) diff --git a/drivers/watchdog/pc87413_wdt.c b/drivers/watchdog/pc87413_wdt.c index a9070a22..bc6c4e19 100644 --- a/drivers/watchdog/pc87413_wdt.c +++ b/drivers/watchdog/pc87413_wdt.c @@ -392,9 +392,3 @@ module_param(timeout, int, 0); MODULE_PARM_DESC(timeout, "Watchdog timeout in minutes (default=" __MODULE_STRING(DEFAULT_TIMEOUT) ")."); - -module_param(nowayout, bool, 0); -MODULE_PARM_DESC(nowayout, - "Watchdog cannot be stopped once started (default=" - __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); - -- 2.17.1
Re: [PATCH] watchdog device drivers:pc87413_wdt: Rewriting of pc87413_wdt driver to utilize common watchdog interface (fwd)
Hello all, Guenter, I am being evaluated as a student by my organization. I appreciate your patience with my emails and patches. I would like to please propose that we divide and conquer: I write the code for converting the driver to common watchdog interface (and I thank you for your guidance on it in previous email) and you test the code on the actual hardware you may happen to have, as I do not have it. I accept the fact that it is indeed risky without the hardware to ensure the driver works correctly, but that will be where we work in tandem, software to hardware, yes, if that's alright? I think it's better if I use git send-email for the corresponding patch with the improvements you forecasted since it may format things better and may result in a non-corrupted patching. Thank you, Mark
Re: [PATCH] watchdog device drivers:pc87413_wdt: Rewriting of pc87413_wdt driver to utilize common watchdog interface (fwd)
-- Forwarded message -- Hi all, sorry for the duplicate message Guenter, wanted to be sure my message is transferred: Thank you for your reply, Guenter! Sorry there were issues applying the patch, I used git format-patch to produce the patch and pasted the main contents into a plaintext email client so I thought it would work.. May I please request clarification on which functions are no longer needed? Sorry about forgetting about that last misc_deregister(). Will do more tests, if that’s alright with you. In effect, may it be best to start the watchdog from the “init” function? Thank you, Mark
[PATCH] watchdog device drivers:pc87413_wdt: Rewriting of pc87413_wdt driver to utilize common watchdog interface
There is a potential for the variable swc_base_addr in the call chain of the driver initialization function (init) to be used before
initialization. This brought up the need for, by rewriting the driver to use the common watchdog interface, ensuring to have all resources in place. This patch addresses this need by rewriting into common watchdog interface utilization for the driver.
Signed-off-by: Mark Balantzyan
---
drivers/media/pci/tw686x/Kconfig | 1 +
drivers/watchdog/pc87413_wdt.c | 92 +++-
2 files changed, 45 insertions(+), 48 deletions(-)
diff --git a/drivers/media/pci/tw686x/Kconfig
b/drivers/media/pci/tw686x/Kconfig
index da8bfee7..079d7464 100644
--- a/drivers/media/pci/tw686x/Kconfig
+++ b/drivers/media/pci/tw686x/Kconfig
@@ -5,6 +5,7 @@ config VIDEO_TW686X
select VIDEOBUF2_DMA_CONTIG
select VIDEOBUF2_DMA_SG
select SND_PCM
+ select WATCHDOG_CORE
help
Support for Intersil/Techwell TW686x-based frame grabber cards.
diff --git a/drivers/watchdog/pc87413_wdt.c
b/drivers/watchdog/pc87413_wdt.c
index 06a892e3..4c330ee5 100644
--- a/drivers/watchdog/pc87413_wdt.c
+++ b/drivers/watchdog/pc87413_wdt.c
@@ -22,12 +22,10 @@
#include
#include
-#include
#include
#include
#include
#include
-#include
#include
#include
#include
@@ -65,7 +63,6 @@ static char expect_close; /* is the close
expected? */
static DEFINE_SPINLOCK(io_lock); /* to guard us from io races */
-static bool nowayout = WATCHDOG_NOWAYOUT;
/* -- Low level function */
@@ -216,9 +213,9 @@ static inline void pc87413_disable_sw_wd_trg(void)
/* -- Higher level functions */
-/* Enable the watchdog */
+/* Enable/start the watchdog */
-static void pc87413_enable(void)
+static void pc87413_start(void)
{
spin_lock(&io_lock);
@@ -231,9 +228,9 @@ static void pc87413_enable(void)
spin_unlock(&io_lock);
}
-/* Disable the watchdog */
+/* Disable/stop the watchdog */
-static void pc87413_disable(void)
+static void pc87413_stop(void)
{
spin_lock(&io_lock);
@@ -245,9 +242,9 @@ static void pc87413_disable(void)
spin_unlock(&io_lock);
}
-/* Refresh the watchdog */
+/* Refresh/keepalive the watchdog */
-static void pc87413_refresh(void)
+static void pc87413_keepalive(struct watchdog_device *wdd)
{
spin_lock(&io_lock);
@@ -260,6 +257,8 @@ static void pc87413_refresh(void)
pc87413_enable_sw_wd_trg();
spin_unlock(&io_lock);
+
+ return 0;
}
/* -- File operations ---*/
@@ -278,9 +277,6 @@ static int pc87413_open(struct inode *inode, struct
file *file)
if (test_and_set_bit(0, &timer_enabled))
return -EBUSY;
- if (nowayout)
- __module_get(THIS_MODULE);
-
/* Reload and activate timer */
pc87413_refresh();
@@ -331,7 +327,6 @@ static int pc87413_status(void)
/**
* pc87413_write:
- * @file: file handle to the watchdog
* @data: data buffer to write
* @len: length in bytes
* @ppos: pointer to the position to write. No seeks allowed
@@ -345,26 +340,25 @@ static ssize_t pc87413_write(struct file *file,
const char __user *data,
{
/* See if we got the magic character 'V' and reload the timer */
if (len) {
- if (!nowayout) {
- size_t i;
-
- /* reset expect flag */
- expect_close = 0;
-
- /* scan to see whether or not we got the
- magic character */
- for (i = 0; i != len; i++) {
- char c;
- if (get_user(c, data + i))
- return -EFAULT;
- if (c == 'V')
- expect_close = 42;
- }
+ size_t i;
+
+ /* reset expect flag */
+ expect_close = 0;
+
+ /* scan to see whether or not we got the
+ magic character */
+ for (i = 0; i != len; i++) {
+ char c;
+ if (get_user(c, data + i))
+ return -EFAULT;
+ if (c == 'V')
+ expect_close = 42;
}
+ }
/* someone wrote to us, we should reload the timer */
- pc87413_refresh();
- }
+ pc87413_refresh();
+
return len;
}
@@ -417,7 +411,7 @@ static long pc87413_ioctl(struct file *file, unsigned
int cmd,
retval = 0;
}
if (options & WDIOS_ENABLECARD) {
- pc87413_enable();
+
[PATCH] media input infrastructure:tw686x: Fix of possibleinconsistent memory deallocation and/or race condition by implementation of custom video_device_release function in tw686x driver
Possible inconsistent memory deallocation and/or race conditions were
detected specifically with respect to remaining open handles to the video
device handled by the tw686x driver. This patch addresses this by
implementing a revised independent instance of the video_device_release
function to free the remaining resources and memory where the last open
handle(s) is/were closed.
Signed-off-by: Mark Balantzyan
---
drivers/media/pci/tw686x/tw686x-video.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/media/pci/tw686x/tw686x-video.c
b/drivers/media/pci/tw686x/tw686x-video.c
index 3a06c000..29e10c85 100644
--- a/drivers/media/pci/tw686x/tw686x-video.c
+++ b/drivers/media/pci/tw686x/tw686x-video.c
@@ -1151,18 +1151,25 @@ void tw686x_video_irq(struct tw686x_dev *dev,
unsigned long requests,
}
}
+void video_device_release(tw686x_dev *dev) {
+ for (int pb = 0; pb < 2; pb++) {
+ dev->dma_ops->free(dev->video_channels,pb);
+ }
+ kfree(dev);
+}
+
void tw686x_video_free(struct tw686x_dev *dev)
{
- unsigned int ch, pb;
+ unsigned int ch;
for (ch = 0; ch < max_channels(dev); ch++) {
struct tw686x_video_channel *vc =
&dev->video_channels[ch];
video_unregister_device(vc->device);
- if (dev->dma_ops->free)
- for (pb = 0; pb < 2; pb++)
- dev->dma_ops->free(vc, pb);
+ if (dev->dma_ops->free) {
+ video_device_release(dev);
+ }
}
}
--
2.17.1
[PATCH] watchdog:alim1535_wdt: Fix data race in ali_settimer() concerning ali_timeout_bits variable. variable.
---
drivers/watchdog/alim1535_wdt.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/watchdog/alim1535_wdt.c b/drivers/watchdog/alim1535_wdt.c
index 60f0c2eb..4ba2b860 100644
--- a/drivers/watchdog/alim1535_wdt.c
+++ b/drivers/watchdog/alim1535_wdt.c
@@ -107,6 +107,7 @@ static void ali_keepalive(void)
static int ali_settimer(int t)
{
+spin_lock(&ali_lock);
if (t < 0)
return -EINVAL;
else if (t < 60)
@@ -117,7 +118,7 @@ static int ali_settimer(int t)
ali_timeout_bits = (t / 300)|(1 << 6)|(1 << 7);
else
return -EINVAL;
-
+spin_unlock(&ali_lock);
timeout = t;
return 0;
}
--
2.17.1
Signed-off-by: Mark Balantzyan
Cc: Pavel Andrianov
Cc:Wim Van Sebroeck (maintainer:WATCHDOG DEVICE
DRIVERS)
Cc: Guenter Roeck (maintainer:WATCHDOG DEVICE DRIVERS)
Cc:linux-watch...@vger.kernel.org (open list:WATCHDOG DEVICE DRIVERS)
Cc:linux-kernel@vger.kernel.org (open list)
