* [PATCH] [SCSI] hpsa: fix return value check in start_controller_lockup_detector() @ 2013-10-29 6:43 Wei Yongjun 2013-10-29 16:09 ` scameron 0 siblings, 1 reply; 4+ messages in thread From: Wei Yongjun @ 2013-10-29 6:43 UTC (permalink / raw) To: scameron, JBottomley; +Cc: yongjun_wei, iss_storagedev, linux-scsi From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> In case of error, the function kthread_run() returns ERR_PTR() and never returns NULL. The NULL test in the return value check should be replaced with IS_ERR(). Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> --- drivers/scsi/hpsa.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c index 891c86b..f413b14 100644 --- a/drivers/scsi/hpsa.c +++ b/drivers/scsi/hpsa.c @@ -4757,7 +4757,8 @@ static void start_controller_lockup_detector(struct ctlr_info *h) kthread_run(detect_controller_lockup_thread, NULL, HPSA); } - if (!hpsa_lockup_detector) { + if (IS_ERR(hpsa_lockup_detector)) { + hpsa_lockup_detector = NULL; dev_warn(&h->pdev->dev, "Could not start lockup detector thread\n"); return; ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] [SCSI] hpsa: fix return value check in start_controller_lockup_detector() 2013-10-29 6:43 [PATCH] [SCSI] hpsa: fix return value check in start_controller_lockup_detector() Wei Yongjun @ 2013-10-29 16:09 ` scameron 2013-10-29 21:52 ` James Bottomley 0 siblings, 1 reply; 4+ messages in thread From: scameron @ 2013-10-29 16:09 UTC (permalink / raw) To: Wei Yongjun; +Cc: JBottomley, yongjun_wei, iss_storagedev, linux-scsi, scameron On Tue, Oct 29, 2013 at 02:43:56PM +0800, Wei Yongjun wrote: > From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > > In case of error, the function kthread_run() returns ERR_PTR() > and never returns NULL. The NULL test in the return value check > should be replaced with IS_ERR(). > > Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > --- > drivers/scsi/hpsa.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c > index 891c86b..f413b14 100644 > --- a/drivers/scsi/hpsa.c > +++ b/drivers/scsi/hpsa.c > @@ -4757,7 +4757,8 @@ static void start_controller_lockup_detector(struct ctlr_info *h) > kthread_run(detect_controller_lockup_thread, > NULL, HPSA); > } > - if (!hpsa_lockup_detector) { > + if (IS_ERR(hpsa_lockup_detector)) { > + hpsa_lockup_detector = NULL; > dev_warn(&h->pdev->dev, > "Could not start lockup detector thread\n"); > return; Ack. Thanks. -- steve ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] [SCSI] hpsa: fix return value check in start_controller_lockup_detector() 2013-10-29 16:09 ` scameron @ 2013-10-29 21:52 ` James Bottomley 2013-10-30 13:33 ` scameron 0 siblings, 1 reply; 4+ messages in thread From: James Bottomley @ 2013-10-29 21:52 UTC (permalink / raw) To: scameron; +Cc: Wei Yongjun, yongjun_wei, iss_storagedev, linux-scsi On Tue, 2013-10-29 at 11:09 -0500, scameron@beardog.cce.hp.com wrote: > On Tue, Oct 29, 2013 at 02:43:56PM +0800, Wei Yongjun wrote: > > From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > > > > In case of error, the function kthread_run() returns ERR_PTR() > > and never returns NULL. The NULL test in the return value check > > should be replaced with IS_ERR(). > > > > Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > > --- > > drivers/scsi/hpsa.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c > > index 891c86b..f413b14 100644 > > --- a/drivers/scsi/hpsa.c > > +++ b/drivers/scsi/hpsa.c > > @@ -4757,7 +4757,8 @@ static void start_controller_lockup_detector(struct ctlr_info *h) > > kthread_run(detect_controller_lockup_thread, > > NULL, HPSA); > > } > > - if (!hpsa_lockup_detector) { > > + if (IS_ERR(hpsa_lockup_detector)) { > > + hpsa_lockup_detector = NULL; > > dev_warn(&h->pdev->dev, > > "Could not start lockup detector thread\n"); > > return; > > Ack. Hmm, this whole lockup thread start/stop thing is horribly racy with hotplug (and bind and unbind). Firstly, why do you need an actual thread, why not just use a workqueue? That way you don't need a list or a lock, you can just schedule work for each instance? since they don't interfere, no global list and no lock. If you insist on a thread, it should probably start at module insertion and be shut down at module removal, so no races. You can have it sleep forever on list_empty(&hpsa_ctlr_list) and wake it when you add a controller. James ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] [SCSI] hpsa: fix return value check in start_controller_lockup_detector() 2013-10-29 21:52 ` James Bottomley @ 2013-10-30 13:33 ` scameron 0 siblings, 0 replies; 4+ messages in thread From: scameron @ 2013-10-30 13:33 UTC (permalink / raw) To: James Bottomley Cc: Wei Yongjun, yongjun_wei, iss_storagedev, linux-scsi, scameron On Tue, Oct 29, 2013 at 02:52:38PM -0700, James Bottomley wrote: > On Tue, 2013-10-29 at 11:09 -0500, scameron@beardog.cce.hp.com wrote: > > On Tue, Oct 29, 2013 at 02:43:56PM +0800, Wei Yongjun wrote: > > > From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > > > > > > In case of error, the function kthread_run() returns ERR_PTR() > > > and never returns NULL. The NULL test in the return value check > > > should be replaced with IS_ERR(). > > > > > > Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > > > --- > > > drivers/scsi/hpsa.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c > > > index 891c86b..f413b14 100644 > > > --- a/drivers/scsi/hpsa.c > > > +++ b/drivers/scsi/hpsa.c > > > @@ -4757,7 +4757,8 @@ static void start_controller_lockup_detector(struct ctlr_info *h) > > > kthread_run(detect_controller_lockup_thread, > > > NULL, HPSA); > > > } > > > - if (!hpsa_lockup_detector) { > > > + if (IS_ERR(hpsa_lockup_detector)) { > > > + hpsa_lockup_detector = NULL; > > > dev_warn(&h->pdev->dev, > > > "Could not start lockup detector thread\n"); > > > return; > > > > Ack. > > Hmm, this whole lockup thread start/stop thing is horribly racy with > hotplug (and bind and unbind). > > Firstly, why do you need an actual thread, why not just use a workqueue? > That way you don't need a list or a lock, you can just schedule work for > each instance? since they don't interfere, no global list and no lock. > > If you insist on a thread, it should probably start at module insertion > and be shut down at module removal, so no races. You can have it sleep > forever on list_empty(&hpsa_ctlr_list) and wake it when you add a > controller. Alright, I'll work on it. Thanks. -- steve ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-10-30 13:33 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-10-29 6:43 [PATCH] [SCSI] hpsa: fix return value check in start_controller_lockup_detector() Wei Yongjun 2013-10-29 16:09 ` scameron 2013-10-29 21:52 ` James Bottomley 2013-10-30 13:33 ` scameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).