On Thu, Jun 04, 2020 at 04:33:24PM +1000, Herbert Xu wrote: > +static void crc_t10dif_rehash(struct work_struct *work) > +{ > + struct crypto_shash *new, *old; > + > mutex_lock(&crc_t10dif_mutex); > old = rcu_dereference_protected(crct10dif_tfm, > lockdep_is_held(&crc_t10dif_mutex)); > if (!old) { > mutex_unlock(&crc_t10dif_mutex); > - return 0; > + return; > } > new = crypto_alloc_shash("crct10dif", 0, 0); > if (IS_ERR(new)) { > mutex_unlock(&crc_t10dif_mutex); > - return 0; > + return; > } > rcu_assign_pointer(crct10dif_tfm, new); > mutex_unlock(&crc_t10dif_mutex); > > synchronize_rcu(); > crypto_free_shash(old); > - return 0; > + return; > } The last return statement is unnecessary. > static int __init crc_t10dif_mod_init(void) > { > + struct crypto_shash *tfm; > + > + INIT_WORK(&crct10dif_rehash_work, crc_t10dif_rehash); > crypto_register_notifier(&crc_t10dif_nb); > - crct10dif_tfm = crypto_alloc_shash("crct10dif", 0, 0); > - if (IS_ERR(crct10dif_tfm)) { > + mutex_lock(&crc_t10dif_mutex); > + tfm = crypto_alloc_shash("crct10dif", 0, 0); > + if (IS_ERR(tfm)) { > static_key_slow_inc(&crct10dif_fallback); > - crct10dif_tfm = NULL; > + tfm = NULL; > } > + RCU_INIT_POINTER(crct10dif_tfm, tfm); > + mutex_unlock(&crc_t10dif_mutex); > return 0; > } Wouldn't it make more sense to initialize crct10dif_tfm before registering the notifier? Then the mutex wouldn't be needed. - Eric