Dan Williams wrote:
Pierre-Louis Bossart wrote:
On 1/30/24 19:46, Greg Kroah-Hartman wrote:
From: Dan Williams dan.j.williams@intel.com
Add a mechanism for named attribute_groups to hide their directory at sysfs_update_group() time, or otherwise skip emitting the group directory when the group is first registered. It piggybacks on is_visible() in a similar manner as SYSFS_PREALLOC, i.e. special flags in the upper bits of the returned mode. To use it, specify a symbol prefix to DEFINE_SYSFS_GROUP_VISIBLE(), and then pass that same prefix to SYSFS_GROUP_VISIBLE() when assigning the @is_visible() callback:
DEFINE_SYSFS_GROUP_VISIBLE($prefix)
struct attribute_group $prefix_group = { .name = $name, .is_visible = SYSFS_GROUP_VISIBLE($prefix), };
SYSFS_GROUP_VISIBLE() expects a definition of $prefix_group_visible() and $prefix_attr_visible(), where $prefix_group_visible() just returns true / false and $prefix_attr_visible() behaves as normal.
The motivation for this capability is to centralize PCI device authentication in the PCI core with a named sysfs group while keeping that group hidden for devices and platforms that do not meet the requirements. In a PCI topology, most devices will not support authentication, a small subset will support just PCI CMA (Component Measurement and Authentication), a smaller subset will support PCI CMA + PCIe IDE (Link Integrity and Encryption), and only next generation server hosts will start to include a platform TSM (TEE Security Manager).
Without this capability the alternatives are:
Check if all attributes are invisible and if so, hide the directory. Beyond trouble getting this to work [1], this is an ABI change for scenarios if userspace happens to depend on group visibility absent any attributes. I.e. this new capability avoids regression since it does not retroactively apply to existing cases.
Publish an empty /sys/bus/pci/devices/$pdev/tsm/ directory for all PCI devices (i.e. for the case when TSM platform support is present, but device support is absent). Unfortunate that this will be a vestigial empty directory in the vast majority of cases.
Reintroduce usage of runtime calls to sysfs_{create,remove}_group() in the PCI core. Bjorn has already indicated that he does not want to see any growth of pci_sysfs_init() [2].
Drop the named group and simulate a directory by prefixing all TSM-related attributes with "tsm_". Unfortunate to not use the naming capability of a sysfs group as intended.
In comparison, there is a small potential for regression if for some reason an @is_visible() callback had dependencies on how many times it was called. Additionally, it is no longer an error to update a group that does not have its directory already present, and it is no longer a WARN() to remove a group that was never visible.
Link: https://lore.kernel.org/all/2024012321-envious-procedure-4a58@gregkh/ [1] Link: https://lore.kernel.org/linux-pci/20231019200110.GA1410324@bhelgaas/ [2] Signed-off-by: Dan Williams dan.j.williams@intel.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
This patch seems to introduce a regression on our Lunar Lake test devices, where we can't boot to an ssh shell. No issues on older devices [1]. Bard Liao and I reproduced the same results on different boards.
We'll need to find someone with direct device access to provide more information on the problem, remote testing without ssh is a self-negating proposition.
Is there a dependency on other patches? Our tests are still based on 6.7.0-rc3 due to other upstream issues we're currently working through.
The only behavior change I can imagine with this patch is that ->is_visble() callbacks get called extra times for named attribute groups.
...or if an is_visible() callback was inadvertantly already using the SYSFS_GROUP_INVISIBLE flag in umode_t result.
Are you able to get kernel logs? A before and after with this patch applied might highlight which attribute does not appreciate the extra callback...
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c index ccb275cdabcb..683c0b10990b 100644 --- a/fs/sysfs/group.c +++ b/fs/sysfs/group.c @@ -33,11 +33,17 @@ static void remove_files(struct kernfs_node *parent,
static umode_t __first_visible(const struct attribute_group *grp, struct kobject *kobj) { - if (grp->attrs && grp->is_visible) + if (grp->attrs && grp->is_visible) { + pr_info("kobj: %s is_visible: %pS\n", kobj->name, + grp->is_visible); return grp->is_visible(kobj, grp->attrs[0], 0); + }
- if (grp->bin_attrs && grp->is_bin_visible) + if (grp->bin_attrs && grp->is_bin_visible) { + pr_info("kobj: %s is_bin_visible: %pS\n", kobj->name, + grp->is_bin_visible); return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0); + }
return 0; } @@ -62,6 +68,8 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj, if (update) kernfs_remove_by_name(parent, (*attr)->name); if (grp->is_visible) { + pr_info("kobj: %s is_visible: %pS\n", + kobj->name, grp->is_visible); mode = grp->is_visible(kobj, *attr, i); mode &= ~SYSFS_GROUP_INVISIBLE; if (!mode) @@ -92,6 +100,8 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj, kernfs_remove_by_name(parent, (*bin_attr)->attr.name); if (grp->is_bin_visible) { + pr_info("kobj: %s is_bin_visible: %pS\n", + kobj->name, grp->is_bin_visible); mode = grp->is_bin_visible(kobj, *bin_attr, i); mode &= ~SYSFS_GROUP_INVISIBLE; if (!mode)