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

[Eric Biggers <ebiggers@kernel.org>]

Hi Greg, thanks for the review!

On Sat, Nov 27, 2021 at 10:06:18AM +0100, Greg KH wrote:
> > 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.

> > +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.

> > +}
> > +
> > +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.

> > +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.

> > +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