From: Dan Williams <dan.j.williams@intel.com>
To: Lukas Wunner <lukas@wunner.de>, Dan Williams <dan.j.williams@intel.com>
Cc: <gregkh@linuxfoundation.org>,
Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
Marc Herbert <marc.herbert@intel.com>,
"Rafael J. Wysocki" <rafael@kernel.org>,
<linux-kernel@vger.kernel.org>, <linux-pci@vger.kernel.org>,
<linux-coco@lists.linux.dev>, <alsa-devel@alsa-project.org>
Subject: Re: [PATCH 1/3] sysfs: Fix crash on empty group attributes array
Date: Mon, 29 Apr 2024 10:47:46 -0700 [thread overview]
Message-ID: <662fdd4283b0b_1487294a7@dwillia2-mobl3.amr.corp.intel.com.notmuch> (raw)
In-Reply-To: <Zi4gK8rs7jjdJOh8@wunner.de>
Lukas Wunner wrote:
> On Sat, Apr 27, 2024 at 02:33:24PM -0700, Dan Williams wrote:
> > Lukas Wunner wrote:
> > > Perhaps an optional ->is_group_visible() callback in struct attribute_group
> > > which gets passed only the struct kobject pointer?
> > >
> > > At least for PCI device authentication, that would be sufficient.
> > > I could get from the kobject to the corresponding struct device,
> > > then determine whether the device supports authentication or not.
> > >
> > > Because it's a new, optional callback, there should be no compatibility
> > > issues. The SYSFS_GROUP_INVISIBLE return code from the ->is_visible()
> > > call for individual attributes would not be needed then, at least in my
> > > use case.
> >
> > That's where I started with this, but decided it was overkill to
> > increase the size of that data structure globally for a small number of
> > use cases.
>
> Memory is cheap and memory-constrained devices can set CONFIG_SYSFS=n.
That sounds severe, but point taken that someone could config-off the
cases that need this extension.
> There aren't that many struct attribute_groups and this is just
> 8 additional bytes on a 64-bit machine. (There are way more
> struct attribute than struct attribute_group.) The contortions
> necessary to overload individual attribute ->is_visible() callbacks
> to also govern the group's visibility aren't worth it.
I agree that most systems would not care about growing this structure,
but the same is true for almost all other data structure growth in the
kernel. It is a typical kernel pastime to squeeze functionality into
existing data structures.
> Having an ->is_group_visible() callback has the additional benefit that
> the mode of directories no longer needs to be hardcoded to 0755 in
> sysfs_create_dir_ns(), but can be set to, say, 0500 or 0700 or 0511,
> depending on the use case. So more flexibility there as well.
Unnecessary growth is unnecessary growth. In this case all the known use
cases can use the SYSFS_GROUP_INVISIBLE flag returned from is_visible().
The awkwardness around cases that want to have an empty attributes array
and invisible group directory is noted and puts the solution on notice
for running afoul of the sunk cost fallacy in the future.
next prev parent reply other threads:[~2024-04-29 17:47 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-22 20:40 [PATCH 0/3] sysfs: Group visibility fixups Dan Williams
2024-02-22 20:40 ` [PATCH 1/3] sysfs: Fix crash on empty group attributes array Dan Williams
2024-02-22 21:14 ` Rafael J. Wysocki
2024-02-22 22:03 ` Dan Williams
2024-02-22 23:15 ` Dan Williams
2024-04-22 9:20 ` Lukas Wunner
2024-04-26 17:59 ` Dan Williams
2024-04-26 19:18 ` Lukas Wunner
2024-04-27 11:05 ` Greg KH
2024-04-27 16:49 ` Dan Williams
2024-04-27 21:14 ` Lukas Wunner
2024-04-27 21:33 ` Dan Williams
2024-04-27 22:39 ` Dan Williams
2024-04-27 23:09 ` Dan Williams
2024-04-28 10:08 ` Lukas Wunner
2024-04-29 17:47 ` Dan Williams [this message]
2024-02-22 20:41 ` [PATCH 2/3] sysfs: Document new "group visible" helpers Dan Williams
2024-02-22 20:41 ` [PATCH 3/3] sysfs: Introduce DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE() Dan Williams
2024-02-23 6:33 ` [PATCH 0/3] sysfs: Group visibility fixups Greg KH
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=662fdd4283b0b_1487294a7@dwillia2-mobl3.amr.corp.intel.com.notmuch \
--to=dan.j.williams@intel.com \
--cc=alsa-devel@alsa-project.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-coco@lists.linux.dev \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lukas@wunner.de \
--cc=marc.herbert@intel.com \
--cc=pierre-louis.bossart@linux.intel.com \
--cc=rafael@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 read-only IMAP folder(s) and NNTP newsgroup(s).