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
