[PATCH] edac, poll timeout cannot be zero

[PATCH] edac, poll timeout cannot be zero

[Prarit Bhargava]

If you do

echo 0 > /sys/module/edac_core/parameters/edac_mc_poll_msec

the following stack trace is output because the edac module is not
designed to poll with a timeout of zero.

------------[ cut here ]------------
WARNING: CPU: 12 PID: 0 at lib/list_debug.c:33 __list_add+0xac/0xc0()
list_add corruption. prev->next should be next (ffff8808291dd1b8), but was           (null). (prev=ffff8808286fe3f8).
Modules linked in: sg nfsv3 rpcsec_gss_krb5 nfsv4 dns_resolver nfs fscache cfg80211 rfkill x86_pkg_temp_thermal coretemp kvm_intel kvm ixgbe e1000e crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel aesni_intel lrw gf128mul glue_helper ablk_helper cryptd iTCO_wdt ptp sb_edac iTCO_vendor_support pps_core mdio ipmi_devintf edac_core ioatdma microcode shpchp lpc_ich pcspkr i2c_i801 dca mfd_core ipmi_si wmi ipmi_msghandler nfsd auth_rpcgss nfs_acl lockd sunrpc xfs libcrc32c sd_mod sr_mod cdrom crc_t10dif crct10dif_common mgag200 syscopyarea sysfillrect sysimgblt isci i2c_algo_bit drm_kms_helper ttm drm libsas ahci libahci scsi_transport_sas libata i2c_core dm_mirror dm_region_hash dm_log dm_mod
CPU: 12 PID: 0 Comm: swapper/12 Not tainted 3.13.0+ #1
Hardware name: Intel Corporation LH Pass ........../SVRBD-ROW_T, BIOS SE5C600.86B.01.08.0003.022620131521 02/26/2013
 0000000000000009 ffff88082f683cc8 ffffffff815f1945 ffff88082f683d10
 ffff88082f683d00 ffffffff81069dad ffff880826f9b248 ffff8808291dd1b8
 ffff8808286fe3f8 ffff8808291dc000 0000000000000000 ffff88082f683d60
Call Trace:
 <IRQ>  [<ffffffff815f1945>] dump_stack+0x45/0x56
 [<ffffffff81069dad>] warn_slowpath_common+0x7d/0xa0
 [<ffffffff81069e1c>] warn_slowpath_fmt+0x4c/0x50
 [<ffffffff812dd0fc>] __list_add+0xac/0xc0
 [<ffffffff8107570b>] __internal_add_timer+0xab/0x130
 [<ffffffff81075907>] internal_add_timer+0x17/0x40
 [<ffffffff81077dfa>] mod_timer_pinned+0xca/0x170
 [<ffffffff8149f7f0>] ? pid_param_set+0x130/0x130
 [<ffffffff8149fa7a>] intel_pstate_timer_func+0x28a/0x380
 [<ffffffff8149f7f0>] ? pid_param_set+0x130/0x130
 [<ffffffff810757d6>] call_timer_fn+0x36/0x100
 [<ffffffff8149f7f0>] ? pid_param_set+0x130/0x130
 [<ffffffff810767ff>] run_timer_softirq+0x1ff/0x2f0
 [<ffffffff8106f3b5>] __do_softirq+0xf5/0x2e0
 [<ffffffff8106f87d>] irq_exit+0x10d/0x120
 [<ffffffff816042b5>] smp_apic_timer_interrupt+0x45/0x60
 [<ffffffff81602c1d>] apic_timer_interrupt+0x6d/0x80
 <EOI>  [<ffffffff814a022f>] ? cpuidle_enter_state+0x4f/0xc0
 [<ffffffff814a0228>] ? cpuidle_enter_state+0x48/0xc0
 [<ffffffff814a0359>] cpuidle_idle_call+0xb9/0x1f0
 [<ffffffff8101e44e>] arch_cpu_idle+0xe/0x30
 [<ffffffff810c327e>] cpu_startup_entry+0x9e/0x240
 [<ffffffff81043544>] start_secondary+0x1e4/0x290
