Re: [PATCH] crypto: iaa - Fix race condition when probing IAA devices

[Vinicius Costa Gomes]

Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> writes:

> On Tue, Jun 03, 2025 at 04:55:31PM -0700, Vinicius Costa Gomes wrote:
>>
>> diff --git a/drivers/crypto/intel/iaa/iaa_crypto_main.c b/drivers/crypto/intel/iaa/iaa_crypto_main.c
>> index 23f585219fb4..2185c101bef3 100644
>> --- a/drivers/crypto/intel/iaa/iaa_crypto_main.c
>> +++ b/drivers/crypto/intel/iaa/iaa_crypto_main.c
>> @@ -35,28 +35,39 @@ static unsigned int cpus_per_iaa;
>>  
>>  /* Per-cpu lookup table for balanced wqs */
>>  static struct wq_table_entry __percpu *wq_table;
>> +static DEFINE_SPINLOCK(wq_table_lock);
>
> This can be called in BH context so you need to disable BH when
> taking the spinlock.
>

My tests with lockdep enabled must have missed something, will verify
and fix this.

>>  static struct idxd_wq *wq_table_next_wq(int cpu)
>>  {
>> -	struct wq_table_entry *entry = per_cpu_ptr(wq_table, cpu);
>> +	struct wq_table_entry *entry;
>> +	struct idxd_wq *wq;
>> +	int id;
>> +
>> +	guard(spinlock)(&wq_table_lock);
>> +
>> +	entry = per_cpu_ptr(wq_table, cpu);
>
> You're taking a global spinlock around a per-cpu variable.  In that
> case you might as well get rid of the per-cpu variable and use the
> spinlock only.
>

>From what I could gather, the idea of the per-cpu workqueue table ("map"
really) is more to "spread" the workqueues to different CPUS than to
reduce contention.

If the question is more about the choice of using per-cpu variables, I
can look for alternatives.

> Cheers,
> -- 
> Email: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt


Cheers,
-- 
Vinicius