Received-SPF: pass (google.com: domain of linux-kernel+bounces-46950-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 31 Jan 2024 10:08:37 -0800
From: Dan Williams <dan.j.williams@intel.com>
To: 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: <65ba8ca5709fe_4e7f5294ce@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>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <b93ec9c2-23f5-486b-a3dc-ed9b960df359@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?rJ845QMEKKj9yyu89VB3hI+VDIeeqkQOSRIMv8In2hjHQbF4Ejlstf0hr64x?=
 =?us-ascii?Q?tzS4RdE0QQKfiu9YiT2ySa4VUK3SfvPTTNkuPKpLjLmCpyWPslCQaUM9NCL+?=
 =?us-ascii?Q?iU201B64Un+UpWirAsvtUVRdaXbeLsH6twFhkWfKFKGQIT8isP/ApXeSPvS9?=
 =?us-ascii?Q?mU+h5vaQRU/EWjAvQ6BcAx9ccaiGvAR2+7OmeFLH1qnn94AXh8K4J2GwAZDs?=
 =?us-ascii?Q?tVWsCrUNiAiOiaJnzjN8Xk6xNudMkpZTu44gE+MqdZ6LeObuuv8Jk58XuFWj?=
 =?us-ascii?Q?RZ6OrM+AxrfhslFHKdZTETVNj1T9kLtO3yO+MG++HoaDKXsTE0/BL3WMiqzg?=
 =?us-ascii?Q?2lFGlHHbRF9QxKU+V1G+gYLanZ5SaplVLQh0LrU81z1OEnBB0ZuNTDhpy/ry?=
 =?us-ascii?Q?uyZCLU8heRbV4XU1zvVYgpLdh0FY0UlM+Ifl0vgkCUIAJT6cqOiglNODUuky?=
 =?us-ascii?Q?xkFG+FrCFo6gRGI9PuRwNxutJ8U7gXvduxmtiYd1PcH0R+125DgO3QWKgTMX?=
 =?us-ascii?Q?3ABghseSJs71HT6bfD7jOFYfRNi5992GwCaOPYULDTz7HpohvLWzLNu64jgz?=
 =?us-ascii?Q?XJnto5UOZEqhlxTOKJ/qrXSC7eVrTM0MmSYVn7Eu5OnKigPBovaanKolKJwn?=
 =?us-ascii?Q?ebSE/ksQFTpeNH2AdHGgikH/RNNFuUOHm6OWlKF6xU4FY9qJTVPwtYHZIG1a?=
 =?us-ascii?Q?dNR9mBvLZ9fuihmko9R3BPw0RQP7735Qd4/osm7WDLPGoBGjJTkNeyXAY2ic?=
 =?us-ascii?Q?Und6MoL5+M7AhST45dEYmpqwC1IEyOxakdvf1u6g9Igav0r90xGnswlI0By2?=
 =?us-ascii?Q?e/N2GNAo9KyW03+hNuMckzRrIEUzdefIXgvdWGPsW8sais0aA+EdpV7WTWXl?=
 =?us-ascii?Q?n5U+6qLsBkwn8RI+g87hoj9Nwok+W/NQ9rLzBEgwCJkEuHw4nrrftbwxvAsx?=
 =?us-ascii?Q?F0ECZYCa1VXS1uDGqH0ZOdT+DVr9GLqrS6bHyycvzK9UM8xSH8PpwP8hQitC?=
 =?us-ascii?Q?9iZX2IjEtNWH6Ih3bRTZLWGyBngio5eLadkN6kt8/I2w1PKzd+hvFgNBGCwo?=
 =?us-ascii?Q?XTztjWMKIaLbCeD54lSWkreNTlDpPZFpFxcAiJq0aWLhfTKteqdeTRwPG+HK?=
 =?us-ascii?Q?1hgyUzshjGK9ClWkRpc6/DVxt7IFCJbY3yP/XXcLVeTU4n5hJ8hTbFqH1376?=
 =?us-ascii?Q?VIusPwUuAidinaktQbD/mLTMbs/6+uYUH2gNWSziBGJyWZgkjvvtmt8iZroW?=
 =?us-ascii?Q?3auVSSusICc4Ird993+PRSso4GgD0A79T229x4bQd6iCGlTlopQ8Tyf0UOe2?=
 =?us-ascii?Q?ZJqnhp8jbx36qTyyJlwwFSEDEKuO18lcJ9zLZr9Gq8aYsZg5r16sZyQD6Y7A?=
 =?us-ascii?Q?/O490kupYwCAgnQhUcERHF5Jc8vV9oYUtF6DBpg5IrVeMFK3wLYX4bpl9NTZ?=
 =?us-ascii?Q?jU15hJOJWzOgyqAQ2DI0i2+i8MdsPZv8YlD8x1kLkYgTEO73j7HnfOKTowUv?=
 =?us-ascii?Q?kdY6l37RDDQlC2Hpv4TW8q9PppmzbQxm+gAIuD+u2mGVa8dWVOkK//dzD1Ej?=
 =?us-ascii?Q?LVgumZclIMjnkTqS0XqcXmk5x5JHat3awn1St/c5VMOPe/lOUve7vAxcwN83?=
 =?us-ascii?Q?Ow=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ea7638fb-73cc-4b2d-04ae-08dc2287a6e8
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jan 2024 18:08:39.6981
 (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: ske+mI3g7wRhJZ06SPID2IucIMChUZJk4RA0dHAfSPHPOwe9ARCDSOnZ4HKYWadCBHtq1WosL+Pvq4wA+O7jyBxiCwUrR4Rw6hZtvZCykZ0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB7177
X-OriginatorOrg: intel.com

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.