* [KJ] (re) audit return code handling for kernel_thread [0/3]
@ 2006-07-29 20:11 ` Neil Horman
0 siblings, 0 replies; 16+ messages in thread
From: Neil Horman @ 2006-07-29 20:11 UTC (permalink / raw)
To: kernel-janitors, linux-kernel, nhorman, torvalds, akpm, marcel,
fpavlic, paulus, bcollins, tony.luck
Hello all-
I recently posted 11 patches to audit the handling of return codes for
calls made to kernel_thread. I'd like to rescind those patches, in favor of the
following three, which take into account the feedback I've recieved on them.
Specifically these patches are redistributed to only be three patches rather
than 11, so that each patch fixes a class of problem that I found. Also the
changes for delayed checks of return codes that don't consider negative returns
now properly check for >= 0 codes. Finally added printks, all have the same
string format, to save space in the string table.
Thanks & Regards
Neil
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [KJ] (re) audit return code handling for kernel_thread [1/3]
2006-07-29 20:11 ` Neil Horman
@ 2006-07-29 20:15 ` Neil Horman
-1 siblings, 0 replies; 16+ messages in thread
From: Neil Horman @ 2006-07-29 20:15 UTC (permalink / raw)
To: kernel-janitors, linux-kernel, torvalds, akpm, marcel, fpavlic,
paulus, bcollins, tony.luck
Patch to audit return code checking of kernel_thread. These fixes correct those
callers who fail to check the return code of kernel_thread at all
Thanks & Regards
Neil
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
arch/s390/mm/cmm.c | 6 +++++-
drivers/macintosh/mediabay.c | 4 +++-
drivers/s390/net/lcs.c | 10 +++++++---
drivers/s390/net/qeth_main.c | 12 +++++++++---
init/main.c | 6 +++++-
net/bluetooth/rfcomm/core.c | 6 +++++-
6 files changed, 34 insertions(+), 10 deletions(-)
--- a/arch/s390/mm/cmm.c
+++ b/arch/s390/mm/cmm.c
@@ -161,7 +161,11 @@ cmm_thread(void *dummy)
static void
cmm_start_thread(void)
{
- kernel_thread(cmm_thread, NULL, 0);
+ if (kernel_thread(cmm_thread, NULL, 0) < 0) {
+ printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__,__LINE__);
+ clear_bit(0,&cmm_thread_active);
+ }
}
--- a/drivers/macintosh/mediabay.c
+++ b/drivers/macintosh/mediabay.c
@@ -699,7 +699,9 @@ static int __devinit media_bay_attach(st
/* Startup kernel thread */
if (i = 0)
- kernel_thread(media_bay_task, NULL, CLONE_KERNEL);
+ if (kernel_thread(media_bay_task, NULL, CLONE_KERNEL) < 0)
+ printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__,__LINE__);
return 0;
--- a/drivers/s390/net/lcs.c
+++ b/drivers/s390/net/lcs.c
@@ -1729,11 +1729,15 @@ lcs_start_kernel_thread(struct lcs_card
{
LCS_DBF_TEXT(5, trace, "krnthrd");
if (lcs_do_start_thread(card, LCS_RECOVERY_THREAD))
- kernel_thread(lcs_recovery, (void *) card, SIGCHLD);
+ if (kernel_thread(lcs_recovery, (void *) card, SIGCHLD) < 0)
+ printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__, __LINE__);
#ifdef CONFIG_IP_MULTICAST
if (lcs_do_start_thread(card, LCS_SET_MC_THREAD))
- kernel_thread(lcs_register_mc_addresses,
- (void *) card, SIGCHLD);
+ if (kernel_thread(lcs_register_mc_addresses,
+ (void *) card, SIGCHLD) < 0)
+ printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__, __LINE__);
#endif
}
--- a/drivers/s390/net/qeth_main.c
+++ b/drivers/s390/net/qeth_main.c
@@ -1048,11 +1048,17 @@ qeth_start_kernel_thread(struct qeth_car
return;
if (qeth_do_start_thread(card, QETH_SET_IP_THREAD))
- kernel_thread(qeth_register_ip_addresses, (void *)card,SIGCHLD);
+ if (kernel_thread(qeth_register_ip_addresses, (void *)card,SIGCHLD) < 0)
+ printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__, __LINE__);
if (qeth_do_start_thread(card, QETH_SET_PROMISC_MODE_THREAD))
- kernel_thread(qeth_set_promisc_mode, (void *)card, SIGCHLD);
+ if (kernel_thread(qeth_set_promisc_mode, (void *)card, SIGCHLD) < 0)
+ printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__, __LINE__);
if (qeth_do_start_thread(card, QETH_RECOVER_THREAD))
- kernel_thread(qeth_recover, (void *) card, SIGCHLD);
+ if (kernel_thread(qeth_recover, (void *) card, SIGCHLD) < 0)
+ printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__, __LINE__);
}
--- a/init/main.c
+++ b/init/main.c
@@ -389,7 +389,11 @@ #endif
static void noinline rest_init(void)
__releases(kernel_lock)
{
- kernel_thread(init, NULL, CLONE_FS | CLONE_SIGHAND);
+ if (kernel_thread(init, NULL, CLONE_FS | CLONE_SIGHAND) < 0) {
+ printk(KERN_CRIT "Unable to start kernel thread at %s:%d\n",
+ __FUNCTION__, __LINE__);
+ BUG();
+ }
numa_default_policy();
unlock_kernel();
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -2052,11 +2052,15 @@ static CLASS_ATTR(rfcomm_dlc, S_IRUGO, r
/* ---- Initialization ---- */
static int __init rfcomm_init(void)
{
+ int ret;
l2cap_load();
hci_register_cb(&rfcomm_cb);
- kernel_thread(rfcomm_run, NULL, CLONE_KERNEL);
+ ret = kernel_thread(rfcomm_run, NULL, CLONE_KERNEL);
+
+ if (ret < 0)
+ return ret;
class_create_file(bt_class, &class_attr_rfcomm_dlc);
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [KJ] (re) audit return code handling for kernel_thread [1/3]
@ 2006-07-29 20:15 ` Neil Horman
0 siblings, 0 replies; 16+ messages in thread
From: Neil Horman @ 2006-07-29 20:15 UTC (permalink / raw)
To: kernel-janitors, linux-kernel, torvalds, akpm, marcel, fpavlic,
paulus, bcollins, tony.luck
Patch to audit return code checking of kernel_thread. These fixes correct those
callers who fail to check the return code of kernel_thread at all
Thanks & Regards
Neil
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
arch/s390/mm/cmm.c | 6 +++++-
drivers/macintosh/mediabay.c | 4 +++-
drivers/s390/net/lcs.c | 10 +++++++---
drivers/s390/net/qeth_main.c | 12 +++++++++---
init/main.c | 6 +++++-
net/bluetooth/rfcomm/core.c | 6 +++++-
6 files changed, 34 insertions(+), 10 deletions(-)
--- a/arch/s390/mm/cmm.c
+++ b/arch/s390/mm/cmm.c
@@ -161,7 +161,11 @@ cmm_thread(void *dummy)
static void
cmm_start_thread(void)
{
- kernel_thread(cmm_thread, NULL, 0);
+ if (kernel_thread(cmm_thread, NULL, 0) < 0) {
+ printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__,__LINE__);
+ clear_bit(0,&cmm_thread_active);
+ }
}
--- a/drivers/macintosh/mediabay.c
+++ b/drivers/macintosh/mediabay.c
@@ -699,7 +699,9 @@ static int __devinit media_bay_attach(st
/* Startup kernel thread */
if (i == 0)
- kernel_thread(media_bay_task, NULL, CLONE_KERNEL);
+ if (kernel_thread(media_bay_task, NULL, CLONE_KERNEL) < 0)
+ printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__,__LINE__);
return 0;
--- a/drivers/s390/net/lcs.c
+++ b/drivers/s390/net/lcs.c
@@ -1729,11 +1729,15 @@ lcs_start_kernel_thread(struct lcs_card
{
LCS_DBF_TEXT(5, trace, "krnthrd");
if (lcs_do_start_thread(card, LCS_RECOVERY_THREAD))
- kernel_thread(lcs_recovery, (void *) card, SIGCHLD);
+ if (kernel_thread(lcs_recovery, (void *) card, SIGCHLD) < 0)
+ printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__, __LINE__);
#ifdef CONFIG_IP_MULTICAST
if (lcs_do_start_thread(card, LCS_SET_MC_THREAD))
- kernel_thread(lcs_register_mc_addresses,
- (void *) card, SIGCHLD);
+ if (kernel_thread(lcs_register_mc_addresses,
+ (void *) card, SIGCHLD) < 0)
+ printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__, __LINE__);
#endif
}
--- a/drivers/s390/net/qeth_main.c
+++ b/drivers/s390/net/qeth_main.c
@@ -1048,11 +1048,17 @@ qeth_start_kernel_thread(struct qeth_car
return;
if (qeth_do_start_thread(card, QETH_SET_IP_THREAD))
- kernel_thread(qeth_register_ip_addresses, (void *)card,SIGCHLD);
+ if (kernel_thread(qeth_register_ip_addresses, (void *)card,SIGCHLD) < 0)
+ printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__, __LINE__);
if (qeth_do_start_thread(card, QETH_SET_PROMISC_MODE_THREAD))
- kernel_thread(qeth_set_promisc_mode, (void *)card, SIGCHLD);
+ if (kernel_thread(qeth_set_promisc_mode, (void *)card, SIGCHLD) < 0)
+ printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__, __LINE__);
if (qeth_do_start_thread(card, QETH_RECOVER_THREAD))
- kernel_thread(qeth_recover, (void *) card, SIGCHLD);
+ if (kernel_thread(qeth_recover, (void *) card, SIGCHLD) < 0)
+ printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__, __LINE__);
}
--- a/init/main.c
+++ b/init/main.c
@@ -389,7 +389,11 @@ #endif
static void noinline rest_init(void)
__releases(kernel_lock)
{
- kernel_thread(init, NULL, CLONE_FS | CLONE_SIGHAND);
+ if (kernel_thread(init, NULL, CLONE_FS | CLONE_SIGHAND) < 0) {
+ printk(KERN_CRIT "Unable to start kernel thread at %s:%d\n",
+ __FUNCTION__, __LINE__);
+ BUG();
+ }
numa_default_policy();
unlock_kernel();
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -2052,11 +2052,15 @@ static CLASS_ATTR(rfcomm_dlc, S_IRUGO, r
/* ---- Initialization ---- */
static int __init rfcomm_init(void)
{
+ int ret;
l2cap_load();
hci_register_cb(&rfcomm_cb);
- kernel_thread(rfcomm_run, NULL, CLONE_KERNEL);
+ ret = kernel_thread(rfcomm_run, NULL, CLONE_KERNEL);
+
+ if (ret < 0)
+ return ret;
class_create_file(bt_class, &class_attr_rfcomm_dlc);
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [KJ] (re) audit return code handling for kernel_thread [1/3]
2006-07-29 20:15 ` Neil Horman
@ 2006-07-30 0:03 ` Andrew Morton
-1 siblings, 0 replies; 16+ messages in thread
From: Andrew Morton @ 2006-07-30 0:03 UTC (permalink / raw)
To: Neil Horman
Cc: kernel-janitors, linux-kernel, torvalds, marcel, fpavlic, paulus,
bcollins, tony.luck
On Sat, 29 Jul 2006 16:15:55 -0400
Neil Horman <nhorman@tuxdriver.com> wrote:
> Patch to audit return code checking of kernel_thread. These fixes correct those
> callers who fail to check the return code of kernel_thread at all
>
>
Various people are running around converting open-coded kernel_thread
callers over to the kthread API. Generally that's a good thing, and error
checking should be incorporated at that time.
So there's probably not a lot of point in making these changes now - it'd
be better to work with the various subsystem owners on doing the kthread
conversion.
> --- a/arch/s390/mm/cmm.c
> +++ b/arch/s390/mm/cmm.c
> @@ -161,7 +161,11 @@ cmm_thread(void *dummy)
> static void
> cmm_start_thread(void)
> {
> - kernel_thread(cmm_thread, NULL, 0);
> + if (kernel_thread(cmm_thread, NULL, 0) < 0) {
> + printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
> + __FUNCTION__,__LINE__);
> + clear_bit(0,&cmm_thread_active);
> + }
> }
This is OK as far as it goes. But really we should propagate any failure
back up to cmm_init() and fail the whole thing, rather than leaving the
driver hanging around in a loaded-but-useless state.
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [KJ] (re) audit return code handling for kernel_thread [1/3]
@ 2006-07-30 0:03 ` Andrew Morton
0 siblings, 0 replies; 16+ messages in thread
From: Andrew Morton @ 2006-07-30 0:03 UTC (permalink / raw)
To: Neil Horman
Cc: kernel-janitors, linux-kernel, torvalds, marcel, fpavlic, paulus,
bcollins, tony.luck
On Sat, 29 Jul 2006 16:15:55 -0400
Neil Horman <nhorman@tuxdriver.com> wrote:
> Patch to audit return code checking of kernel_thread. These fixes correct those
> callers who fail to check the return code of kernel_thread at all
>
>
Various people are running around converting open-coded kernel_thread
callers over to the kthread API. Generally that's a good thing, and error
checking should be incorporated at that time.
So there's probably not a lot of point in making these changes now - it'd
be better to work with the various subsystem owners on doing the kthread
conversion.
> --- a/arch/s390/mm/cmm.c
> +++ b/arch/s390/mm/cmm.c
> @@ -161,7 +161,11 @@ cmm_thread(void *dummy)
> static void
> cmm_start_thread(void)
> {
> - kernel_thread(cmm_thread, NULL, 0);
> + if (kernel_thread(cmm_thread, NULL, 0) < 0) {
> + printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
> + __FUNCTION__,__LINE__);
> + clear_bit(0,&cmm_thread_active);
> + }
> }
This is OK as far as it goes. But really we should propagate any failure
back up to cmm_init() and fail the whole thing, rather than leaving the
driver hanging around in a loaded-but-useless state.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [KJ] (re) audit return code handling for kernel_thread [1/3]
2006-07-30 0:03 ` Andrew Morton
@ 2006-07-30 0:48 ` Neil Horman
-1 siblings, 0 replies; 16+ messages in thread
From: Neil Horman @ 2006-07-30 0:48 UTC (permalink / raw)
To: Andrew Morton
Cc: kernel-janitors, linux-kernel, torvalds, marcel, fpavlic, paulus,
bcollins, tony.luck
On Sat, Jul 29, 2006 at 05:03:33PM -0700, Andrew Morton wrote:
> On Sat, 29 Jul 2006 16:15:55 -0400
> Neil Horman <nhorman@tuxdriver.com> wrote:
>
> > Patch to audit return code checking of kernel_thread. These fixes correct those
> > callers who fail to check the return code of kernel_thread at all
> >
> >
>
> Various people are running around converting open-coded kernel_thread
> callers over to the kthread API. Generally that's a good thing, and error
> checking should be incorporated at that time.
>
> So there's probably not a lot of point in making these changes now - it'd
> be better to work with the various subsystem owners on doing the kthread
> conversion.
>
> > --- a/arch/s390/mm/cmm.c
> > +++ b/arch/s390/mm/cmm.c
> > @@ -161,7 +161,11 @@ cmm_thread(void *dummy)
> > static void
> > cmm_start_thread(void)
> > {
> > - kernel_thread(cmm_thread, NULL, 0);
> > + if (kernel_thread(cmm_thread, NULL, 0) < 0) {
> > + printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
> > + __FUNCTION__,__LINE__);
> > + clear_bit(0,&cmm_thread_active);
> > + }
> > }
>
> This is OK as far as it goes. But really we should propagate any failure
> back up to cmm_init() and fail the whole thing, rather than leaving the
> driver hanging around in a loaded-but-useless state.
Understood, new patch attached, that removes most of the additional failure to
check return code cases. I've left the cmm_start_thread case and the
rfcomm_init cases as is, because the cmm_start_thread case is called
asynchronously from a work queue, fired from a timer, meaning we cannot
propogate the error to prevent the module from loading, and the rfcomm_init case
does precisely what you ask, in that it detects a failure to start the kernel
thread, and fails the module load if the thread creation fails.
Thanks & Regards
Neil
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
arch/s390/mm/cmm.c | 6 +++++-
net/bluetooth/rfcomm/core.c | 6 +++++-
2 files changed, 10 insertions(+), 2 deletions(-)
--- a/arch/s390/mm/cmm.c
+++ b/arch/s390/mm/cmm.c
@@ -161,7 +161,11 @@
static void
cmm_start_thread(void)
{
- kernel_thread(cmm_thread, NULL, 0);
+ if (kernel_thread(cmm_thread, NULL, 0) < 0) {
+ printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__,__LINE__);
+ clear_bit(0,&cmm_thread_active);
+ }
}
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -2052,11 +2052,15 @@
/* ---- Initialization ---- */
static int __init rfcomm_init(void)
{
+ int ret;
l2cap_load();
hci_register_cb(&rfcomm_cb);
- kernel_thread(rfcomm_run, NULL, CLONE_KERNEL);
+ ret = kernel_thread(rfcomm_run, NULL, CLONE_KERNEL);
+
+ if (ret < 0)
+ return ret;
class_create_file(bt_class, &class_attr_rfcomm_dlc);
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [KJ] (re) audit return code handling for kernel_thread [1/3]
@ 2006-07-30 0:48 ` Neil Horman
0 siblings, 0 replies; 16+ messages in thread
From: Neil Horman @ 2006-07-30 0:48 UTC (permalink / raw)
To: Andrew Morton
Cc: kernel-janitors, linux-kernel, torvalds, marcel, fpavlic, paulus,
bcollins, tony.luck
On Sat, Jul 29, 2006 at 05:03:33PM -0700, Andrew Morton wrote:
> On Sat, 29 Jul 2006 16:15:55 -0400
> Neil Horman <nhorman@tuxdriver.com> wrote:
>
> > Patch to audit return code checking of kernel_thread. These fixes correct those
> > callers who fail to check the return code of kernel_thread at all
> >
> >
>
> Various people are running around converting open-coded kernel_thread
> callers over to the kthread API. Generally that's a good thing, and error
> checking should be incorporated at that time.
>
> So there's probably not a lot of point in making these changes now - it'd
> be better to work with the various subsystem owners on doing the kthread
> conversion.
>
> > --- a/arch/s390/mm/cmm.c
> > +++ b/arch/s390/mm/cmm.c
> > @@ -161,7 +161,11 @@ cmm_thread(void *dummy)
> > static void
> > cmm_start_thread(void)
> > {
> > - kernel_thread(cmm_thread, NULL, 0);
> > + if (kernel_thread(cmm_thread, NULL, 0) < 0) {
> > + printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
> > + __FUNCTION__,__LINE__);
> > + clear_bit(0,&cmm_thread_active);
> > + }
> > }
>
> This is OK as far as it goes. But really we should propagate any failure
> back up to cmm_init() and fail the whole thing, rather than leaving the
> driver hanging around in a loaded-but-useless state.
Understood, new patch attached, that removes most of the additional failure to
check return code cases. I've left the cmm_start_thread case and the
rfcomm_init cases as is, because the cmm_start_thread case is called
asynchronously from a work queue, fired from a timer, meaning we cannot
propogate the error to prevent the module from loading, and the rfcomm_init case
does precisely what you ask, in that it detects a failure to start the kernel
thread, and fails the module load if the thread creation fails.
Thanks & Regards
Neil
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
arch/s390/mm/cmm.c | 6 +++++-
net/bluetooth/rfcomm/core.c | 6 +++++-
2 files changed, 10 insertions(+), 2 deletions(-)
--- a/arch/s390/mm/cmm.c
+++ b/arch/s390/mm/cmm.c
@@ -161,7 +161,11 @@
static void
cmm_start_thread(void)
{
- kernel_thread(cmm_thread, NULL, 0);
+ if (kernel_thread(cmm_thread, NULL, 0) < 0) {
+ printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__,__LINE__);
+ clear_bit(0,&cmm_thread_active);
+ }
}
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -2052,11 +2052,15 @@
/* ---- Initialization ---- */
static int __init rfcomm_init(void)
{
+ int ret;
l2cap_load();
hci_register_cb(&rfcomm_cb);
- kernel_thread(rfcomm_run, NULL, CLONE_KERNEL);
+ ret = kernel_thread(rfcomm_run, NULL, CLONE_KERNEL);
+
+ if (ret < 0)
+ return ret;
class_create_file(bt_class, &class_attr_rfcomm_dlc);
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [KJ] (re) audit return code handling for kernel_thread [1/3]
2006-07-30 0:48 ` Neil Horman
@ 2006-07-30 21:39 ` Heiko Carstens
-1 siblings, 0 replies; 16+ messages in thread
From: Heiko Carstens @ 2006-07-30 21:39 UTC (permalink / raw)
To: Neil Horman
Cc: Andrew Morton, kernel-janitors, linux-kernel, torvalds, marcel,
fpavlic, paulus, bcollins, tony.luck
> > > --- a/arch/s390/mm/cmm.c
> > > +++ b/arch/s390/mm/cmm.c
> > > @@ -161,7 +161,11 @@ cmm_thread(void *dummy)
> > > static void
> > > cmm_start_thread(void)
> > > {
> > > - kernel_thread(cmm_thread, NULL, 0);
> > > + if (kernel_thread(cmm_thread, NULL, 0) < 0) {
> > > + printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
> > > + __FUNCTION__,__LINE__);
> > > + clear_bit(0,&cmm_thread_active);
> > > + }
> > > }
> >
> > This is OK as far as it goes. But really we should propagate any failure
> > back up to cmm_init() and fail the whole thing, rather than leaving the
> > driver hanging around in a loaded-but-useless state.
>
>
> Understood, new patch attached, that removes most of the additional failure to
> check return code cases. I've left the cmm_start_thread case and the
> rfcomm_init cases as is, because the cmm_start_thread case is called
> asynchronously from a work queue, fired from a timer, meaning we cannot
> propogate the error to prevent the module from loading, and the rfcomm_init case
> does precisely what you ask, in that it detects a failure to start the kernel
> thread, and fails the module load if the thread creation fails.
The cmm stuff is a bit more broken than that it just doesn't take the return
value of kernel_thread into account. I think this patch would be better:
From: Heiko Carstens <heiko.carstens@de.ibm.com>
[patch] s390: fix cmm kernel thread handling.
Convert cmm's usage of kernel_thread to kthread_create. Also create the
cmmthread at module load time, so it is possible to check if creation of
the thread fails.
In addition the cmmthread now gets terminated when the module gets unloaded
instead of leaving a stale kernel thread that would execute random data
if it ever would run again.
Also check the return values of other registration functions at module load
and handle their return values appropriately.
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
arch/s390/mm/cmm.c | 45 +++++++++++++++++++++++----------------------
1 file changed, 23 insertions(+), 22 deletions(-)
diff --git a/arch/s390/mm/cmm.c b/arch/s390/mm/cmm.c
index ceea51c..5428689 100644
--- a/arch/s390/mm/cmm.c
+++ b/arch/s390/mm/cmm.c
@@ -15,6 +15,7 @@
#include <linux/sched.h>
#include <linux/sysctl.h>
#include <linux/ctype.h>
+#include <linux/kthread.h>
#include <asm/pgalloc.h>
#include <asm/uaccess.h>
@@ -44,8 +45,7 @@ static long cmm_timeout_seconds = 0;
static struct cmm_page_array *cmm_page_list = NULL;
static struct cmm_page_array *cmm_timed_page_list = NULL;
-static unsigned long cmm_thread_active = 0;
-static struct work_struct cmm_thread_starter;
+static struct task_struct *cmm_thread_ptr;
static wait_queue_head_t cmm_thread_wait;
static struct timer_list cmm_timer;
@@ -124,16 +124,11 @@ cmm_free_pages(long pages, long *counter
static int
cmm_thread(void *dummy)
{
- int rc;
-
- daemonize("cmmthread");
while (1) {
- rc = wait_event_interruptible(cmm_thread_wait,
- (cmm_pages != cmm_pages_target ||
- cmm_timed_pages != cmm_timed_pages_target));
- if (rc = -ERESTARTSYS) {
- /* Got kill signal. End thread. */
- clear_bit(0, &cmm_thread_active);
+ wait_event(cmm_thread_wait,
+ (cmm_pages != cmm_pages_target ||
+ cmm_timed_pages != cmm_timed_pages_target));
+ if (kthread_should_stop()) {
cmm_pages_target = cmm_pages;
cmm_timed_pages_target = cmm_timed_pages;
break;
@@ -159,16 +154,8 @@ cmm_thread(void *dummy)
}
static void
-cmm_start_thread(void)
-{
- kernel_thread(cmm_thread, NULL, 0);
-}
-
-static void
cmm_kick_thread(void)
{
- if (!test_and_set_bit(0, &cmm_thread_active))
- schedule_work(&cmm_thread_starter);
wake_up(&cmm_thread_wait);
}
@@ -412,21 +399,35 @@ struct ctl_table_header *cmm_sysctl_head
static int
cmm_init (void)
{
+ int rc = 0;
+
+ cmm_thread_ptr = kthread_create(cmm_thread, NULL, "cmmthread");
+ if (IS_ERR(cmm_thread_ptr))
+ return PTR_ERR(cmm_thread_ptr);
#ifdef CONFIG_CMM_PROC
cmm_sysctl_header = register_sysctl_table(cmm_dir_table, 1);
+ if (!cmm_sysctl_header) {
+ kthread_stop(cmm_thread_ptr);
+ return -ENOMEM;
+ }
#endif
#ifdef CONFIG_CMM_IUCV
- smsg_register_callback(SMSG_PREFIX, cmm_smsg_target);
+ rc = smsg_register_callback(SMSG_PREFIX, cmm_smsg_target);
+ if (rc < 0) {
+ unregister_sysctl_table(cmm_sysctl_header);
+ kthread_stop(cmm_thread_ptr);
+ return rc;
+ }
#endif
- INIT_WORK(&cmm_thread_starter, (void *) cmm_start_thread, NULL);
init_waitqueue_head(&cmm_thread_wait);
init_timer(&cmm_timer);
- return 0;
+ return rc;
}
static void
cmm_exit(void)
{
+ kthread_stop(cmm_thread_ptr);
cmm_free_pages(cmm_pages, &cmm_pages, &cmm_page_list);
cmm_free_pages(cmm_timed_pages, &cmm_timed_pages, &cmm_timed_page_list);
#ifdef CONFIG_CMM_PROC
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [KJ] (re) audit return code handling for kernel_thread [1/3]
@ 2006-07-30 21:39 ` Heiko Carstens
0 siblings, 0 replies; 16+ messages in thread
From: Heiko Carstens @ 2006-07-30 21:39 UTC (permalink / raw)
To: Neil Horman
Cc: Andrew Morton, kernel-janitors, linux-kernel, torvalds, marcel,
fpavlic, paulus, bcollins, tony.luck
> > > --- a/arch/s390/mm/cmm.c
> > > +++ b/arch/s390/mm/cmm.c
> > > @@ -161,7 +161,11 @@ cmm_thread(void *dummy)
> > > static void
> > > cmm_start_thread(void)
> > > {
> > > - kernel_thread(cmm_thread, NULL, 0);
> > > + if (kernel_thread(cmm_thread, NULL, 0) < 0) {
> > > + printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
> > > + __FUNCTION__,__LINE__);
> > > + clear_bit(0,&cmm_thread_active);
> > > + }
> > > }
> >
> > This is OK as far as it goes. But really we should propagate any failure
> > back up to cmm_init() and fail the whole thing, rather than leaving the
> > driver hanging around in a loaded-but-useless state.
>
>
> Understood, new patch attached, that removes most of the additional failure to
> check return code cases. I've left the cmm_start_thread case and the
> rfcomm_init cases as is, because the cmm_start_thread case is called
> asynchronously from a work queue, fired from a timer, meaning we cannot
> propogate the error to prevent the module from loading, and the rfcomm_init case
> does precisely what you ask, in that it detects a failure to start the kernel
> thread, and fails the module load if the thread creation fails.
The cmm stuff is a bit more broken than that it just doesn't take the return
value of kernel_thread into account. I think this patch would be better:
From: Heiko Carstens <heiko.carstens@de.ibm.com>
[patch] s390: fix cmm kernel thread handling.
Convert cmm's usage of kernel_thread to kthread_create. Also create the
cmmthread at module load time, so it is possible to check if creation of
the thread fails.
In addition the cmmthread now gets terminated when the module gets unloaded
instead of leaving a stale kernel thread that would execute random data
if it ever would run again.
Also check the return values of other registration functions at module load
and handle their return values appropriately.
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
arch/s390/mm/cmm.c | 45 +++++++++++++++++++++++----------------------
1 file changed, 23 insertions(+), 22 deletions(-)
diff --git a/arch/s390/mm/cmm.c b/arch/s390/mm/cmm.c
index ceea51c..5428689 100644
--- a/arch/s390/mm/cmm.c
+++ b/arch/s390/mm/cmm.c
@@ -15,6 +15,7 @@
#include <linux/sched.h>
#include <linux/sysctl.h>
#include <linux/ctype.h>
+#include <linux/kthread.h>
#include <asm/pgalloc.h>
#include <asm/uaccess.h>
@@ -44,8 +45,7 @@ static long cmm_timeout_seconds = 0;
static struct cmm_page_array *cmm_page_list = NULL;
static struct cmm_page_array *cmm_timed_page_list = NULL;
-static unsigned long cmm_thread_active = 0;
-static struct work_struct cmm_thread_starter;
+static struct task_struct *cmm_thread_ptr;
static wait_queue_head_t cmm_thread_wait;
static struct timer_list cmm_timer;
@@ -124,16 +124,11 @@ cmm_free_pages(long pages, long *counter
static int
cmm_thread(void *dummy)
{
- int rc;
-
- daemonize("cmmthread");
while (1) {
- rc = wait_event_interruptible(cmm_thread_wait,
- (cmm_pages != cmm_pages_target ||
- cmm_timed_pages != cmm_timed_pages_target));
- if (rc == -ERESTARTSYS) {
- /* Got kill signal. End thread. */
- clear_bit(0, &cmm_thread_active);
+ wait_event(cmm_thread_wait,
+ (cmm_pages != cmm_pages_target ||
+ cmm_timed_pages != cmm_timed_pages_target));
+ if (kthread_should_stop()) {
cmm_pages_target = cmm_pages;
cmm_timed_pages_target = cmm_timed_pages;
break;
@@ -159,16 +154,8 @@ cmm_thread(void *dummy)
}
static void
-cmm_start_thread(void)
-{
- kernel_thread(cmm_thread, NULL, 0);
-}
-
-static void
cmm_kick_thread(void)
{
- if (!test_and_set_bit(0, &cmm_thread_active))
- schedule_work(&cmm_thread_starter);
wake_up(&cmm_thread_wait);
}
@@ -412,21 +399,35 @@ struct ctl_table_header *cmm_sysctl_head
static int
cmm_init (void)
{
+ int rc = 0;
+
+ cmm_thread_ptr = kthread_create(cmm_thread, NULL, "cmmthread");
+ if (IS_ERR(cmm_thread_ptr))
+ return PTR_ERR(cmm_thread_ptr);
#ifdef CONFIG_CMM_PROC
cmm_sysctl_header = register_sysctl_table(cmm_dir_table, 1);
+ if (!cmm_sysctl_header) {
+ kthread_stop(cmm_thread_ptr);
+ return -ENOMEM;
+ }
#endif
#ifdef CONFIG_CMM_IUCV
- smsg_register_callback(SMSG_PREFIX, cmm_smsg_target);
+ rc = smsg_register_callback(SMSG_PREFIX, cmm_smsg_target);
+ if (rc < 0) {
+ unregister_sysctl_table(cmm_sysctl_header);
+ kthread_stop(cmm_thread_ptr);
+ return rc;
+ }
#endif
- INIT_WORK(&cmm_thread_starter, (void *) cmm_start_thread, NULL);
init_waitqueue_head(&cmm_thread_wait);
init_timer(&cmm_timer);
- return 0;
+ return rc;
}
static void
cmm_exit(void)
{
+ kthread_stop(cmm_thread_ptr);
cmm_free_pages(cmm_pages, &cmm_pages, &cmm_page_list);
cmm_free_pages(cmm_timed_pages, &cmm_timed_pages, &cmm_timed_page_list);
#ifdef CONFIG_CMM_PROC
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [KJ] (re) audit return code handling for kernel_thread [1/3]
2006-07-30 21:39 ` Heiko Carstens
@ 2006-07-31 9:17 ` Heiko Carstens
-1 siblings, 0 replies; 16+ messages in thread
From: Heiko Carstens @ 2006-07-31 9:17 UTC (permalink / raw)
To: Neil Horman
Cc: Andrew Morton, kernel-janitors, linux-kernel, torvalds, marcel,
fpavlic, paulus, bcollins, tony.luck, Martin Schwidefsky
On Sun, Jul 30, 2006 at 11:39:36PM +0200, Heiko Carstens wrote:
> > > > --- a/arch/s390/mm/cmm.c
> > > > +++ b/arch/s390/mm/cmm.c
> > > > @@ -161,7 +161,11 @@ cmm_thread(void *dummy)
> > > > static void
> > > > cmm_start_thread(void)
> > > > {
> > > > - kernel_thread(cmm_thread, NULL, 0);
> > > > + if (kernel_thread(cmm_thread, NULL, 0) < 0) {
> > > > + printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
> > > > + __FUNCTION__,__LINE__);
> > > > + clear_bit(0,&cmm_thread_active);
> > > > + }
> > > > }
> > >
> > > This is OK as far as it goes. But really we should propagate any failure
> > > back up to cmm_init() and fail the whole thing, rather than leaving the
> > > driver hanging around in a loaded-but-useless state.
Updated patch. This time against -mm. And actually it also works unlike
my first patch...
From: Heiko Carstens <heiko.carstens@de.ibm.com>
[patch] s390: fix cmm kernel thread handling.
Convert cmm's usage of kernel_thread to kthread_run. Also create the
cmmthread at module load time, so it is possible to check if creation of
the thread fails.
In addition the cmmthread now gets terminated when the module gets unloaded
instead of leaving a stale kernel thread.
Also check the return values of other registration functions at module load
and handle their return values appropriately.
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
Index: linux-2.6.18-rc2-mm1/arch/s390/mm/cmm.c
=================================--- linux-2.6.18-rc2-mm1.orig/arch/s390/mm/cmm.c 2006-07-31 10:27:31.000000000 +0200
+++ linux-2.6.18-rc2-mm1/arch/s390/mm/cmm.c 2006-07-31 11:09:19.000000000 +0200
@@ -16,6 +16,7 @@
#include <linux/sysctl.h>
#include <linux/ctype.h>
#include <linux/swap.h>
+#include <linux/kthread.h>
#include <asm/pgalloc.h>
#include <asm/uaccess.h>
@@ -46,8 +47,7 @@
static struct cmm_page_array *cmm_timed_page_list;
static DEFINE_SPINLOCK(cmm_lock);
-static unsigned long cmm_thread_active;
-static struct work_struct cmm_thread_starter;
+static struct task_struct *cmm_thread_ptr;
static wait_queue_head_t cmm_thread_wait;
static struct timer_list cmm_timer;
@@ -159,14 +159,12 @@
{
int rc;
- daemonize("cmmthread");
while (1) {
rc = wait_event_interruptible(cmm_thread_wait,
(cmm_pages != cmm_pages_target ||
- cmm_timed_pages != cmm_timed_pages_target));
- if (rc = -ERESTARTSYS) {
- /* Got kill signal. End thread. */
- clear_bit(0, &cmm_thread_active);
+ cmm_timed_pages != cmm_timed_pages_target ||
+ kthread_should_stop()));
+ if (kthread_should_stop() || rc = -ERESTARTSYS) {
cmm_pages_target = cmm_pages;
cmm_timed_pages_target = cmm_timed_pages;
break;
@@ -192,16 +190,8 @@
}
static void
-cmm_start_thread(void)
-{
- kernel_thread(cmm_thread, NULL, 0);
-}
-
-static void
cmm_kick_thread(void)
{
- if (!test_and_set_bit(0, &cmm_thread_active))
- schedule_work(&cmm_thread_starter);
wake_up(&cmm_thread_wait);
}
@@ -445,22 +435,48 @@
static int
cmm_init (void)
{
+ int rc = -ENOMEM;
+
#ifdef CONFIG_CMM_PROC
cmm_sysctl_header = register_sysctl_table(cmm_dir_table, 1);
+ if (!cmm_sysctl_header)
+ goto out;
#endif
#ifdef CONFIG_CMM_IUCV
- smsg_register_callback(SMSG_PREFIX, cmm_smsg_target);
+ rc = smsg_register_callback(SMSG_PREFIX, cmm_smsg_target);
+ if (rc < 0)
+ goto out_smsg;
#endif
- register_oom_notifier(&cmm_oom_nb);
- INIT_WORK(&cmm_thread_starter, (void *) cmm_start_thread, NULL);
+ rc = register_oom_notifier(&cmm_oom_nb);
+ if (rc < 0)
+ goto out_oom_notify;
init_waitqueue_head(&cmm_thread_wait);
init_timer(&cmm_timer);
- return 0;
+ cmm_thread_ptr = kthread_run(cmm_thread, NULL, "cmmthread");
+ rc = IS_ERR(cmm_thread_ptr) ? PTR_ERR(cmm_thread_ptr) : 0;
+ if (!rc)
+ goto out;
+ /*
+ * kthread_create failed. undo all the stuff from above again.
+ */
+ unregister_oom_notifier(&cmm_oom_nb);
+
+out_oom_notify:
+#ifdef CONFIG_CMM_IUCV
+ smsg_unregister_callback(SMSG_PREFIX, cmm_smsg_target);
+out_smsg:
+#endif
+#ifdef CONFIG_CMM_PROC
+ unregister_sysctl_table(cmm_sysctl_header);
+#endif
+out:
+ return rc;
}
static void
cmm_exit(void)
{
+ kthread_stop(cmm_thread_ptr);
unregister_oom_notifier(&cmm_oom_nb);
cmm_free_pages(cmm_pages, &cmm_pages, &cmm_page_list);
cmm_free_pages(cmm_timed_pages, &cmm_timed_pages, &cmm_timed_page_list);
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [KJ] (re) audit return code handling for kernel_thread [1/3]
@ 2006-07-31 9:17 ` Heiko Carstens
0 siblings, 0 replies; 16+ messages in thread
From: Heiko Carstens @ 2006-07-31 9:17 UTC (permalink / raw)
To: Neil Horman
Cc: Andrew Morton, kernel-janitors, linux-kernel, torvalds, marcel,
fpavlic, paulus, bcollins, tony.luck, Martin Schwidefsky
On Sun, Jul 30, 2006 at 11:39:36PM +0200, Heiko Carstens wrote:
> > > > --- a/arch/s390/mm/cmm.c
> > > > +++ b/arch/s390/mm/cmm.c
> > > > @@ -161,7 +161,11 @@ cmm_thread(void *dummy)
> > > > static void
> > > > cmm_start_thread(void)
> > > > {
> > > > - kernel_thread(cmm_thread, NULL, 0);
> > > > + if (kernel_thread(cmm_thread, NULL, 0) < 0) {
> > > > + printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
> > > > + __FUNCTION__,__LINE__);
> > > > + clear_bit(0,&cmm_thread_active);
> > > > + }
> > > > }
> > >
> > > This is OK as far as it goes. But really we should propagate any failure
> > > back up to cmm_init() and fail the whole thing, rather than leaving the
> > > driver hanging around in a loaded-but-useless state.
Updated patch. This time against -mm. And actually it also works unlike
my first patch...
From: Heiko Carstens <heiko.carstens@de.ibm.com>
[patch] s390: fix cmm kernel thread handling.
Convert cmm's usage of kernel_thread to kthread_run. Also create the
cmmthread at module load time, so it is possible to check if creation of
the thread fails.
In addition the cmmthread now gets terminated when the module gets unloaded
instead of leaving a stale kernel thread.
Also check the return values of other registration functions at module load
and handle their return values appropriately.
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
Index: linux-2.6.18-rc2-mm1/arch/s390/mm/cmm.c
===================================================================
--- linux-2.6.18-rc2-mm1.orig/arch/s390/mm/cmm.c 2006-07-31 10:27:31.000000000 +0200
+++ linux-2.6.18-rc2-mm1/arch/s390/mm/cmm.c 2006-07-31 11:09:19.000000000 +0200
@@ -16,6 +16,7 @@
#include <linux/sysctl.h>
#include <linux/ctype.h>
#include <linux/swap.h>
+#include <linux/kthread.h>
#include <asm/pgalloc.h>
#include <asm/uaccess.h>
@@ -46,8 +47,7 @@
static struct cmm_page_array *cmm_timed_page_list;
static DEFINE_SPINLOCK(cmm_lock);
-static unsigned long cmm_thread_active;
-static struct work_struct cmm_thread_starter;
+static struct task_struct *cmm_thread_ptr;
static wait_queue_head_t cmm_thread_wait;
static struct timer_list cmm_timer;
@@ -159,14 +159,12 @@
{
int rc;
- daemonize("cmmthread");
while (1) {
rc = wait_event_interruptible(cmm_thread_wait,
(cmm_pages != cmm_pages_target ||
- cmm_timed_pages != cmm_timed_pages_target));
- if (rc == -ERESTARTSYS) {
- /* Got kill signal. End thread. */
- clear_bit(0, &cmm_thread_active);
+ cmm_timed_pages != cmm_timed_pages_target ||
+ kthread_should_stop()));
+ if (kthread_should_stop() || rc == -ERESTARTSYS) {
cmm_pages_target = cmm_pages;
cmm_timed_pages_target = cmm_timed_pages;
break;
@@ -192,16 +190,8 @@
}
static void
-cmm_start_thread(void)
-{
- kernel_thread(cmm_thread, NULL, 0);
-}
-
-static void
cmm_kick_thread(void)
{
- if (!test_and_set_bit(0, &cmm_thread_active))
- schedule_work(&cmm_thread_starter);
wake_up(&cmm_thread_wait);
}
@@ -445,22 +435,48 @@
static int
cmm_init (void)
{
+ int rc = -ENOMEM;
+
#ifdef CONFIG_CMM_PROC
cmm_sysctl_header = register_sysctl_table(cmm_dir_table, 1);
+ if (!cmm_sysctl_header)
+ goto out;
#endif
#ifdef CONFIG_CMM_IUCV
- smsg_register_callback(SMSG_PREFIX, cmm_smsg_target);
+ rc = smsg_register_callback(SMSG_PREFIX, cmm_smsg_target);
+ if (rc < 0)
+ goto out_smsg;
#endif
- register_oom_notifier(&cmm_oom_nb);
- INIT_WORK(&cmm_thread_starter, (void *) cmm_start_thread, NULL);
+ rc = register_oom_notifier(&cmm_oom_nb);
+ if (rc < 0)
+ goto out_oom_notify;
init_waitqueue_head(&cmm_thread_wait);
init_timer(&cmm_timer);
- return 0;
+ cmm_thread_ptr = kthread_run(cmm_thread, NULL, "cmmthread");
+ rc = IS_ERR(cmm_thread_ptr) ? PTR_ERR(cmm_thread_ptr) : 0;
+ if (!rc)
+ goto out;
+ /*
+ * kthread_create failed. undo all the stuff from above again.
+ */
+ unregister_oom_notifier(&cmm_oom_nb);
+
+out_oom_notify:
+#ifdef CONFIG_CMM_IUCV
+ smsg_unregister_callback(SMSG_PREFIX, cmm_smsg_target);
+out_smsg:
+#endif
+#ifdef CONFIG_CMM_PROC
+ unregister_sysctl_table(cmm_sysctl_header);
+#endif
+out:
+ return rc;
}
static void
cmm_exit(void)
{
+ kthread_stop(cmm_thread_ptr);
unregister_oom_notifier(&cmm_oom_nb);
cmm_free_pages(cmm_pages, &cmm_pages, &cmm_page_list);
cmm_free_pages(cmm_timed_pages, &cmm_timed_pages, &cmm_timed_page_list);
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [KJ] (re) audit return code handling for kernel_thread [2/3]
2006-07-29 20:11 ` Neil Horman
@ 2006-07-29 20:18 ` Neil Horman
-1 siblings, 0 replies; 16+ messages in thread
From: Neil Horman @ 2006-07-29 20:18 UTC (permalink / raw)
To: kernel-janitors, linux-kernel, torvalds, akpm, marcel, fpavlic,
paulus, bcollins, tony.luck
Patch to audit return codes from kernel_thread. This patch handles cases in
which the return code was stored for later interrogations, but the later checks
assumed that negative return codes were successful.
Thanks
Neil
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
arch/s390/mm/cmm.c | 6 +++++-
drivers/macintosh/adb.c | 7 +++++--
drivers/macintosh/therm_pm72.c | 5 ++++-
3 files changed, 14 insertions(+), 4 deletions(-)
--- a/arch/s390/mm/cmm.c
+++ b/arch/s390/mm/cmm.c
@@ -161,7 +161,11 @@ cmm_thread(void *dummy)
static void
cmm_start_thread(void)
{
- kernel_thread(cmm_thread, NULL, 0);
+ if (kernel_thread(cmm_thread, NULL, 0) < 0) {
+ printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__,__LINE__);
+ clear_bit(0,&cmm_thread_active);
+ }
}
static void
--- a/drivers/macintosh/adb.c
+++ b/drivers/macintosh/adb.c
@@ -137,7 +137,7 @@ #endif
static __inline__ void adb_wait_ms(unsigned int ms)
{
- if (current->pid && adb_probe_task_pid &&
+ if (current->pid && (adb_probe_task_pid >= 0) &&
adb_probe_task_pid = current->pid)
msleep(ms);
else
@@ -270,6 +270,9 @@ static void
__adb_probe_task(void *data)
{
adb_probe_task_pid = kernel_thread(adb_probe_task, NULL, SIGCHLD | CLONE_KERNEL);
+ if (adb_probe_task_pid < 0)
+ printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__,__LINE__);
}
static DECLARE_WORK(adb_reset_work, __adb_probe_task, NULL);
@@ -494,7 +497,7 @@ adb_request(struct adb_request *req, voi
* block. Beware that the "done" callback will be overriden !
*/
if ((flags & ADBREQ_SYNC) &&
- (current->pid && adb_probe_task_pid &&
+ (current->pid && (adb_probe_task_pid >= 0) &&
adb_probe_task_pid = current->pid)) {
req->done = adb_probe_wakeup;
rc = adb_controller->send_request(req, 0);
--- a/drivers/macintosh/therm_pm72.c
+++ b/drivers/macintosh/therm_pm72.c
@@ -1769,6 +1769,9 @@ static void start_control_loops(void)
init_completion(&ctrl_complete);
ctrl_task = kernel_thread(main_control_loop, NULL, SIGCHLD | CLONE_KERNEL);
+ if (ctrl_task < 0)
+ printk(KERN_CRIT "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__, __LINE__);
}
/*
@@ -1776,7 +1779,7 @@ static void start_control_loops(void)
*/
static void stop_control_loops(void)
{
- if (ctrl_task != 0)
+ if (ctrl_task >= 0)
wait_for_completion(&ctrl_complete);
}
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [KJ] (re) audit return code handling for kernel_thread [2/3]
@ 2006-07-29 20:18 ` Neil Horman
0 siblings, 0 replies; 16+ messages in thread
From: Neil Horman @ 2006-07-29 20:18 UTC (permalink / raw)
To: kernel-janitors, linux-kernel, torvalds, akpm, marcel, fpavlic,
paulus, bcollins, tony.luck
Patch to audit return codes from kernel_thread. This patch handles cases in
which the return code was stored for later interrogations, but the later checks
assumed that negative return codes were successful.
Thanks
Neil
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
arch/s390/mm/cmm.c | 6 +++++-
drivers/macintosh/adb.c | 7 +++++--
drivers/macintosh/therm_pm72.c | 5 ++++-
3 files changed, 14 insertions(+), 4 deletions(-)
--- a/arch/s390/mm/cmm.c
+++ b/arch/s390/mm/cmm.c
@@ -161,7 +161,11 @@ cmm_thread(void *dummy)
static void
cmm_start_thread(void)
{
- kernel_thread(cmm_thread, NULL, 0);
+ if (kernel_thread(cmm_thread, NULL, 0) < 0) {
+ printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__,__LINE__);
+ clear_bit(0,&cmm_thread_active);
+ }
}
static void
--- a/drivers/macintosh/adb.c
+++ b/drivers/macintosh/adb.c
@@ -137,7 +137,7 @@ #endif
static __inline__ void adb_wait_ms(unsigned int ms)
{
- if (current->pid && adb_probe_task_pid &&
+ if (current->pid && (adb_probe_task_pid >= 0) &&
adb_probe_task_pid == current->pid)
msleep(ms);
else
@@ -270,6 +270,9 @@ static void
__adb_probe_task(void *data)
{
adb_probe_task_pid = kernel_thread(adb_probe_task, NULL, SIGCHLD | CLONE_KERNEL);
+ if (adb_probe_task_pid < 0)
+ printk(KERN_WARNING "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__,__LINE__);
}
static DECLARE_WORK(adb_reset_work, __adb_probe_task, NULL);
@@ -494,7 +497,7 @@ adb_request(struct adb_request *req, voi
* block. Beware that the "done" callback will be overriden !
*/
if ((flags & ADBREQ_SYNC) &&
- (current->pid && adb_probe_task_pid &&
+ (current->pid && (adb_probe_task_pid >= 0) &&
adb_probe_task_pid == current->pid)) {
req->done = adb_probe_wakeup;
rc = adb_controller->send_request(req, 0);
--- a/drivers/macintosh/therm_pm72.c
+++ b/drivers/macintosh/therm_pm72.c
@@ -1769,6 +1769,9 @@ static void start_control_loops(void)
init_completion(&ctrl_complete);
ctrl_task = kernel_thread(main_control_loop, NULL, SIGCHLD | CLONE_KERNEL);
+ if (ctrl_task < 0)
+ printk(KERN_CRIT "Could not start kernel thread at %s:%d\n",
+ __FUNCTION__, __LINE__);
}
/*
@@ -1776,7 +1779,7 @@ static void start_control_loops(void)
*/
static void stop_control_loops(void)
{
- if (ctrl_task != 0)
+ if (ctrl_task >= 0)
wait_for_completion(&ctrl_complete);
}
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [KJ] (re) audit return code handling for kernel_thread [3/3]
2006-07-29 20:11 ` Neil Horman
@ 2006-07-29 20:20 ` Neil Horman
-1 siblings, 0 replies; 16+ messages in thread
From: Neil Horman @ 2006-07-29 20:20 UTC (permalink / raw)
To: kernel-janitors, linux-kernel, torvalds, akpm, marcel, fpavlic,
paulus, bcollins, tony.luck
Patch to audit return codes for kernel_thread. This patch corrects callers who
have differing assumptions about the meaning of a zero return code. I've
normalized all the callers to consider a zero return code a successful return.
Thanks & Regards
Neil
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
arch/ia64/sn/kernel/xpc_main.c | 2 +-
init/do_mounts_initrd.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
--- a/arch/ia64/sn/kernel/xpc_main.c
+++ b/arch/ia64/sn/kernel/xpc_main.c
@@ -583,7 +583,7 @@ xpc_activate_partition(struct xpc_partit
pid = kernel_thread(xpc_activating, (void *) ((u64) partid), 0);
- if (unlikely(pid <= 0)) {
+ if (unlikely(pid < 0)) {
spin_lock_irqsave(&part->act_lock, irq_flags);
part->act_state = XPC_P_INACTIVE;
XPC_SET_REASON(part, xpcCloneKThreadFailed, __LINE__);
--- a/init/do_mounts_initrd.c
+++ b/init/do_mounts_initrd.c
@@ -57,7 +57,7 @@ static void __init handle_initrd(void)
current->flags |= PF_NOFREEZE;
pid = kernel_thread(do_linuxrc, "/linuxrc", SIGCHLD);
- if (pid > 0) {
+ if (pid >= 0) {
while (pid != sys_wait4(-1, NULL, 0, NULL))
yield();
}
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [KJ] (re) audit return code handling for kernel_thread [3/3]
@ 2006-07-29 20:20 ` Neil Horman
0 siblings, 0 replies; 16+ messages in thread
From: Neil Horman @ 2006-07-29 20:20 UTC (permalink / raw)
To: kernel-janitors, linux-kernel, torvalds, akpm, marcel, fpavlic,
paulus, bcollins, tony.luck
Patch to audit return codes for kernel_thread. This patch corrects callers who
have differing assumptions about the meaning of a zero return code. I've
normalized all the callers to consider a zero return code a successful return.
Thanks & Regards
Neil
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
arch/ia64/sn/kernel/xpc_main.c | 2 +-
init/do_mounts_initrd.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
--- a/arch/ia64/sn/kernel/xpc_main.c
+++ b/arch/ia64/sn/kernel/xpc_main.c
@@ -583,7 +583,7 @@ xpc_activate_partition(struct xpc_partit
pid = kernel_thread(xpc_activating, (void *) ((u64) partid), 0);
- if (unlikely(pid <= 0)) {
+ if (unlikely(pid < 0)) {
spin_lock_irqsave(&part->act_lock, irq_flags);
part->act_state = XPC_P_INACTIVE;
XPC_SET_REASON(part, xpcCloneKThreadFailed, __LINE__);
--- a/init/do_mounts_initrd.c
+++ b/init/do_mounts_initrd.c
@@ -57,7 +57,7 @@ static void __init handle_initrd(void)
current->flags |= PF_NOFREEZE;
pid = kernel_thread(do_linuxrc, "/linuxrc", SIGCHLD);
- if (pid > 0) {
+ if (pid >= 0) {
while (pid != sys_wait4(-1, NULL, 0, NULL))
yield();
}
^ permalink raw reply [flat|nested] 16+ messages in thread