Re: [PATCH 3/3] blk-crypto: show crypto capabilities in sysfs

Hi Greg, thanks for the review!

On Sat, Nov 27, 2021 at 10:06:18AM +0100, Greg KH wrote:

quoted

diff --git a/Documentation/block/queue-sysfs.rst b/Documentation/block/queue-sysfs.rst
index 3f569d5324857..252939f340459 100644
--- a/Documentation/block/queue-sysfs.rst
+++ b/Documentation/block/queue-sysfs.rst

Why is all of this information not in Documentation/ABI/ like the rest
of the kernel's sysfs information?  When it is there it can be
automatically tested as well.

Please don't add new entries to the wrong place if at all possible.

Some of the block queue attributes are documented in
Documentation/ABI/testing/sysfs-block, but Documentation/block/queue-sysfs.rst
seems to be the authoritative source in practice.  I checked all QUEUE_*_ENTRY
in block/blk-sysfs.c, and I got:

- 16 attributes are documented in both places
- 23 attributes are documented in Documentation/block/ only
- 0 attributes are documented in Documentation/ABI/ only
- 2 attributes ("virt_boundary_mask" and "stable_writes") not documented in
  either place

So most block queue attributes are documented only in Documentation/block/.  And
if I added my new attributes to Documentation/ABI/ only, as you're requesting,
they would be the only block queue attributes that would be documented in only
that place.  I think that would make things worse, as then there would be no
authoritative source anymore.

If both you and the block people agree that *all* block queue attributes should
be documented in Documentation/ABI/ only, I'd be glad to send a separate patch
that adds anything missing to Documentation/ABI/testing/sysfs-block, then
removes Documentation/block/queue-sysfs.rst.  (BTW, shouldn't it really be in
Documentation/ABI/stable/?  This ABI has been around a long time, so surely
users are relying on it.)  But it doesn't seem fair to block this patch on that.

quoted

+static ssize_t blk_crypto_max_dun_bits_show(struct blk_crypto_profile *profile,
+					    struct blk_crypto_attr *attr,
+					    char *page)
+{
+	return sprintf(page, "%u\n", 8 * profile->max_dun_bytes_supported);

sysfs_emit() please, for this, and all other show functions.

Sure.  Note that in .show() functions kernel-wide, it appears that sprintf() is
much more commonly used than sysfs_emit().  Is there any plan to convert these?
As-is, if people use existing code as a reference, it will be "wrong" most of
the time, which is unfortunate.

quoted

+}
+
+static ssize_t blk_crypto_num_keyslots_show(struct blk_crypto_profile *profile,
+					    struct blk_crypto_attr *attr,
+					    char *page)
+{
+	return sprintf(page, "%u\n", profile->num_slots);
+}
+
+#define BLK_CRYPTO_RO_ATTR(_name)			\
+static struct blk_crypto_attr blk_crypto_##_name = {	\
+	.attr	= { .name = #_name, .mode = 0444 },	\

__ATTR_RO()?

Sure.  This would require removing the "blk_crypto_" prefix from the .show()
functions, which I'd prefer to have, but it doesn't really matter.

quoted

+static const struct attribute_group *blk_crypto_attr_groups[] = {
+	&blk_crypto_attr_group,
+	&blk_crypto_modes_attr_group,
+	NULL,
+};

ATTRIBUTE_GROUP()?

Hm, maybe not, but I think it could be used here.

ATTRIBUTE_GROUP() doesn't exist; probably you're referring to
ATTRIBUTE_GROUPS()?  ATTRIBUTE_GROUPS() is only usable when there is only one
attribute group.  In this case, there are two attribute groups.

quoted

+static int __init blk_crypto_sysfs_init(void)
+{
+	int i;
+
+	BUILD_BUG_ON(BLK_ENCRYPTION_MODE_INVALID != 0);
+	for (i = 1; i < BLK_ENCRYPTION_MODE_MAX; i++) {
+		struct blk_crypto_attr *attr = &__blk_crypto_mode_attrs[i];

sysfs_attr_init() might be needed here, have you run with lockdep
enabled?

It's not needed because __blk_crypto_mode_attrs isn't dynamically allocated
memory.  Yes, I've run with lockdep enabled.

- Eric