Re: [PATCH] crypto/ccp: don't disable interrupts while setting up debugfs

[Herbert Xu]

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 :03:00.2: enabling device ( -> 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): [<97a18c49>] 
> restore_regs_and_return_to_kernel+0x0/0x23
> |hardirqs last disabled at (17150): [<0773b3a9>] 
> _raw_write_lock_irqsave+0x1b/0x50
> |softirqs last  enabled at (17148): [<64d56155>] 
> __do_softirq+0x3b8/0x4c1
> |softirqs last disabled at (17125): [<92633c18>] 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 
> Signed-off-by: Sebastian Andrzej Siewior 

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Can a driver->probe be called for two devices at the same time (WAS: Re: [PATCH] crypto/ccp: don't disable interrupts while setting up debugfs)

[Gary R Hook]

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

Re: Can a driver->probe be called for two devices at the same time (WAS: Re: [PATCH] crypto/ccp: don't disable interrupts while setting up debugfs)

[Sebastian Andrzej Siewior]

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

Re: Can a driver->probe be called for two devices at the same time (WAS: Re: [PATCH] crypto/ccp: don't disable interrupts while setting up debugfs)

[Greg Kroah-Hartman]

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

Re: Can a driver->probe be called for two devices at the same time (WAS: Re: [PATCH] crypto/ccp: don't disable interrupts while setting up debugfs)

[Gary R Hook]

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.

Can a driver->probe be called for two devices at the same time (WAS: Re: [PATCH] crypto/ccp: don't disable interrupts while setting up debugfs)

[Sebastian Andrzej Siewior]

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

Re: [PATCH] crypto/ccp: don't disable interrupts while setting up debugfs

[Gary R Hook]

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

Re: [PATCH] crypto/ccp: don't disable interrupts while setting up debugfs

[Tom Lendacky]

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 :03:00.2: enabling device ( -> 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): [<97a18c49>]
>> restore_regs_and_return_to_kernel+0x0/0x23
>> |hardirqs last disabled at (17150): [<0773b3a9>]
>> _raw_write_lock_irqsave+0x1b/0x50
>> |softirqs last  enabled at (17148): [<64d56155>]
>> __do_softirq+0x3b8/0x4c1
>> |softirqs last disabled at (17125): [<92633c18>] 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 
>> Signed-off-by: Sebastian Andrzej Siewior 
>> ---
>>   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(_debugfs_lock, flags);
>> +    mutex_lock(_debugfs_lock);
>>   if (!ccp_debugfs_dir)
>>   ccp_debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, NULL);
>> -    write_unlock_irqrestore(_debugfs_lock, flags);
>> +    mutex_unlock(_debugfs_lock);
>>   if (!ccp_debugfs_dir)
>>   return;
>>  
> 

Re: [PATCH] crypto/ccp: don't disable interrupts while setting up debugfs

[Sebastian Andrzej Siewior]

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 :03:00.2: enabling device ( -> 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): [<97a18c49>] 
> > restore_regs_and_return_to_kernel+0x0/0x23
> > |hardirqs last disabled at (17150): [<0773b3a9>] 
> > _raw_write_lock_irqsave+0x1b/0x50
> > |softirqs last  enabled at (17148): [<64d56155>] 
> > __do_softirq+0x3b8/0x4c1
> > |softirqs last disabled at (17125): [<92633c18>] 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

Re: [PATCH] crypto/ccp: don't disable interrupts while setting up debugfs

[Hook, Gary]

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 :03:00.2: enabling device ( -> 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): [<97a18c49>] 
restore_regs_and_return_to_kernel+0x0/0x23
|hardirqs last disabled at (17150): [<0773b3a9>] 
_raw_write_lock_irqsave+0x1b/0x50
|softirqs last  enabled at (17148): [<64d56155>] 
__do_softirq+0x3b8/0x4c1
|softirqs last disabled at (17125): [<92633c18>] 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 
Signed-off-by: Sebastian Andrzej Siewior 
---
  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(_debugfs_lock, flags);

+   mutex_lock(_debugfs_lock);
if (!ccp_debugfs_dir)
ccp_debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, NULL);
-   write_unlock_irqrestore(_debugfs_lock, flags);
+   mutex_unlock(_debugfs_lock);
if (!ccp_debugfs_dir)
return;
  

[PATCH] crypto/ccp: don't disable interrupts while setting up debugfs

[Sebastian Andrzej Siewior]

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 :03:00.2: enabling device ( -> 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): [<97a18c49>] 
restore_regs_and_return_to_kernel+0x0/0x23
|hardirqs last disabled at (17150): [<0773b3a9>] 
_raw_write_lock_irqsave+0x1b/0x50
|softirqs last  enabled at (17148): [<64d56155>] 
__do_softirq+0x3b8/0x4c1
|softirqs last disabled at (17125): [<92633c18>] 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 
Signed-off-by: Sebastian Andrzej Siewior 
---
 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);
 
 #defineMAX_NAME_LEN20
 
@@ -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(_debugfs_lock, flags);
+   mutex_lock(_debugfs_lock);
if (!ccp_debugfs_dir)
ccp_debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, NULL);
-   write_unlock_irqrestore(_debugfs_lock, flags);
+   mutex_unlock(_debugfs_lock);
if (!ccp_debugfs_dir)
return;
 
-- 
2.16.1