Received-SPF: pass (google.com: domain of linux-kernel+bounces-47030-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 31 Jan 2024 11:43:03 -0800
From: Dan Williams <dan.j.williams@intel.com>
To: Dan Williams <dan.j.williams@intel.com>, Pierre-Louis Bossart
	<pierre-louis.bossart@linux.intel.com>, Greg Kroah-Hartman
	<gregkh@linuxfoundation.org>, <alsa-devel@alsa-project.org>
CC: <linux-kernel@vger.kernel.org>, Dan Williams <dan.j.williams@intel.com>,
	Vinod Koul <vkoul@kernel.org>, Bard Liao <yung-chuan.liao@linux.intel.com>,
	Sanyog Kale <sanyog.r.kale@intel.com>
Subject: Re: [PATCH 1/6] sysfs: Introduce a mechanism to hide static
 attribute_groups
Message-ID: <65baa2c754e8d_37ad2941f@dwillia2-xfh.jf.intel.com.notmuch>
References: <2024013025-spoiling-exact-ad20@gregkh>
 <2024013028-deflator-flaring-ec62@gregkh>
 <b93ec9c2-23f5-486b-a3dc-ed9b960df359@linux.intel.com>
 <65ba8ca5709fe_4e7f5294ce@dwillia2-xfh.jf.intel.com.notmuch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <65ba8ca5709fe_4e7f5294ce@dwillia2-xfh.jf.intel.com.notmuch>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?E9y5caEjxDY4Io3FVZ1qrPlIof3bvbXIcbEMa7/XBUXCUA0tXjBiBLGHcHnv?=
 =?us-ascii?Q?CmYZMP+1LDDuprQnFt+DXL9pt8Z9ufm1M62cX3QeDWunKjyd6HUWckFocJBv?=
 =?us-ascii?Q?rTou1CDWcsWYYJNdSZ6PrKxdBMbbrm0K8iCqkswdla5oIaYr/zbhRXruJI/X?=
 =?us-ascii?Q?DP62lsTkI82OVV0PEdiUkEPvy8l1zO14SFlsaxdKAq1vL6lTt9eHjjOa6i+n?=
 =?us-ascii?Q?tliZnXQsoqE46eYkGuWyoK4WxaK7n72HmH0Oi6yDRLsI/RIFulXsLG/wjwG+?=
 =?us-ascii?Q?JCNibvNF3QOxnur0Zq1xWJ+UYHgUlwlIXRD7UDBcdqJhhCOCQBqTZyjlUslp?=
 =?us-ascii?Q?8ugk6af3MdZ7PM0AwFjHwuXa93Xb1sednUM/4kxKerXsqT6GBb7sUxAtLk1y?=
 =?us-ascii?Q?0wB99esJSNurkzF8cfSBGTVjaVCSCNfG3Upbq9z3nyh1KsnxleZHbHC6+iiT?=
 =?us-ascii?Q?byqNmFmzp9FJ3+WB934KkrMGPURJCNcXDsIyJLUCOx4IEFlRaY0HUvUBix6c?=
 =?us-ascii?Q?9v3JOO7t30IgnarQA5TCX1rcbiemW7bBwvsii19A+0vK2Vipcm35m6b8Z/UY?=
 =?us-ascii?Q?sc2Ev1vEoAidU/NU+H3vLajx9ttN7nIvJP7hqNAP/MhZ8K6/Fd2n3Irh1iVo?=
 =?us-ascii?Q?Sk0iwR7+vpB1YtwiAxAbrpH6y8j8YCCEk/ZMzTDvctCLVMF8l6igSYMl7zNf?=
 =?us-ascii?Q?UC/e5y7kiT32TBd7ZrjYHrIAZ81f4bZlskaoJaRD5QC4Zb/r6UMtpSzWgc4R?=
 =?us-ascii?Q?VpAJn2yZQnDPb8BL3N0JGmySL4aXvNWDSxwMoZCsrgcDMrHr8fRbTugz2A0a?=
 =?us-ascii?Q?EFS0rHgG+ZyvKoluHLaIme6R9JybLekvRg+S/kYEZLIXYYVLVq82FMWT8pi/?=
 =?us-ascii?Q?WAOndOb/fvy/bydzo6TtTibmleNUo0V840SdjKaFgtBcFZAIQaWCivyZpcle?=
 =?us-ascii?Q?4L3q7E2a9X8/pAaeuNPVv5zKeD7MsCLYYF6SfKdnKSK8CE10B1PuccXAddJ4?=
 =?us-ascii?Q?NsktHcjyrH+Amaes71W3Dn8m8R1xPX07eqQDMKPHV1xO0EBLkA6/XyA/szQZ?=
 =?us-ascii?Q?pwEprc89eN32aSgGWs8zsHu2LSHsw+t9Js0ckR0EaJ1YrwSC6YvkaSfgXNTi?=
 =?us-ascii?Q?ykeRQiPBAu95p/JK4hsa7dRa39FwzZ5paq8OcPESAAhNLhqgnKRJz9eKVF43?=
 =?us-ascii?Q?HO9mEfFC06Dxl4p13DWQhKL9hFzy3ZTpyHGD3YF9CQip3CVnV9QsBfGexAqG?=
 =?us-ascii?Q?qM539Z66BNktTioZ/FKnu8LvFQYFP0hhG1HrZSOpS+JDhKy1DOhEVVp9Cofh?=
 =?us-ascii?Q?VtZskoVz/mxAAu+bdls+G2lRpeFyPRsscGGSPsJ7oWl8DyA8OZLsWcmZBcuH?=
 =?us-ascii?Q?A7rhqKaqib59R9spKJ2hYmKt32ALGMxkIYRoqcENl4P4P1eybI0QWUSaJp7h?=
 =?us-ascii?Q?x/8hBIHEuHqY5hhunBnLgVCIYASThoMiNUD29lRLdKzD/08O+uaSjDgmxYCL?=
 =?us-ascii?Q?t9U/UuhFZhifEPgTr3gY3jGClVEHSiInJ1jiVeH2WwtS0UgIqZPhrTzrIYES?=
 =?us-ascii?Q?4YHQ34qC7a3yBWT8mHMM+A7HThRCqL8btWk+0x98ITu6YMWQW9Dw9JZjGd60?=
 =?us-ascii?Q?jQ=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c16f26a6-de3c-46ca-66dd-08dc2294d820
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jan 2024 19:43:05.7515
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: WI6EzjQIvAoL6bgLf5tQ20/v7TtOC+b2vZEdhJaQnUmv3u1JsnqZ4yam5orLU4IZ4QTjaj/WM+nQQFk48cnpJYNBQVE0cuV1ytAbYkDeTM8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB5934
X-OriginatorOrg: intel.com

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)