Date:   Tue, 30 Nov 2021 08:13:37 +0100
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-api@vger.kernel.org, linux-scsi@vger.kernel.org,
        linux-mmc@vger.kernel.org, Bart Van Assche <bvanassche@acm.org>
Subject: Re: [PATCH v2 3/3] blk-crypto: show crypto capabilities in sysfs
Message-ID: <YaXPIYHg8Xnk1Lbh@kroah.com>
References: <20211130040306.148925-1-ebiggers@kernel.org>
 <20211130040306.148925-4-ebiggers@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211130040306.148925-4-ebiggers@kernel.org>
Precedence: bulk

On Mon, Nov 29, 2021 at 08:03:06PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Add sysfs files that expose the inline encryption capabilities of
> request queues:
> 
> 	/sys/class/block/$disk/queue/crypto/max_dun_bits
> 	/sys/class/block/$disk/queue/crypto/modes/$mode
> 	/sys/class/block/$disk/queue/crypto/num_keyslots
> 
> Userspace can use these new files to decide what encryption settings to
> use, or whether to use inline encryption at all.  This also brings the
> crypto capabilities in line with the other queue properties, which are
> already discoverable via the queue directory in sysfs.
> 
> Design notes:
> 
>   - Place the new files in a new subdirectory "crypto" to group them
>     together and to avoid complicating the main "queue" directory.  This
>     also makes it possible to replace "crypto" with a symlink later if
>     we ever make the blk_crypto_profiles into real kobjects (see below).
> 
>   - It was necessary to define a new kobject that corresponds to the
>     crypto subdirectory.  For now, this kobject just contains a pointer
>     to the blk_crypto_profile.  Note that multiple queues (and hence
>     multiple such kobjects) may refer to the same blk_crypto_profile.
> 
>     An alternative design would more closely match the current kernel
>     data structures: the blk_crypto_profile could be a kobject itself,
>     located directly under the host controller device's kobject, while
>     /sys/class/block/$disk/queue/crypto would be a symlink to it.
> 
>     I decided not to do that for now because it would require a lot more
>     changes, such as no longer embedding blk_crypto_profile in other
>     structures, and also because I'm not sure we can rule out moving the
>     crypto capabilities into 'struct queue_limits' in the future.  (Even
>     if multiple queues share the same crypto engine, maybe the supported
>     data unit sizes could differ due to other queue properties.)  It
>     would also still be possible to switch to that design later without
>     breaking userspace, by replacing the directory with a symlink.
> 
>   - Use "max_dun_bits" instead of "max_dun_bytes".  Currently, the
>     kernel internally stores this value in bytes, but that's an
>     implementation detail.  It probably makes more sense to talk about
>     this value in bits, and choosing bits is more future-proof.
> 
>   - "modes" is a sub-subdirectory, since there may be multiple supported
>     crypto modes, and sysfs is supposed to have one value per file.
> 
>   - Each mode had to be named.  The crypto API names like "xts(aes)" are
>     not appropriate because they don't specify the key size.  Therefore,
>     I assigned new names.  The exact names chosen are arbitrary, but
>     they happen to match the names used in log messages in fs/crypto/.
> 
>   - The "num_keyslots" file is a bit different from the others in that
>     it is only useful to know for performance reasons.  However, it's
>     included as it can still be useful.  For example, a user might not
>     want to use inline encryption if there aren't very many keyslots.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  Documentation/block/queue-sysfs.rst |  30 +++++

Same objection as before.  Please add these to Documentation/ABI/
otherwise when running 'scripts/get_abi undefined' will show these new
sysfs files you are adding as having no documentation :(

thanks,

greg k-h