---[ end trace 078b214fc68689e6 ]---
------------[ cut here ]------------
------------[ cut here ]------------
kernel BUG at kernel/timer.c:1084!
invalid opcode: 0000 [#1] SMP
Modules linked in: sg nfsv3 rpcsec_gss_krb5 nfsv4 dns_resolver nfs fscache cfg80211 rfkill x86_pkg_temp_thermal coretemp kvm_intel kvm ixgbe e1000e crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel aesni_intel lrw gf128mul glue_helper ablk_helper cryptd iTCO_wdt ptp sb_edac iTCO_vendor_support pps_core mdio ipmi_devintf edac_core ioatdma microcode shpchp lpc_ich pcspkr i2c_i801 dca mfd_core ipmi_si wmi ipmi_msghandler nfsd auth_rpcgss nfs_acl lockd sunrpc xfs libcrc32c sd_mod sr_mod cdrom crc_t10dif crct10dif_common mgag200 syscopyarea sysfillrect sysimgblt isci i2c_algo_bit drm_kms_helper ttm drm libsas ahci libahci scsi_transport_sas libata i2c_core dm_mirror dm_region_hash dm_log dm_mod
CPU: 12 PID: 0 Comm: swapper/12 Tainted: G        W    3.13.0+ #1
Hardware name: Intel Corporation LH Pass ........../SVRBD-ROW_T, BIOS SE5C600.86B.01.08.0003.022620131521 02/26/2013
task: ffff880428eadd30 ti: ffff880428eb8000 task.ti: ffff880428eb8000
RIP: 0010:[<ffffffff810759c3>]  [<ffffffff810759c3>] cascade+0x93/0xa0
RSP: 0018:ffff88082f683e70  EFLAGS: 00010083
RAX: ffffffff81c64580 RBX: 0000000000000000 RCX: ffff88082f683ed8
RDX: 0000000000000019 RSI: ffff8808286f83f8 RDI: ffff8808291dc000
RBP: ffff88082f683ea0 R08: ffff8808291dd3b8 R09: 0000000000000100
R10: 0000000000000000 R11: ffff88082f6839fe R12: ffff8808291dc000
R13: ffff88082f683e70 R14: 0000000000000019 R15: ffff88082f683ed8
FS:  0000000000000000(0000) GS:ffff88082f680000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000004a4fb0 CR3: 00000000018f8000 CR4: 00000000000407e0
Stack:
 ffff8808286f83f8 ffff880826f9b248 ffff8808291dc000 0000000000000000
 ffffffff81083c10 0000000000000002 ffff88082f683f10 ffffffff81076845
 ffff8808291ddc28 ffff8808291dd828 ffff8808291dd428 ffff8808291dd028
Call Trace:
 <IRQ>
 [<ffffffff81083c10>] ? __queue_work+0x320/0x320
 [<ffffffff81076845>] run_timer_softirq+0x245/0x2f0
 [<ffffffff8106f3b5>] __do_softirq+0xf5/0x2e0
 [<ffffffff8106f87d>] irq_exit+0x10d/0x120
 [<ffffffff816042b5>] smp_apic_timer_interrupt+0x45/0x60
 [<ffffffff81602c1d>] apic_timer_interrupt+0x6d/0x80
 <EOI>
 [<ffffffff814a022f>] ? cpuidle_enter_state+0x4f/0xc0
 [<ffffffff814a0228>] ? cpuidle_enter_state+0x48/0xc0
 [<ffffffff814a0359>] cpuidle_idle_call+0xb9/0x1f0
 [<ffffffff8101e44e>] arch_cpu_idle+0xe/0x30
 [<ffffffff810c327e>] cpu_startup_entry+0x9e/0x240
 [<ffffffff81043544>] start_secondary+0x1e4/0x290
Code: 49 39 cc 75 26 48 89 de 48 89 c3 4c 89 e7 e8 b5 fc ff ff 4c 39 eb 48 8b 03 75 dd 48 83 c4 10 44 89 f0 5b 41 5c 41 5d 41 5e 5d c3 <0f> 0b 66 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66 90 55 48 89
RIP  [<ffffffff810759c3>] cascade+0x93/0xa0
 RSP <ffff88082f683e70>
WARNING: CPU: 36 PID: 1154 at kernel/workqueue.c:1461 __queue_delayed_work+0xed/0x1a0()
[ 3108.665556] Modules linked in: sg nfsv3 rpcsec_gss_krb5 nfsv4 dns_resolver nfs fscache cfg80211 rfkill x86_pkg_temp_thermal coretemp kvm_intel kvm ixgbe e1000e crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel aesni_intel lrw gf128mul glue_helper ablk_helper cryptd iTCO_wdt ptp sb_edac iTCO_vendor_support pps_core mdio ipmi_devintf edac_core ioatdma microcode shpchp lpc_ich pcspkr i2c_i801 dca mfd_core ipmi_si wmi ipmi_msghandler nfsd auth_rpcgss nfs_acl lockd sunrpc xfs libcrc32c sd_mod sr_mod cdrom crc_t10dif crct10dif_common mgag200 syscopyarea sysfillrect sysimgblt isci i2c_algo_bit drm_kms_helper ttm drm libsas ahci libahci scsi_transport_sas libata i2c_core dm_mirror dm_region_hash dm_log dm_mod
[ 3108.737843] CPU: 36 PID: 1154 Comm: kworker/u481:3 Tainted: G        W    3.13.0+ #1
[ 3108.746480] Hardware name: Intel Corporation LH Pass ........../SVRBD-ROW_T, BIOS SE5C600.86B.01.08.0003.022620131521 02/26/2013
[ 3108.759435] Workqueue: edac-poller edac_mc_workq_function [edac_core]
[ 3108.766630]  0000000000000009 ffff880425137d70 ffffffff815f1945 0000000000000000
[ 3108.774932]  ffff880425137da8 ffffffff81069dad 0000000000002000 ffff880826bb1200
[ 3108.783230]  ffff8808286fe3f8 ffff880036371000 0000000000003c20 ffff880425137db8
[ 3108.791521] Call Trace:
[ 3108.794265]  [<ffffffff815f1945>] dump_stack+0x45/0x56
[ 3108.800015]  [<ffffffff81069dad>] warn_slowpath_common+0x7d/0xa0
[ 3108.806732]  [<ffffffff81069e8a>] warn_slowpath_null+0x1a/0x20
[ 3108.813238]  [<ffffffff81083d1d>] __queue_delayed_work+0xed/0x1a0
[ 3108.820034]  [<ffffffff81084077>] queue_delayed_work_on+0x27/0x50
[ 3108.826831]  [<ffffffffa14b0392>] edac_mc_workq_function+0x72/0xa0 [edac_core]
[ 3108.834908]  [<ffffffff81085c3b>] process_one_work+0x17b/0x460
[ 3108.841414]  [<ffffffff810869db>] worker_thread+0x11b/0x400
[ 3108.847635]  [<ffffffff810868c0>] ? rescuer_thread+0x3e0/0x3e0
[ 3108.854143]  [<ffffffff8108d612>] kthread+0xd2/0xf0
[ 3108.859584]  [<ffffffff8108d540>] ? kthread_create_on_node+0x180/0x180
[ 3108.866873]  [<ffffffff81601efc>] ret_from_fork+0x7c/0xb0
[ 3108.872896]  [<ffffffff8108d540>] ? kthread_create_on_node+0x180/0x180
[ 3108.880176] ---[ end trace 078b214fc68689e7 ]---

This patch adds a range check in the edac_mc_poll_msec code to check for 0.

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Cc: Doug Thompson <dougthompson@xmission.com>
Cc: linux-kernel@vger.kernel.org
---
 drivers/edac/edac_mc_sysfs.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 51c0362..8ec1747 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -61,7 +61,7 @@ static int edac_set_poll_msec(const char *val, struct kernel_param *kp)
 	ret = kstrtol(val, 0, &l);
 	if (ret)
 		return ret;
-	if ((int)l != l)
+	if (!l || ((int)l != l))
 		return -EINVAL;
 	*((int *)kp->arg) = l;
 
-- 
1.7.9.3

Re: [PATCH] edac, poll timeout cannot be zero

[Borislav Petkov]

On Mon, Feb 03, 2014 at 03:05:13PM -0500, Prarit Bhargava wrote:
> If you do
> 
> echo 0 > /sys/module/edac_core/parameters/edac_mc_poll_msec
> 
> the following stack trace is output because the edac module is not
> designed to poll with a timeout of zero.

Ok, I took your patch and extended it a bit, see bottom of mail. We're
not allowing intervals lower than a second now because it doesn't make
any sense, IMO.

While testing, however, I keep seeing the splat below and that's:

        if (WARN_ON(!list_empty(&work->entry))) {
                spin_unlock(&pwq->pool->lock);
                return;
        }

and there seems to be some interference with edac_mc_workq_setup()
which does mod_delayed_work() and then the workqueue callback
edac_mc_workq_function() which does queue_delayed_work().

What I'm seeing in the splat is that when the timer fires to run the
delayed work, __queue_work() complains that the work list is not empty
even though we've done mod_delayed_work() which is supposed to cancel
any pending work.

Tejun, any ideas what's happening? Do we need synchronization here or do
you have a _sync version of mod_delayed_work() which makes sure any work
is cancelled?

Or does this mean that once the work is getting queued from the timer
callback delayed_work_timer_fn, it cannot be cancelled anymore? Or
something else I'm missing...?

Thanks.

[ 4143.086470] ------------[ cut here ]------------
[ 4143.094342] WARNING: CPU: 1 PID: 0 at kernel/workqueue.c:1393 __queue_work+0x1d7/0x340()
[ 4143.105683] Modules linked in: sb_edac edac_core ext2 vfat fat fuse loop dm_crypt dm_mod usbhid x86_pkg_temp_thermal coretemp kvm_intel kvm crc32_pclmul crc32c_intel ghash_clmulni_intel ehci_pci aesni_intel ehci_hcd aes_x86_64 xhci_hcd glue_helper snd_hda_codec_hdmi lrw usbcore gf128mul ablk_helper cryptd snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_codec microcode snd_hwdep snd_pcm iTCO_wdt snd_timer pcspkr iTCO_vendor_support evdev i2c_i801 lpc_ich usb_common button snd dcdbas mfd_core acpi_cpufreq soundcore processor [last unloaded: edac_core]
[ 4143.167227] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 3.14.0-rc2+ #3
[ 4143.177066] Hardware name: Dell Inc. Precision T3600/0PTTT9, BIOS A08 01/24/2013
[ 4143.187959]  0000000000000009 ffff88044ec43da8 ffffffff8163a5e0 0000000000000000
[ 4143.198898]  ffff88044ec43de0 ffffffff8104ae9d ffff88043a866100 ffff8804338b6428
[ 4143.209844]  0000000000000008 ffff88043a39f200 000000000000f128 ffff88044ec43df0
[ 4143.220808] Call Trace:
[ 4143.226686]  <IRQ>  [<ffffffff8163a5e0>] dump_stack+0x4d/0x66
[ 4143.235942]  [<ffffffff8104ae9d>] warn_slowpath_common+0x7d/0xa0
[ 4143.245407]  [<ffffffff8104af7a>] warn_slowpath_null+0x1a/0x20
[ 4143.254662]  [<ffffffff81066707>] __queue_work+0x1d7/0x340
[ 4143.263546]  [<ffffffff81066ed0>] ? execute_in_process_context+0xa0/0xa0
[ 4143.273625]  [<ffffffff81066eee>] delayed_work_timer_fn+0x1e/0x20
[ 4143.283091]  [<ffffffff81056cef>] call_timer_fn+0x7f/0x180
[ 4143.291939]  [<ffffffff81056c75>] ? call_timer_fn+0x5/0x180
[ 4143.300833]  [<ffffffff81066ed0>] ? execute_in_process_context+0xa0/0xa0
[ 4143.310866]  [<ffffffff81056f52>] run_timer_softirq+0x162/0x2b0
[ 4143.320117]  [<ffffffff810503ae>] __do_softirq+0x12e/0x300
[ 4143.328889]  [<ffffffff81050835>] irq_exit+0xa5/0xb0
[ 4143.337114]  [<ffffffff8164d925>] smp_apic_timer_interrupt+0x45/0x60
[ 4143.346711]  [<ffffffff8164c6ef>] apic_timer_interrupt+0x6f/0x80
[ 4143.355944]  <EOI>  [<ffffffff815202a4>] ? cpuidle_enter_state+0x54/0xc0
[ 4143.365927]  [<ffffffff815203d2>] cpuidle_idle_call+0xc2/0x220
[ 4143.374979]  [<ffffffff8100c00e>] arch_cpu_idle+0xe/0x30
[ 4143.383499]  [<ffffffff810a655a>] cpu_startup_entry+0xea/0x2c0
[ 4143.392524]  [<ffffffff8102f946>] start_secondary+0x1e6/0x240
[ 4143.401423] ---[ end trace a140660262786ef9 ]---

---
From: Prarit Bhargava <prarit@redhat.com>
Subject: [PATCH] EDAC: Poll timeout cannot be zero

Filter out 0 as it is an invalid poll timeout.

Boris: sanitize code even more to accept unsigned longs only and to
not allow polling intervals below 1 second as this is unnecessary and
doesn't make much sense for polling errors.

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Link: http://lkml.kernel.org/r/1391457913-881-1-git-send-email-prarit@redhat.com
Cc: Doug Thompson <dougthompson@xmission.com>
Cc: stable@vger.kernel.org
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 drivers/edac/edac_mc.c       |  4 ++--
 drivers/edac/edac_mc_sysfs.c | 10 ++++++----
 drivers/edac/edac_module.h   |  2 +-
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index e8c9ef03495b..aef5ec24908e 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -601,7 +601,7 @@ static void edac_mc_workq_teardown(struct mem_ctl_info *mci)
  *	user space has updated our poll period value, need to
  *	reset our workq delays
  */
-void edac_mc_reset_delay_period(int value)
+void edac_mc_reset_delay_period(unsigned long value)
 {
 	struct mem_ctl_info *mci;
 	struct list_head *item;
@@ -611,7 +611,7 @@ void edac_mc_reset_delay_period(int value)
 	list_for_each(item, &mc_devices) {
 		mci = list_entry(item, struct mem_ctl_info, link);
 
-		edac_mc_workq_setup(mci, (unsigned long) value);
+		edac_mc_workq_setup(mci, value);
 	}
 
 	mutex_unlock(&mem_ctls_mutex);
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 51c0362acf5c..b335c6ab5efe 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -52,18 +52,20 @@ int edac_mc_get_poll_msec(void)
 
 static int edac_set_poll_msec(const char *val, struct kernel_param *kp)
 {
-	long l;
+	unsigned long l;
 	int ret;
 
 	if (!val)
 		return -EINVAL;
 
-	ret = kstrtol(val, 0, &l);
+	ret = kstrtoul(val, 0, &l);
 	if (ret)
 		return ret;
-	if ((int)l != l)
+
+	if (l < 1000)
 		return -EINVAL;
-	*((int *)kp->arg) = l;
+
+	*((unsigned long *)kp->arg) = l;
 
 	/* notify edac_mc engine to reset the poll period */
 	edac_mc_reset_delay_period(l);
diff --git a/drivers/edac/edac_module.h b/drivers/edac/edac_module.h
index 3d139c6e7fe3..f2118bfcf8df 100644
--- a/drivers/edac/edac_module.h
+++ b/drivers/edac/edac_module.h
@@ -52,7 +52,7 @@ extern void edac_device_workq_setup(struct edac_device_ctl_info *edac_dev,
 extern void edac_device_workq_teardown(struct edac_device_ctl_info *edac_dev);
 extern void edac_device_reset_delay_period(struct edac_device_ctl_info
 					   *edac_dev, unsigned long value);
-extern void edac_mc_reset_delay_period(int value);
+extern void edac_mc_reset_delay_period(unsigned long value);
 
 extern void *edac_align_ptr(void **p, unsigned size, int n_elems);
 
-- 
1.8.5.2.192.g7794a68

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] edac, poll timeout cannot be zero

[Tejun Heo]

Hello,

On Wed, Feb 12, 2014 at 04:07:48PM +0100, Borislav Petkov wrote:
> While testing, however, I keep seeing the splat below and that's:
> 
>         if (WARN_ON(!list_empty(&work->entry))) {
>                 spin_unlock(&pwq->pool->lock);
>                 return;
>         }
> 
> and there seems to be some interference with edac_mc_workq_setup()
> which does mod_delayed_work() and then the workqueue callback
> edac_mc_workq_function() which does queue_delayed_work().
> 
> What I'm seeing in the splat is that when the timer fires to run the
> delayed work, __queue_work() complains that the work list is not empty
> even though we've done mod_delayed_work() which is supposed to cancel
> any pending work.
> 
> Tejun, any ideas what's happening? Do we need synchronization here or do
> you have a _sync version of mod_delayed_work() which makes sure any work
> is cancelled?

No, you don't need to.  All workqueue operations should be able to
synchronize with each other.

> Or does this mean that once the work is getting queued from the timer
> callback delayed_work_timer_fn, it cannot be cancelled anymore? Or
> something else I'm missing...?

Looking at edac_mc_workq_setup().... it contains INIT_DELAYED_WORK().
Does this race with other workqueue operations on the work item?  If
so, it of course breaks.  It's like doing spin_lock_init() while other
spinlock operations are in progress.

Thanks.

-- 
tejun

Re: [PATCH] edac, poll timeout cannot be zero

[Borislav Petkov]

On Wed, Feb 12, 2014 at 10:16:57AM -0500, Tejun Heo wrote:
> No, you don't need to.  All workqueue operations should be able to
> synchronize with each other.

Ok.

> > Or does this mean that once the work is getting queued from the timer
> > callback delayed_work_timer_fn, it cannot be cancelled anymore? Or
> > something else I'm missing...?
> 
> Looking at edac_mc_workq_setup().... it contains INIT_DELAYED_WORK().
> Does this race with other workqueue operations on the work item?  If
> so, it of course breaks.  It's like doing spin_lock_init() while other
> spinlock operations are in progress.

Ha, so this sounds like the issue because the splat happens
when updating /sys/.../edac_mc_poll_msec and it goes and calls
edac_mc_workq_setup()...

And this code is pretty old and edac_mc_workq_setup() is used both
when one inits an edac driver and also when one wants to change the
polling period (edac_mc_reset_delay_period) and thus needs to mod the
workqueue's timeout.

So I'm guessing a simple fix would be to differentiate between the two
paths, something like the diff below.

Or is there a reliable way to check whether a workqueue has been
initialized already, say, something like

	if (work->func)

or so... I.e., what PREPARE_WORK() does?

Thanks!

---
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index e8c9ef03495b..17c51d3a1143 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -559,7 +559,8 @@ static void edac_mc_workq_function(struct work_struct *work_req)
  *
  *		called with the mem_ctls_mutex held
  */
-static void edac_mc_workq_setup(struct mem_ctl_info *mci, unsigned msec)
+static void edac_mc_workq_setup(struct mem_ctl_info *mci, unsigned msec,
+				bool init)
 {
 	edac_dbg(0, "\n");
 
@@ -567,7 +568,9 @@ static void edac_mc_workq_setup(struct mem_ctl_info *mci, unsigned msec)
 	if (mci->op_state != OP_RUNNING_POLL)
 		return;
 
-	INIT_DELAYED_WORK(&mci->work, edac_mc_workq_function);
+	if (init)
+		INIT_DELAYED_WORK(&mci->work, edac_mc_workq_function);
+
 	mod_delayed_work(edac_workqueue, &mci->work, msecs_to_jiffies(msec));
 }
 
@@ -611,7 +614,7 @@ void edac_mc_reset_delay_period(int value)
 	list_for_each(item, &mc_devices) {
 		mci = list_entry(item, struct mem_ctl_info, link);
 
-		edac_mc_workq_setup(mci, (unsigned long) value);
+		edac_mc_workq_setup(mci, (unsigned long) value, false);
 	}
 
 	mutex_unlock(&mem_ctls_mutex);
@@ -782,7 +785,7 @@ int edac_mc_add_mc(struct mem_ctl_info *mci)
 		/* This instance is NOW RUNNING */
 		mci->op_state = OP_RUNNING_POLL;
 
-		edac_mc_workq_setup(mci, edac_mc_get_poll_msec());
+		edac_mc_workq_setup(mci, edac_mc_get_poll_msec(), true);
 	} else {
 		mci->op_state = OP_RUNNING_INTERRUPT;
 	}

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] edac, poll timeout cannot be zero

[Tejun Heo]

Hello,

On Wed, Feb 12, 2014 at 04:57:53PM +0100, Borislav Petkov wrote:
> Ha, so this sounds like the issue because the splat happens
> when updating /sys/.../edac_mc_poll_msec and it goes and calls
> edac_mc_workq_setup()...
> 
> And this code is pretty old and edac_mc_workq_setup() is used both
> when one inits an edac driver and also when one wants to change the
> polling period (edac_mc_reset_delay_period) and thus needs to mod the
> workqueue's timeout.
> 
> So I'm guessing a simple fix would be to differentiate between the two
> paths, something like the diff below.
> 
> Or is there a reliable way to check whether a workqueue has been
> initialized already, say, something like
> 
> 	if (work->func)

Hah... can't you just do it on object initialization?  It's a bit
nasty and fragile to game initialization state.

> or so... I.e., what PREPARE_WORK() does?

It allows you to change the callback without breaking the
synchronization.  Of course, if there's a pending work item, it may or
may not execute the newly assigned function.  It's also a bit nasty
and has only few users in the whole kernel and I think we might wanna
get rid of them.

Thanks.

-- 
tejun

Re: [PATCH] edac, poll timeout cannot be zero

[Borislav Petkov]

On Wed, Feb 12, 2014 at 11:23:11AM -0500, Tejun Heo wrote:
> Hah... can't you just do it on object initialization?  It's a bit
> nasty and fragile to game initialization state.
> 
> > or so... I.e., what PREPARE_WORK() does?
> 
> It allows you to change the callback without breaking the
> synchronization.  Of course, if there's a pending work item, it may or
> may not execute the newly assigned function.  It's also a bit nasty
> and has only few users in the whole kernel and I think we might wanna
> get rid of them.

Yeah, no, I certainly don't have to touch wq inner organs - I was
wondering whether there's such functionality already. The simple patch I
sent earlier should be good enough then.

Thanks!

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

[PATCH] EDAC: Correct workqueue setup path

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

We're using edac_mc_workq_setup() both on the init path, when
we load an edac driver and when we change the polling period
(edac_mc_reset_delay_period) through /sys/.../edac_mc_poll_msec.

On that second path we don't need to init the workqueue which has been
initialized already.

Thanks to Tejun for workqueue insights.

Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: <stable@vger.kernel.org>
---
 drivers/edac/edac_mc.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index aef5ec24908e..33edd6766344 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -559,7 +559,8 @@ static void edac_mc_workq_function(struct work_struct *work_req)
  *
  *		called with the mem_ctls_mutex held
  */
-static void edac_mc_workq_setup(struct mem_ctl_info *mci, unsigned msec)
+static void edac_mc_workq_setup(struct mem_ctl_info *mci, unsigned msec,
+				bool init)
 {
 	edac_dbg(0, "\n");
 
@@ -567,7 +568,9 @@ static void edac_mc_workq_setup(struct mem_ctl_info *mci, unsigned msec)
 	if (mci->op_state != OP_RUNNING_POLL)
 		return;
 
-	INIT_DELAYED_WORK(&mci->work, edac_mc_workq_function);
+	if (init)
+		INIT_DELAYED_WORK(&mci->work, edac_mc_workq_function);
+
 	mod_delayed_work(edac_workqueue, &mci->work, msecs_to_jiffies(msec));
 }
 
@@ -611,7 +614,7 @@ void edac_mc_reset_delay_period(unsigned long value)
 	list_for_each(item, &mc_devices) {
 		mci = list_entry(item, struct mem_ctl_info, link);
 
-		edac_mc_workq_setup(mci, value);
+		edac_mc_workq_setup(mci, value, false);
 	}
 
 	mutex_unlock(&mem_ctls_mutex);
@@ -782,7 +785,7 @@ int edac_mc_add_mc(struct mem_ctl_info *mci)
 		/* This instance is NOW RUNNING */
 		mci->op_state = OP_RUNNING_POLL;
 
-		edac_mc_workq_setup(mci, edac_mc_get_poll_msec());
+		edac_mc_workq_setup(mci, edac_mc_get_poll_msec(), true);
 	} else {
 		mci->op_state = OP_RUNNING_INTERRUPT;
 	}
-- 
1.8.5.2.192.g7794a68

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--