Avoid that scsi_device_put() triggers a kernel warning
PATCH 1/1 Avoid that scsi_device_put() triggers a kernel warning
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).
--- drivers/scsi/hosts.c | 1 + drivers/scsi/scsi.c | 13 ++++--------- include/scsi/scsi_host.h | 3 +++ 3 files changed, 8 insertions(+), 9 deletions(-)
drivers/scsi/hosts.c +1 -0
@@ -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);
drivers/scsi/scsi.c +4 -9
@@ -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);
include/scsi/scsi_host.h +3 -0
@@ -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).
Signature · 1 line
2.1.2
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
Quoted 2 lines · Christoph Hellwig
I don't like this. The problem is that
sd_shutdownshouldn'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?
Quoted 86 lines · Bart Van Assche
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/0xb0See also patch "module: Remove
stop_machinefrom module unloading" (Masami Hiramatsu; commite513cc1c07e2; kernel v3.19-rc1).Signed-off-by Bart Van Assche <[email protected]>Cc Christoph Hellwig <[email protected]>Cc Hannes Reinecke <[email protected]>--- drivers/scsi/hosts.c | 1 + drivers/scsi/scsi.c | 13 ++++--------- include/scsi/scsi_host.h | 3 +++ 3 files changed, 8 insertions(+), 9 deletions(-)drivers/scsi/hosts.c +1
-0@@ -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);drivers/scsi/scsi.c +4
-9@@ -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);include/scsi/scsi_host.h +0
-0
include/scsi/scsi_host.h struct Scsi_Host { · line 574
Quoted 9 lines
include/scsi/scsi_host.h +3
-0@@ -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 <[email protected]> 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
---
drivers/scsi/scsi.c +1 -1
@@ -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);
James Bottomley
maintainerQuoted 131 lines · James Bottomley
Quoted 86 lines · Bart Van Assche
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/0xb0See also patch "module: Remove
stop_machinefrom module unloading" (Masami Hiramatsu; commite513cc1c07e2; kernel v3.19-rc1).Signed-off-by Bart Van Assche <[email protected]>Cc Christoph Hellwig <[email protected]>Cc Hannes Reinecke <[email protected]>--- drivers/scsi/hosts.c | 1 + drivers/scsi/scsi.c | 13 ++++--------- include/scsi/scsi_host.h | 3 +++ 3 files changed, 8 insertions(+), 9 deletions(-)drivers/scsi/hosts.c +1
-0@@ -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);drivers/scsi/scsi.c +4
-9@@ -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);include/scsi/scsi_host.h +0
-0include/scsi/scsi_host.h struct Scsi_Host { · line 574
Quoted 9 lines
include/scsi/scsi_host.h +3
-0@@ -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
e513cc1c07e2ab93a4514eec9833e031df3e30bbAuthor: Masami Hiramatsu <[email protected]> Date: Mon Nov 10 09:30:29 2014 +1030module: Remove
stop_machinefrom module unloadingAre 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
---
drivers/scsi/scsi.c +0
-0
drivers/scsi/scsi.c void scsi_device_put(struct scsi_device *sdev) · line 1015
Quoted 8 lines
drivers/scsi/scsi.c +1
-1@@ -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 inmodule_refcount(), I'll code it up and send it in.
James