Re: [PATCH 1/2] block/keyslot-manager: introduce devm_blk_ksm_init()

[Satya Tangirala <satyat@google.com>]

On Thu, Jan 21, 2021 at 12:23 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> From: Eric Biggers <ebiggers@google.com>
>
> Add a resource-managed variant of blk_ksm_init() so that drivers don't
> have to worry about calling blk_ksm_destroy().
>
> Note that the implementation uses a custom devres action to call
> blk_ksm_destroy() rather than switching the two allocations to be
> directly devres-managed, e.g. with devm_kmalloc().  This is because we
> need to keep zeroing the memory containing the keyslots when it is
> freed, and also because we want to continue using kvmalloc() (and there
> is no devm_kvmalloc()).
>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  Documentation/block/inline-encryption.rst | 12 +++++-----
>  block/keyslot-manager.c                   | 29 +++++++++++++++++++++++
>  include/linux/keyslot-manager.h           |  3 +++
>  3 files changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/block/inline-encryption.rst b/Documentation/block/inline-encryption.rst
> index e75151e467d39..7f9b40d6b416b 100644
> --- a/Documentation/block/inline-encryption.rst
> +++ b/Documentation/block/inline-encryption.rst
> @@ -182,8 +182,9 @@ API presented to device drivers
>
>  A :c:type:``struct blk_keyslot_manager`` should be set up by device drivers in
>  the ``request_queue`` of the device. The device driver needs to call
> -``blk_ksm_init`` on the ``blk_keyslot_manager``, which specifying the number of
> -keyslots supported by the hardware.
> +``blk_ksm_init`` (or its resource-managed variant ``devm_blk_ksm_init``) on the
> +``blk_keyslot_manager``, while specifying the number of keyslots supported by
> +the hardware.
>
>  The device driver also needs to tell the KSM how to actually manipulate the
>  IE hardware in the device to do things like programming the crypto key into
> @@ -202,10 +203,9 @@ needs each and every of its keyslots to be reprogrammed with the key it
>  "should have" at the point in time when the function is called. This is useful
>  e.g. if a device loses all its keys on runtime power down/up.
>
> -``blk_ksm_destroy`` should be called to free up all resources used by a keyslot
> -manager upon ``blk_ksm_init``, once the ``blk_keyslot_manager`` is no longer
> -needed.
> -
> +If the driver used ``blk_ksm_init`` instead of ``devm_blk_ksm_init``, then
> +``blk_ksm_destroy`` should be called to free up all resources used by a
> +``blk_keyslot_manager`` once it is no longer needed.
>
>  Layered Devices
>  ===============
> diff --git a/block/keyslot-manager.c b/block/keyslot-manager.c
> index 86f8195d8039e..324bf4244f5fb 100644
> --- a/block/keyslot-manager.c
> +++ b/block/keyslot-manager.c
> @@ -29,6 +29,7 @@
>  #define pr_fmt(fmt) "blk-crypto: " fmt
>
>  #include <linux/keyslot-manager.h>
> +#include <linux/device.h>
>  #include <linux/atomic.h>
>  #include <linux/mutex.h>
>  #include <linux/pm_runtime.h>
> @@ -127,6 +128,34 @@ int blk_ksm_init(struct blk_keyslot_manager *ksm, unsigned int num_slots)
>  }
>  EXPORT_SYMBOL_GPL(blk_ksm_init);
>
> +static void blk_ksm_destroy_callback(void *ksm)
> +{
> +       blk_ksm_destroy(ksm);
> +}
> +
> +/**
> + * devm_blk_ksm_init() - Resource-managed blk_ksm_init()
> + * @dev: The device which owns the blk_keyslot_manager.
> + * @ksm: The blk_keyslot_manager to initialize.
> + * @num_slots: The number of key slots to manage.
> + *
> + * Like blk_ksm_init(), but causes blk_ksm_destroy() to be called automatically
> + * on driver detach.
> + *
> + * Return: 0 on success, or else a negative error code.
> + */
> +int devm_blk_ksm_init(struct device *dev, struct blk_keyslot_manager *ksm,
> +                     unsigned int num_slots)
> +{
> +       int err = blk_ksm_init(ksm, num_slots);
> +
> +       if (err)
> +               return err;
> +
> +       return devm_add_action_or_reset(dev, blk_ksm_destroy_callback, ksm);
> +}
> +EXPORT_SYMBOL_GPL(devm_blk_ksm_init);
> +
>  static inline struct hlist_head *
>  blk_ksm_hash_bucket_for_key(struct blk_keyslot_manager *ksm,
>                             const struct blk_crypto_key *key)
> diff --git a/include/linux/keyslot-manager.h b/include/linux/keyslot-manager.h
> index 18f3f5346843f..443ad817c6c57 100644
> --- a/include/linux/keyslot-manager.h
> +++ b/include/linux/keyslot-manager.h
> @@ -85,6 +85,9 @@ struct blk_keyslot_manager {
>
>  int blk_ksm_init(struct blk_keyslot_manager *ksm, unsigned int num_slots);
>
> +int devm_blk_ksm_init(struct device *dev, struct blk_keyslot_manager *ksm,
> +                     unsigned int num_slots);
> +
>  blk_status_t blk_ksm_get_slot_for_key(struct blk_keyslot_manager *ksm,
>                                       const struct blk_crypto_key *key,
>                                       struct blk_ksm_keyslot **slot_ptr);
> --
> 2.30.0
Looks good to me. Please feel free to add
Reviewed-by: Satya Tangirala <satyat@google.com>