* [PATCH for kernel 3.19] Avoid that scsi_device_put() triggers a kernel warning @ 2015-01-05 11:03 Bart Van Assche 2015-01-05 13:02 ` Christoph Hellwig 2015-01-18 16:22 ` James Bottomley 0 siblings, 2 replies; 5+ messages in thread From: Bart Van Assche @ 2015-01-05 11:03 UTC (permalink / raw) To: James Bottomley, Christoph Hellwig Cc: Hannes Reinecke, linux-scsi@vger.kernel.org Avoid that the following warning is reported when a SCSI LLD kernel module is unloaded: WARNING: CPU: 5 PID: 228 at kernel/module.c:954 module_put+0x207/0x220() Call Trace: [<ffffffff814d1fcf>] dump_stack+0x4c/0x65 [<ffffffff81053ada>] warn_slowpath_common+0x8a/0xc0 [<ffffffff81053bca>] warn_slowpath_null+0x1a/0x20 [<ffffffff810d0507>] module_put+0x207/0x220 [<ffffffffa000bea8>] scsi_device_put+0x48/0x50 [scsi_mod] [<ffffffffa03676d2>] scsi_disk_put+0x32/0x50 [sd_mod] [<ffffffffa0368d4c>] sd_shutdown+0x8c/0x150 [sd_mod] [<ffffffffa0368e79>] sd_remove+0x69/0xc0 [sd_mod] [<ffffffff813457ef>] __device_release_driver+0x7f/0xf0 [<ffffffff81345885>] device_release_driver+0x25/0x40 [<ffffffff81345134>] bus_remove_device+0x124/0x1b0 [<ffffffff8134189e>] device_del+0x13e/0x250 [<ffffffffa001cdcd>] __scsi_remove_device+0xcd/0xe0 [scsi_mod] [<ffffffffa001b39f>] scsi_forget_host+0x6f/0x80 [scsi_mod] [<ffffffffa000d5f6>] scsi_remove_host+0x86/0x140 [scsi_mod] [<ffffffffa07d5c0b>] srp_remove_work+0x9b/0x210 [ib_srp] [<ffffffff8106fd28>] process_one_work+0x1d8/0x780 [<ffffffff810703eb>] worker_thread+0x11b/0x4a0 [<ffffffff81075a6f>] kthread+0xef/0x110 [<ffffffff814dad6c>] ret_from_fork+0x7c/0xb0 See also patch "module: Remove stop_machine from module unloading" (Masami Hiramatsu; commit e513cc1c07e2; kernel v3.19-rc1). Signed-off-by: Bart Van Assche <bvanassche@acm.org> Cc: Christoph Hellwig <hch@lst.de> Cc: Hannes Reinecke <hare@suse.de> --- drivers/scsi/hosts.c | 1 + drivers/scsi/scsi.c | 13 ++++--------- include/scsi/scsi_host.h | 3 +++ 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c index 8bb173e..e9155d0 100644 --- a/drivers/scsi/hosts.c +++ b/drivers/scsi/hosts.c @@ -380,6 +380,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize) shost->host_lock = &shost->default_lock; spin_lock_init(shost->host_lock); + atomic_set(&shost->module_refcnt, 0); shost->shost_state = SHOST_CREATED; INIT_LIST_HEAD(&shost->__devices); INIT_LIST_HEAD(&shost->__targets); diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index e028854..ed325d1 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -988,7 +988,8 @@ int scsi_device_get(struct scsi_device *sdev) return -ENXIO; /* We can fail this if we're doing SCSI operations * from module exit (like cache flush) */ - try_module_get(sdev->host->hostt->module); + if (try_module_get(sdev->host->hostt->module)) + atomic_inc(&sdev->host->module_refcnt); return 0; } @@ -1004,14 +1005,8 @@ EXPORT_SYMBOL(scsi_device_get); */ void scsi_device_put(struct scsi_device *sdev) { -#ifdef CONFIG_MODULE_UNLOAD - struct module *module = sdev->host->hostt->module; - - /* The module refcount will be zero if scsi_device_get() - * was called from a module removal routine */ - if (module && module_refcount(module) != 0) - module_put(module); -#endif + if (atomic_dec_if_positive(&sdev->host->module_refcnt) >= 0) + module_put(sdev->host->hostt->module); put_device(&sdev->sdev_gendev); } EXPORT_SYMBOL(scsi_device_put); diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index 019e668..b8e6f01 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -566,6 +566,9 @@ struct Scsi_Host { struct scsi_host_template *hostt; struct scsi_transport_template *transportt; + /* Number of LLD kernel module references held by the SCSI core. */ + atomic_t module_refcnt; + /* * Area to keep a shared tag map (if needed, will be * NULL if not). -- 2.1.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH for kernel 3.19] Avoid that scsi_device_put() triggers a kernel warning 2015-01-05 11:03 [PATCH for kernel 3.19] Avoid that scsi_device_put() triggers a kernel warning Bart Van Assche @ 2015-01-05 13:02 ` Christoph Hellwig 2015-01-18 15:29 ` Christoph Hellwig 2015-01-18 16:22 ` James Bottomley 1 sibling, 1 reply; 5+ messages in thread From: Christoph Hellwig @ 2015-01-05 13:02 UTC (permalink / raw) To: Bart Van Assche Cc: James Bottomley, Christoph Hellwig, Hannes Reinecke, linux-scsi@vger.kernel.org I don't like this. The problem is that sd_shutdown shouldn't even try to grab a reference to the scsi device, nevermind the HBA module. So I'd say this need a two step fix: (1) stop trying to call try_module_get from sd_shutdown (2) properly propagate the try_module_get return value ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for kernel 3.19] Avoid that scsi_device_put() triggers a kernel warning 2015-01-05 13:02 ` Christoph Hellwig @ 2015-01-18 15:29 ` Christoph Hellwig 0 siblings, 0 replies; 5+ messages in thread From: Christoph Hellwig @ 2015-01-18 15:29 UTC (permalink / raw) To: Christoph Hellwig Cc: Bart Van Assche, James Bottomley, Hannes Reinecke, Don Brace, Alan Stern, linux-scsi@vger.kernel.org On Mon, Jan 05, 2015 at 02:02:59PM +0100, Christoph Hellwig wrote: > I don't like this. The problem is that sd_shutdown shouldn't even try > to grab a reference to the scsi device, nevermind the HBA module. We're fairly late in th 3.19 cycle, and it seems like the proper fix for the root cause might take a while. Thus I'm tempte to add this as the simplest band aid and sort out the root cause for 3.20. Does anyone else have a better idea? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for kernel 3.19] Avoid that scsi_device_put() triggers a kernel warning 2015-01-05 11:03 [PATCH for kernel 3.19] Avoid that scsi_device_put() triggers a kernel warning Bart Van Assche 2015-01-05 13:02 ` Christoph Hellwig @ 2015-01-18 16:22 ` James Bottomley 2015-01-18 16:47 ` James Bottomley 1 sibling, 1 reply; 5+ messages in thread From: James Bottomley @ 2015-01-18 16:22 UTC (permalink / raw) To: bvanassche@acm.org Cc: hch@lst.de, linux-scsi@vger.kernel.org, rusty@rustcorp.com.au, hare@suse.de, masami.hiramatsu.pt@hitachi.com On Mon, 2015-01-05 at 12:03 +0100, Bart Van Assche wrote: > Avoid that the following warning is reported when a SCSI LLD kernel > module is unloaded: > > WARNING: CPU: 5 PID: 228 at kernel/module.c:954 module_put+0x207/0x220() > Call Trace: > [<ffffffff814d1fcf>] dump_stack+0x4c/0x65 > [<ffffffff81053ada>] warn_slowpath_common+0x8a/0xc0 > [<ffffffff81053bca>] warn_slowpath_null+0x1a/0x20 > [<ffffffff810d0507>] module_put+0x207/0x220 > [<ffffffffa000bea8>] scsi_device_put+0x48/0x50 [scsi_mod] > [<ffffffffa03676d2>] scsi_disk_put+0x32/0x50 [sd_mod] > [<ffffffffa0368d4c>] sd_shutdown+0x8c/0x150 [sd_mod] > [<ffffffffa0368e79>] sd_remove+0x69/0xc0 [sd_mod] > [<ffffffff813457ef>] __device_release_driver+0x7f/0xf0 > [<ffffffff81345885>] device_release_driver+0x25/0x40 > [<ffffffff81345134>] bus_remove_device+0x124/0x1b0 > [<ffffffff8134189e>] device_del+0x13e/0x250 > [<ffffffffa001cdcd>] __scsi_remove_device+0xcd/0xe0 [scsi_mod] > [<ffffffffa001b39f>] scsi_forget_host+0x6f/0x80 [scsi_mod] > [<ffffffffa000d5f6>] scsi_remove_host+0x86/0x140 [scsi_mod] > [<ffffffffa07d5c0b>] srp_remove_work+0x9b/0x210 [ib_srp] > [<ffffffff8106fd28>] process_one_work+0x1d8/0x780 > [<ffffffff810703eb>] worker_thread+0x11b/0x4a0 > [<ffffffff81075a6f>] kthread+0xef/0x110 > [<ffffffff814dad6c>] ret_from_fork+0x7c/0xb0 > > See also patch "module: Remove stop_machine from module unloading" > (Masami Hiramatsu; commit e513cc1c07e2; kernel v3.19-rc1). > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Hannes Reinecke <hare@suse.de> > --- > drivers/scsi/hosts.c | 1 + > drivers/scsi/scsi.c | 13 ++++--------- > include/scsi/scsi_host.h | 3 +++ > 3 files changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c > index 8bb173e..e9155d0 100644 > --- a/drivers/scsi/hosts.c > +++ b/drivers/scsi/hosts.c > @@ -380,6 +380,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize) > > shost->host_lock = &shost->default_lock; > spin_lock_init(shost->host_lock); > + atomic_set(&shost->module_refcnt, 0); > shost->shost_state = SHOST_CREATED; > INIT_LIST_HEAD(&shost->__devices); > INIT_LIST_HEAD(&shost->__targets); > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c > index e028854..ed325d1 100644 > --- a/drivers/scsi/scsi.c > +++ b/drivers/scsi/scsi.c > @@ -988,7 +988,8 @@ int scsi_device_get(struct scsi_device *sdev) > return -ENXIO; > /* We can fail this if we're doing SCSI operations > * from module exit (like cache flush) */ > - try_module_get(sdev->host->hostt->module); > + if (try_module_get(sdev->host->hostt->module)) > + atomic_inc(&sdev->host->module_refcnt); > > return 0; > } > @@ -1004,14 +1005,8 @@ EXPORT_SYMBOL(scsi_device_get); > */ > void scsi_device_put(struct scsi_device *sdev) > { > -#ifdef CONFIG_MODULE_UNLOAD > - struct module *module = sdev->host->hostt->module; > - > - /* The module refcount will be zero if scsi_device_get() > - * was called from a module removal routine */ > - if (module && module_refcount(module) != 0) > - module_put(module); > -#endif > + if (atomic_dec_if_positive(&sdev->host->module_refcnt) >= 0) > + module_put(sdev->host->hostt->module); > put_device(&sdev->sdev_gendev); > } > EXPORT_SYMBOL(scsi_device_put); > diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h > index 019e668..b8e6f01 100644 > --- a/include/scsi/scsi_host.h > +++ b/include/scsi/scsi_host.h > @@ -566,6 +566,9 @@ struct Scsi_Host { > struct scsi_host_template *hostt; > struct scsi_transport_template *transportt; > > + /* Number of LLD kernel module references held by the SCSI core. */ > + atomic_t module_refcnt; > + > /* > * Area to keep a shared tag map (if needed, will be > * NULL if not). This is a bit over coded for a fix. We don't need to duplicate the module reference, merely take into account that in the remove code module_refcnt() can now return -1, which we're not expecting. Masami, this looks like a nasty unintended consequence of commit e513cc1c07e2ab93a4514eec9833e031df3e30bb Author: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> Date: Mon Nov 10 09:30:29 2014 +1030 module: Remove stop_machine from module unloading Are you sure we all should have to alter our module refcounting in removal routines like this? I think the better fix is having module_refcount() never return a negative number: unsigned long module_refcount(struct module *mod) { unsigned long ret = (unsigned long)atomic_read(&mod->refcnt); if (ret == 0) return ret return ret - MODULE_REF_BASE; } James --- diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index e028854..90dcb2f 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -1009,7 +1009,7 @@ void scsi_device_put(struct scsi_device *sdev) /* The module refcount will be zero if scsi_device_get() * was called from a module removal routine */ - if (module && module_refcount(module) != 0) + if (module && module_refcount(module) > 0) module_put(module); #endif put_device(&sdev->sdev_gendev); ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH for kernel 3.19] Avoid that scsi_device_put() triggers a kernel warning 2015-01-18 16:22 ` James Bottomley @ 2015-01-18 16:47 ` James Bottomley 0 siblings, 0 replies; 5+ messages in thread From: James Bottomley @ 2015-01-18 16:47 UTC (permalink / raw) To: bvanassche@acm.org Cc: hch@lst.de, linux-scsi@vger.kernel.org, rusty@rustcorp.com.au, hare@suse.de, masami.hiramatsu.pt@hitachi.com On Sun, 2015-01-18 at 16:22 +0000, James Bottomley wrote: > On Mon, 2015-01-05 at 12:03 +0100, Bart Van Assche wrote: > > Avoid that the following warning is reported when a SCSI LLD kernel > > module is unloaded: > > > > WARNING: CPU: 5 PID: 228 at kernel/module.c:954 module_put+0x207/0x220() > > Call Trace: > > [<ffffffff814d1fcf>] dump_stack+0x4c/0x65 > > [<ffffffff81053ada>] warn_slowpath_common+0x8a/0xc0 > > [<ffffffff81053bca>] warn_slowpath_null+0x1a/0x20 > > [<ffffffff810d0507>] module_put+0x207/0x220 > > [<ffffffffa000bea8>] scsi_device_put+0x48/0x50 [scsi_mod] > > [<ffffffffa03676d2>] scsi_disk_put+0x32/0x50 [sd_mod] > > [<ffffffffa0368d4c>] sd_shutdown+0x8c/0x150 [sd_mod] > > [<ffffffffa0368e79>] sd_remove+0x69/0xc0 [sd_mod] > > [<ffffffff813457ef>] __device_release_driver+0x7f/0xf0 > > [<ffffffff81345885>] device_release_driver+0x25/0x40 > > [<ffffffff81345134>] bus_remove_device+0x124/0x1b0 > > [<ffffffff8134189e>] device_del+0x13e/0x250 > > [<ffffffffa001cdcd>] __scsi_remove_device+0xcd/0xe0 [scsi_mod] > > [<ffffffffa001b39f>] scsi_forget_host+0x6f/0x80 [scsi_mod] > > [<ffffffffa000d5f6>] scsi_remove_host+0x86/0x140 [scsi_mod] > > [<ffffffffa07d5c0b>] srp_remove_work+0x9b/0x210 [ib_srp] > > [<ffffffff8106fd28>] process_one_work+0x1d8/0x780 > > [<ffffffff810703eb>] worker_thread+0x11b/0x4a0 > > [<ffffffff81075a6f>] kthread+0xef/0x110 > > [<ffffffff814dad6c>] ret_from_fork+0x7c/0xb0 > > > > See also patch "module: Remove stop_machine from module unloading" > > (Masami Hiramatsu; commit e513cc1c07e2; kernel v3.19-rc1). > > > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > > Cc: Christoph Hellwig <hch@lst.de> > > Cc: Hannes Reinecke <hare@suse.de> > > --- > > drivers/scsi/hosts.c | 1 + > > drivers/scsi/scsi.c | 13 ++++--------- > > include/scsi/scsi_host.h | 3 +++ > > 3 files changed, 8 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c > > index 8bb173e..e9155d0 100644 > > --- a/drivers/scsi/hosts.c > > +++ b/drivers/scsi/hosts.c > > @@ -380,6 +380,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize) > > > > shost->host_lock = &shost->default_lock; > > spin_lock_init(shost->host_lock); > > + atomic_set(&shost->module_refcnt, 0); > > shost->shost_state = SHOST_CREATED; > > INIT_LIST_HEAD(&shost->__devices); > > INIT_LIST_HEAD(&shost->__targets); > > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c > > index e028854..ed325d1 100644 > > --- a/drivers/scsi/scsi.c > > +++ b/drivers/scsi/scsi.c > > @@ -988,7 +988,8 @@ int scsi_device_get(struct scsi_device *sdev) > > return -ENXIO; > > /* We can fail this if we're doing SCSI operations > > * from module exit (like cache flush) */ > > - try_module_get(sdev->host->hostt->module); > > + if (try_module_get(sdev->host->hostt->module)) > > + atomic_inc(&sdev->host->module_refcnt); > > > > return 0; > > } > > @@ -1004,14 +1005,8 @@ EXPORT_SYMBOL(scsi_device_get); > > */ > > void scsi_device_put(struct scsi_device *sdev) > > { > > -#ifdef CONFIG_MODULE_UNLOAD > > - struct module *module = sdev->host->hostt->module; > > - > > - /* The module refcount will be zero if scsi_device_get() > > - * was called from a module removal routine */ > > - if (module && module_refcount(module) != 0) > > - module_put(module); > > -#endif > > + if (atomic_dec_if_positive(&sdev->host->module_refcnt) >= 0) > > + module_put(sdev->host->hostt->module); > > put_device(&sdev->sdev_gendev); > > } > > EXPORT_SYMBOL(scsi_device_put); > > diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h > > index 019e668..b8e6f01 100644 > > --- a/include/scsi/scsi_host.h > > +++ b/include/scsi/scsi_host.h > > @@ -566,6 +566,9 @@ struct Scsi_Host { > > struct scsi_host_template *hostt; > > struct scsi_transport_template *transportt; > > > > + /* Number of LLD kernel module references held by the SCSI core. */ > > + atomic_t module_refcnt; > > + > > /* > > * Area to keep a shared tag map (if needed, will be > > * NULL if not). > > This is a bit over coded for a fix. We don't need to duplicate the > module reference, merely take into account that in the remove code > module_refcnt() can now return -1, which we're not expecting. > > Masami, this looks like a nasty unintended consequence of > > commit e513cc1c07e2ab93a4514eec9833e031df3e30bb > Author: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> > Date: Mon Nov 10 09:30:29 2014 +1030 > > module: Remove stop_machine from module unloading > > Are you sure we all should have to alter our module refcounting in > removal routines like this? I think the better fix is having > module_refcount() never return a negative number: > > unsigned long module_refcount(struct module *mod) > { > unsigned long ret = (unsigned long)atomic_read(&mod->refcnt); > > if (ret == 0) > return ret > return ret - MODULE_REF_BASE; > } > > James > > --- > > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c > index e028854..90dcb2f 100644 > --- a/drivers/scsi/scsi.c > +++ b/drivers/scsi/scsi.c > @@ -1009,7 +1009,7 @@ void scsi_device_put(struct scsi_device *sdev) > > /* The module refcount will be zero if scsi_device_get() > * was called from a module removal routine */ > - if (module && module_refcount(module) != 0) > + if (module && module_refcount(module) > 0) > module_put(module); > #endif > put_device(&sdev->sdev_gendev); Actually, thought about it some more. The above won't work because module_refcount() is returning an unsigned type, so -1 is actually the largest integer. I think this indicates the proper fix is in module_refcount(), I'll code it up and send it in. James ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-01-18 16:47 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-01-05 11:03 [PATCH for kernel 3.19] Avoid that scsi_device_put() triggers a kernel warning Bart Van Assche 2015-01-05 13:02 ` Christoph Hellwig 2015-01-18 15:29 ` Christoph Hellwig 2015-01-18 16:22 ` James Bottomley 2015-01-18 16:47 ` James Bottomley
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.