| Message ID | 20180223223307.18882-1-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
On 2/23/2018 5:33 PM, Sebastian Andrzej Siewior wrote: > I don't why we need take a single write lock and disable interrupts > while setting up debugfs. This is what what happens when we try anyway: There is more than one CCP on some processors. The lock is intended to serialize attempts to initialize the new directory, but a R/W lock isn't required. My testing on an EPYC (8 CCPs) didn't expose this problem. May I ask what hardware you used here? > |ccp 0000:03:00.2: enabling device (0000 -> 0002) > |BUG: sleeping function called from invalid context at kernel/locking/rwsem.c:69 > |in_atomic(): 1, irqs_disabled(): 1, pid: 3, name: kworker/0:0 > |irq event stamp: 17150 > |hardirqs last enabled at (17149): [<0000000097a18c49>] restore_regs_and_return_to_kernel+0x0/0x23 > |hardirqs last disabled at (17150): [<000000000773b3a9>] _raw_write_lock_irqsave+0x1b/0x50 > |softirqs last enabled at (17148): [<0000000064d56155>] __do_softirq+0x3b8/0x4c1 > |softirqs last disabled at (17125): [<0000000092633c18>] irq_exit+0xb1/0xc0 > |CPU: 0 PID: 3 Comm: kworker/0:0 Not tainted 4.16.0-rc2+ #30 > |Workqueue: events work_for_cpu_fn > |Call Trace: > | dump_stack+0x7d/0xb6 > | ___might_sleep+0x1eb/0x250 > | down_write+0x17/0x60 > | start_creating+0x4c/0xe0 > | debugfs_create_dir+0x9/0x100 > | ccp5_debugfs_setup+0x191/0x1b0 > | ccp5_init+0x8a7/0x8c0 > | ccp_dev_init+0xb8/0xe0 > | sp_init+0x6c/0x90 > | sp_pci_probe+0x26e/0x590 > | local_pci_probe+0x3f/0x90 > | work_for_cpu_fn+0x11/0x20 > | process_one_work+0x1ff/0x650 > | worker_thread+0x1d4/0x3a0 > | kthread+0xfe/0x130 > | ret_from_fork+0x27/0x50 > > If any locking is required, a simple mutex will do it. > > Cc: Gary R Hook <gary.hook@amd.com> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > drivers/crypto/ccp/ccp-debugfs.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/crypto/ccp/ccp-debugfs.c b/drivers/crypto/ccp/ccp-debugfs.c > index 59d4ca4e72d8..1a734bd2070a 100644 > --- a/drivers/crypto/ccp/ccp-debugfs.c > +++ b/drivers/crypto/ccp/ccp-debugfs.c > @@ -278,7 +278,7 @@ static const struct file_operations ccp_debugfs_stats_ops = { > }; > > static struct dentry *ccp_debugfs_dir; > -static DEFINE_RWLOCK(ccp_debugfs_lock); > +static DEFINE_MUTEX(ccp_debugfs_lock); > > #define MAX_NAME_LEN 20 > > @@ -290,16 +290,15 @@ void ccp5_debugfs_setup(struct ccp_device *ccp) > struct dentry *debugfs_stats; > struct dentry *debugfs_q_instance; > struct dentry *debugfs_q_stats; > - unsigned long flags; > int i; > > if (!debugfs_initialized()) > return; > > - write_lock_irqsave(&ccp_debugfs_lock, flags); > + mutex_lock(&ccp_debugfs_lock); > if (!ccp_debugfs_dir) > ccp_debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, NULL); > - write_unlock_irqrestore(&ccp_debugfs_lock, flags); > + mutex_unlock(&ccp_debugfs_lock); > if (!ccp_debugfs_dir) > return; > >
On 2018-02-25 21:04:27 [-0500], Hook, Gary wrote: > On 2/23/2018 5:33 PM, Sebastian Andrzej Siewior wrote: > > I don't why we need take a single write lock and disable interrupts > > while setting up debugfs. This is what what happens when we try anyway: > > There is more than one CCP on some processors. The lock is intended to > serialize attempts to initialize the new directory, but a R/W lock isn't > required. And they are probed in parallel? Any you need disable interrupts while creating the debugfs folder? A mutex isn't enough? > My testing on an EPYC (8 CCPs) didn't expose this problem. May I ask what > hardware you used here? an EPYC, too. I have no idea how many CCPs were around but lspci -k printed that driver more than once. Try to enable lockdep and "sleeping while atomic". But I did provide you a complete backtrace, look: > > |ccp 0000:03:00.2: enabling device (0000 -> 0002) > > |BUG: sleeping function called from invalid context at kernel/locking/rwsem.c:69 > > |in_atomic(): 1, irqs_disabled(): 1, pid: 3, name: kworker/0:0 > > |irq event stamp: 17150 > > |hardirqs last enabled at (17149): [<0000000097a18c49>] restore_regs_and_return_to_kernel+0x0/0x23 > > |hardirqs last disabled at (17150): [<000000000773b3a9>] _raw_write_lock_irqsave+0x1b/0x50 > > |softirqs last enabled at (17148): [<0000000064d56155>] __do_softirq+0x3b8/0x4c1 > > |softirqs last disabled at (17125): [<0000000092633c18>] irq_exit+0xb1/0xc0 > > |CPU: 0 PID: 3 Comm: kworker/0:0 Not tainted 4.16.0-rc2+ #30 > > |Workqueue: events work_for_cpu_fn > > |Call Trace: > > | dump_stack+0x7d/0xb6 > > | ___might_sleep+0x1eb/0x250 > > | down_write+0x17/0x60 > > | start_creating+0x4c/0xe0 > > | debugfs_create_dir+0x9/0x100 > > | ccp5_debugfs_setup+0x191/0x1b0 > > | ccp5_init+0x8a7/0x8c0 > > | ccp_dev_init+0xb8/0xe0 > > | sp_init+0x6c/0x90 > > | sp_pci_probe+0x26e/0x590 > > | local_pci_probe+0x3f/0x90 > > | work_for_cpu_fn+0x11/0x20 > > | process_one_work+0x1ff/0x650 > > | worker_thread+0x1d4/0x3a0 > > | kthread+0xfe/0x130 > > | ret_from_fork+0x27/0x50 Sebastian
On 2/25/2018 8:04 PM, Hook, Gary wrote: > On 2/23/2018 5:33 PM, Sebastian Andrzej Siewior wrote: >> I don't why we need take a single write lock and disable interrupts >> while setting up debugfs. This is what what happens when we try anyway: > > There is more than one CCP on some processors. The lock is intended to > serialize attempts to initialize the new directory, but a R/W lock isn't > required. > > My testing on an EPYC (8 CCPs) didn't expose this problem. May I ask what > hardware you used here? Probably not a hardware issue as opposed to a kernel configuration. Try using CONFIG_DEBUG_ATOMIC_SLEEP and see if you can recreate. And if irqs are disabled, then you're probably looking at having to use a spinlock to serialize creation of the directory. Thanks, Tom > >> |ccp 0000:03:00.2: enabling device (0000 -> 0002) >> |BUG: sleeping function called from invalid context at >> kernel/locking/rwsem.c:69 >> |in_atomic(): 1, irqs_disabled(): 1, pid: 3, name: kworker/0:0 >> |irq event stamp: 17150 >> |hardirqs last enabled at (17149): [<0000000097a18c49>] >> restore_regs_and_return_to_kernel+0x0/0x23 >> |hardirqs last disabled at (17150): [<000000000773b3a9>] >> _raw_write_lock_irqsave+0x1b/0x50 >> |softirqs last enabled at (17148): [<0000000064d56155>] >> __do_softirq+0x3b8/0x4c1 >> |softirqs last disabled at (17125): [<0000000092633c18>] irq_exit+0xb1/0xc0 >> |CPU: 0 PID: 3 Comm: kworker/0:0 Not tainted 4.16.0-rc2+ #30 >> |Workqueue: events work_for_cpu_fn >> |Call Trace: >> | dump_stack+0x7d/0xb6 >> | ___might_sleep+0x1eb/0x250 >> | down_write+0x17/0x60 >> | start_creating+0x4c/0xe0 >> | debugfs_create_dir+0x9/0x100 >> | ccp5_debugfs_setup+0x191/0x1b0 >> | ccp5_init+0x8a7/0x8c0 >> | ccp_dev_init+0xb8/0xe0 >> | sp_init+0x6c/0x90 >> | sp_pci_probe+0x26e/0x590 >> | local_pci_probe+0x3f/0x90 >> | work_for_cpu_fn+0x11/0x20 >> | process_one_work+0x1ff/0x650 >> | worker_thread+0x1d4/0x3a0 >> | kthread+0xfe/0x130 >> | ret_from_fork+0x27/0x50 >> >> If any locking is required, a simple mutex will do it. >> >> Cc: Gary R Hook <gary.hook@amd.com> >> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> >> --- >> drivers/crypto/ccp/ccp-debugfs.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/crypto/ccp/ccp-debugfs.c >> b/drivers/crypto/ccp/ccp-debugfs.c >> index 59d4ca4e72d8..1a734bd2070a 100644 >> --- a/drivers/crypto/ccp/ccp-debugfs.c >> +++ b/drivers/crypto/ccp/ccp-debugfs.c >> @@ -278,7 +278,7 @@ static const struct file_operations >> ccp_debugfs_stats_ops = { >> }; >> static struct dentry *ccp_debugfs_dir; >> -static DEFINE_RWLOCK(ccp_debugfs_lock); >> +static DEFINE_MUTEX(ccp_debugfs_lock); >> #define MAX_NAME_LEN 20 >> @@ -290,16 +290,15 @@ void ccp5_debugfs_setup(struct ccp_device *ccp) >> struct dentry *debugfs_stats; >> struct dentry *debugfs_q_instance; >> struct dentry *debugfs_q_stats; >> - unsigned long flags; >> int i; >> if (!debugfs_initialized()) >> return; >> - write_lock_irqsave(&ccp_debugfs_lock, flags); >> + mutex_lock(&ccp_debugfs_lock); >> if (!ccp_debugfs_dir) >> ccp_debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, NULL); >> - write_unlock_irqrestore(&ccp_debugfs_lock, flags); >> + mutex_unlock(&ccp_debugfs_lock); >> if (!ccp_debugfs_dir) >> return; >> >
On 02/26/2018 02:35 AM, Sebastian Andrzej Siewior wrote: > On 2018-02-25 21:04:27 [-0500], Hook, Gary wrote: >> On 2/23/2018 5:33 PM, Sebastian Andrzej Siewior wrote: >>> I don't why we need take a single write lock and disable interrupts >>> while setting up debugfs. This is what what happens when we try anyway: >> >> There is more than one CCP on some processors. The lock is intended to >> serialize attempts to initialize the new directory, but a R/W lock isn't >> required. > > And they are probed in parallel? Any you need disable interrupts while > creating the debugfs folder? A mutex isn't enough? That issue remains unclear to me: Are probes of PCI devices guaranteed to be serialized? Observations on my CCPs says that they occur in order, but I don't know for certain that serialization is guaranteed. Is there a definitive statement on this somewhere that I just don't know about? I think a mutex would be just fine; I got this wrong, clearly. Let me work up a patch using a mutex. Gary
On 2018-02-27 11:08:56 [-0600], Gary R Hook wrote: > That issue remains unclear to me: Are probes of PCI devices guaranteed to be > serialized? Observations on my CCPs says that they occur in order, but I > don't know for certain that serialization is guaranteed. > > Is there a definitive statement on this somewhere that I just don't know > about? So the question if a driver can probe two devices simultaneously. I'm not sure. We have PROBE_PREFER_ASYNCHRONOUS which defers the probe to worker. However I have no idea if two of those worker can run at the same time. > I think a mutex would be just fine; I got this wrong, clearly. Let me work > up a patch using a mutex. I've sent one. Why not just ack it and be done with it? > Gary Sebastian
On 02/27/2018 11:33 AM, Sebastian Andrzej Siewior wrote: > On 2018-02-27 11:08:56 [-0600], Gary R Hook wrote: >> That issue remains unclear to me: Are probes of PCI devices guaranteed to be >> serialized? Observations on my CCPs says that they occur in order, but I >> don't know for certain that serialization is guaranteed. >> >> Is there a definitive statement on this somewhere that I just don't know >> about? > > So the question if a driver can probe two devices simultaneously. I'm > not sure. We have PROBE_PREFER_ASYNCHRONOUS which defers the probe to > worker. However I have no idea if two of those worker can run at the > same time. > >> I think a mutex would be just fine; I got this wrong, clearly. Let me work >> up a patch using a mutex. > > I've sent one. Why not just ack it and be done with it? > >> Gary > > Sebastian > Sorry, too much chaos right now. Of course.
On 02/23/2018 04:33 PM, Sebastian Andrzej Siewior wrote: > I don't why we need take a single write lock and disable interrupts > while setting up debugfs. This is what what happens when we try anyway: > > |ccp 0000:03:00.2: enabling device (0000 -> 0002) > |BUG: sleeping function called from invalid context at kernel/locking/rwsem.c:69 > |in_atomic(): 1, irqs_disabled(): 1, pid: 3, name: kworker/0:0 > |irq event stamp: 17150 > |hardirqs last enabled at (17149): [<0000000097a18c49>] restore_regs_and_return_to_kernel+0x0/0x23 > |hardirqs last disabled at (17150): [<000000000773b3a9>] _raw_write_lock_irqsave+0x1b/0x50 > |softirqs last enabled at (17148): [<0000000064d56155>] __do_softirq+0x3b8/0x4c1 > |softirqs last disabled at (17125): [<0000000092633c18>] irq_exit+0xb1/0xc0 > |CPU: 0 PID: 3 Comm: kworker/0:0 Not tainted 4.16.0-rc2+ #30 > |Workqueue: events work_for_cpu_fn > |Call Trace: > | dump_stack+0x7d/0xb6 > | ___might_sleep+0x1eb/0x250 > | down_write+0x17/0x60 > | start_creating+0x4c/0xe0 > | debugfs_create_dir+0x9/0x100 > | ccp5_debugfs_setup+0x191/0x1b0 > | ccp5_init+0x8a7/0x8c0 > | ccp_dev_init+0xb8/0xe0 > | sp_init+0x6c/0x90 > | sp_pci_probe+0x26e/0x590 > | local_pci_probe+0x3f/0x90 > | work_for_cpu_fn+0x11/0x20 > | process_one_work+0x1ff/0x650 > | worker_thread+0x1d4/0x3a0 > | kthread+0xfe/0x130 > | ret_from_fork+0x27/0x50 > > If any locking is required, a simple mutex will do it. > > Cc: Gary R Hook <gary.hook@amd.com> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Acked-by: Gary R Hook <gary.hook@amd.com> > --- > drivers/crypto/ccp/ccp-debugfs.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/crypto/ccp/ccp-debugfs.c b/drivers/crypto/ccp/ccp-debugfs.c > index 59d4ca4e72d8..1a734bd2070a 100644 > --- a/drivers/crypto/ccp/ccp-debugfs.c > +++ b/drivers/crypto/ccp/ccp-debugfs.c > @@ -278,7 +278,7 @@ static const struct file_operations ccp_debugfs_stats_ops = { > }; > > static struct dentry *ccp_debugfs_dir; > -static DEFINE_RWLOCK(ccp_debugfs_lock); > +static DEFINE_MUTEX(ccp_debugfs_lock); > > #define MAX_NAME_LEN 20 > > @@ -290,16 +290,15 @@ void ccp5_debugfs_setup(struct ccp_device *ccp) > struct dentry *debugfs_stats; > struct dentry *debugfs_q_instance; > struct dentry *debugfs_q_stats; > - unsigned long flags; > int i; > > if (!debugfs_initialized()) > return; > > - write_lock_irqsave(&ccp_debugfs_lock, flags); > + mutex_lock(&ccp_debugfs_lock); > if (!ccp_debugfs_dir) > ccp_debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, NULL); > - write_unlock_irqrestore(&ccp_debugfs_lock, flags); > + mutex_unlock(&ccp_debugfs_lock); > if (!ccp_debugfs_dir) > return; > >
On Tue, Feb 27, 2018 at 06:33:14PM +0100, Sebastian Andrzej Siewior wrote: > On 2018-02-27 11:08:56 [-0600], Gary R Hook wrote: > > That issue remains unclear to me: Are probes of PCI devices guaranteed to be > > serialized? Observations on my CCPs says that they occur in order, but I > > don't know for certain that serialization is guaranteed. > > > > Is there a definitive statement on this somewhere that I just don't know > > about? The bus enforces this. > So the question if a driver can probe two devices simultaneously. Depends on the bus type. thanks, greg k-h
On 2018-02-27 19:40:34 [+0100], Greg Kroah-Hartman wrote: > On Tue, Feb 27, 2018 at 06:33:14PM +0100, Sebastian Andrzej Siewior wrote: > > On 2018-02-27 11:08:56 [-0600], Gary R Hook wrote: > > > That issue remains unclear to me: Are probes of PCI devices guaranteed to be > > > serialized? Observations on my CCPs says that they occur in order, but I > > > don't know for certain that serialization is guaranteed. > > > > > > Is there a definitive statement on this somewhere that I just don't know > > > about? > > The bus enforces this. > > > So the question if a driver can probe two devices simultaneously. > > Depends on the bus type. PCI > thanks, > > greg k-h Sebastian
On 02/27/2018 01:36 PM, Sebastian Andrzej Siewior wrote: > On 2018-02-27 19:40:34 [+0100], Greg Kroah-Hartman wrote: >> On Tue, Feb 27, 2018 at 06:33:14PM +0100, Sebastian Andrzej Siewior wrote: >>> On 2018-02-27 11:08:56 [-0600], Gary R Hook wrote: >>>> That issue remains unclear to me: Are probes of PCI devices guaranteed to be >>>> serialized? Observations on my CCPs says that they occur in order, but I >>>> don't know for certain that serialization is guaranteed. >>>> >>>> Is there a definitive statement on this somewhere that I just don't know >>>> about? >> >> The bus enforces this. >> >>> So the question if a driver can probe two devices simultaneously. >> >> Depends on the bus type. > > PCI So the question is whether or not PCI enforces serial activity within a domain. The CCPs are all on different buses, so that doesn't matter. I think we don't care in this situation, given that the CCP driver has minor requirements for locking. I just found it an interesting (albeit somewhat academic) question. Thanks, Gary
On Fri, Feb 23, 2018 at 11:33:07PM +0100, Sebastian Andrzej Siewior wrote: > I don't why we need take a single write lock and disable interrupts > while setting up debugfs. This is what what happens when we try anyway: > > |ccp 0000:03:00.2: enabling device (0000 -> 0002) > |BUG: sleeping function called from invalid context at kernel/locking/rwsem.c:69 > |in_atomic(): 1, irqs_disabled(): 1, pid: 3, name: kworker/0:0 > |irq event stamp: 17150 > |hardirqs last enabled at (17149): [<0000000097a18c49>] restore_regs_and_return_to_kernel+0x0/0x23 > |hardirqs last disabled at (17150): [<000000000773b3a9>] _raw_write_lock_irqsave+0x1b/0x50 > |softirqs last enabled at (17148): [<0000000064d56155>] __do_softirq+0x3b8/0x4c1 > |softirqs last disabled at (17125): [<0000000092633c18>] irq_exit+0xb1/0xc0 > |CPU: 0 PID: 3 Comm: kworker/0:0 Not tainted 4.16.0-rc2+ #30 > |Workqueue: events work_for_cpu_fn > |Call Trace: > | dump_stack+0x7d/0xb6 > | ___might_sleep+0x1eb/0x250 > | down_write+0x17/0x60 > | start_creating+0x4c/0xe0 > | debugfs_create_dir+0x9/0x100 > | ccp5_debugfs_setup+0x191/0x1b0 > | ccp5_init+0x8a7/0x8c0 > | ccp_dev_init+0xb8/0xe0 > | sp_init+0x6c/0x90 > | sp_pci_probe+0x26e/0x590 > | local_pci_probe+0x3f/0x90 > | work_for_cpu_fn+0x11/0x20 > | process_one_work+0x1ff/0x650 > | worker_thread+0x1d4/0x3a0 > | kthread+0xfe/0x130 > | ret_from_fork+0x27/0x50 > > If any locking is required, a simple mutex will do it. > > Cc: Gary R Hook <gary.hook@amd.com> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Patch applied. Thanks.
diff --git a/drivers/crypto/ccp/ccp-debugfs.c b/drivers/crypto/ccp/ccp-debugfs.c index 59d4ca4e72d8..1a734bd2070a 100644 --- a/drivers/crypto/ccp/ccp-debugfs.c +++ b/drivers/crypto/ccp/ccp-debugfs.c @@ -278,7 +278,7 @@ static const struct file_operations ccp_debugfs_stats_ops = { }; static struct dentry *ccp_debugfs_dir; -static DEFINE_RWLOCK(ccp_debugfs_lock); +static DEFINE_MUTEX(ccp_debugfs_lock); #define MAX_NAME_LEN 20 @@ -290,16 +290,15 @@ void ccp5_debugfs_setup(struct ccp_device *ccp) struct dentry *debugfs_stats; struct dentry *debugfs_q_instance; struct dentry *debugfs_q_stats; - unsigned long flags; int i; if (!debugfs_initialized()) return; - write_lock_irqsave(&ccp_debugfs_lock, flags); + mutex_lock(&ccp_debugfs_lock); if (!ccp_debugfs_dir) ccp_debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, NULL); - write_unlock_irqrestore(&ccp_debugfs_lock, flags); + mutex_unlock(&ccp_debugfs_lock); if (!ccp_debugfs_dir) return;
I don't why we need take a single write lock and disable interrupts while setting up debugfs. This is what what happens when we try anyway: |ccp 0000:03:00.2: enabling device (0000 -> 0002) |BUG: sleeping function called from invalid context at kernel/locking/rwsem.c:69 |in_atomic(): 1, irqs_disabled(): 1, pid: 3, name: kworker/0:0 |irq event stamp: 17150 |hardirqs last enabled at (17149): [<0000000097a18c49>] restore_regs_and_return_to_kernel+0x0/0x23 |hardirqs last disabled at (17150): [<000000000773b3a9>] _raw_write_lock_irqsave+0x1b/0x50 |softirqs last enabled at (17148): [<0000000064d56155>] __do_softirq+0x3b8/0x4c1 |softirqs last disabled at (17125): [<0000000092633c18>] irq_exit+0xb1/0xc0 |CPU: 0 PID: 3 Comm: kworker/0:0 Not tainted 4.16.0-rc2+ #30 |Workqueue: events work_for_cpu_fn |Call Trace: | dump_stack+0x7d/0xb6 | ___might_sleep+0x1eb/0x250 | down_write+0x17/0x60 | start_creating+0x4c/0xe0 | debugfs_create_dir+0x9/0x100 | ccp5_debugfs_setup+0x191/0x1b0 | ccp5_init+0x8a7/0x8c0 | ccp_dev_init+0xb8/0xe0 | sp_init+0x6c/0x90 | sp_pci_probe+0x26e/0x590 | local_pci_probe+0x3f/0x90 | work_for_cpu_fn+0x11/0x20 | process_one_work+0x1ff/0x650 | worker_thread+0x1d4/0x3a0 | kthread+0xfe/0x130 | ret_from_fork+0x27/0x50 If any locking is required, a simple mutex will do it. Cc: Gary R Hook <gary.hook@amd.com> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- drivers/crypto/ccp/ccp-debugfs.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)