From: Greg KH <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
Subject: Re: [PATCH 3/3] blk-crypto: show crypto capabilities in sysfs
Date: Sun, 28 Nov 2021 09:14:28 +0100 [thread overview]
Message-ID: <YaM6ZJUByYXaI3/X@kroah.com> (raw)
In-Reply-To: <YaKZUu0tQc8bblmI@sol.localdomain>
On Sat, Nov 27, 2021 at 12:47:14PM -0800, Eric Biggers wrote:
> 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.
I agree, it should all move to the proper location in Documentation/ABI/
as that is where all sysfs attributes need to be documented. Block
queues are not special here.
> 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.
"stable" is fine with me, people abuse "testing" by throwing everything
into it.
>
> > > +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.
Doing a wholesale replacement across the kernel is a pain and disruptive
and not really needed. But for all new code, please use the new
functions. If you want to convert your driver/subsystem to the new
functions, no objection from me!
> > > +}
> > > +
> > > +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.
Ah, you are right, but I think using the default macros sometimes can be
nicer as they are easier to verify you are doing things correctly.
> > > +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.
You are right, sorry.
> > > +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.
Ok, good, just checking.
thanks,
greg k-h
prev parent reply other threads:[~2021-11-28 8:16 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-26 21:25 [PATCH 0/3] block: show crypto capabilities in sysfs Eric Biggers
2021-11-26 21:25 ` [PATCH 1/3] block: simplify calling convention of elv_unregister_queue() Eric Biggers
2021-11-26 21:25 ` [PATCH 2/3] block: don't delete queue kobject before its children Eric Biggers
2021-11-26 21:25 ` [PATCH 3/3] blk-crypto: show crypto capabilities in sysfs Eric Biggers
2021-11-27 9:06 ` Greg KH
2021-11-27 20:47 ` Eric Biggers
2021-11-28 8:14 ` Greg KH [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YaM6ZJUByYXaI3/X@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=ebiggers@kernel.org \
--cc=linux-api@vger.kernel.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mmc@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).