Received-SPF: pass (google.com: domain of linux-kernel+bounces-109597-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 20 Mar 2024 19:40:18 -0700
From: Dan Williams <dan.j.williams@intel.com>
To: Tom Lendacky <thomas.lendacky@amd.com>, <linux-kernel@vger.kernel.org>,
	<x86@kernel.org>, Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar
	<mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, Dave Hansen
	<dave.hansen@linux.intel.com>, "H. Peter Anvin" <hpa@zytor.com>, "Andy
 Lutomirski" <luto@kernel.org>, Peter Zijlstra <peterz@infradead.org>, "Dan
 Williams" <dan.j.williams@intel.com>, Michael Roth <michael.roth@amd.com>,
	Ashish Kalra <ashish.kalra@amd.com>, Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH v2 12/14] fs/configfs: Add a callback to determine
 attribute visibility
Message-ID: <65fb9e128f7f1_aa22294b3@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <cover.1709922929.git.thomas.lendacky@amd.com>
 <b851e5f74398141bda2f5e6e1f0308cd13b39aa1.1709922929.git.thomas.lendacky@amd.com>
 <ZfIcjeIWaB5RzNye@google.com>
 <9ddaa371-2e42-456f-983f-1bd8d42d029e@amd.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <9ddaa371-2e42-456f-983f-1bd8d42d029e@amd.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?OvKHj1NAlQ1Avp+De18O3hbjO5pC5dpKBWfE34aTJ62+bfSRRwa8rF5kDSCC?=
 =?us-ascii?Q?CD/TI8dimhHWsOIewV060esYAKrDq7SsZFRYtVvRAIht0X1NiqYFQor+9eqI?=
 =?us-ascii?Q?MbdCTmgMCySzDul0D0akRUFp5Wl5xfofb/ifpukDVVIcc1OyqI7loZNOjhNc?=
 =?us-ascii?Q?l+ktrdo81lFAuA/ucfr/rZDCG+Ih9cHfp2Fkf4xa3TyrJGfOXEQdknFVZDk/?=
 =?us-ascii?Q?1HLhCSj0jjKfup4vMuIJhpE4up3KteR00kDsgwamXmeBH6ztPfkR41JFymlm?=
 =?us-ascii?Q?vmAuh+4o9IVdTcFmy13tALBS7h81eZAFIxnjmnOpbRcIsOP3h2zHdZC63x+W?=
 =?us-ascii?Q?Ge7HvtYCRO6LdciKXiB1UNvR+6aZzA0Q+zR+H/FN9G/GoSVVX7MFbVv/zpWi?=
 =?us-ascii?Q?+fQseqBT+qLt+9hGGd+CFsUOkadpsVEpzdRs3EHBL34aDWoK8OLy1WGewZ7s?=
 =?us-ascii?Q?l3XJlqyoCfdlksqaSSZ24S4GqPM+/TfR0VkVBGeujYh9+UIZoUViO4URNSBf?=
 =?us-ascii?Q?smd3Q8HJCbztQr9XSEJDzG3C/YwPh1NrlnYJKIkgaQySzmeWnzKVTuLk3eTI?=
 =?us-ascii?Q?cqBIeMbHyTeXGBlvhyV4leMLtaxee1ej40MVeMe5bi7UMbP0xmIGndkRUncb?=
 =?us-ascii?Q?2R+V4p8bww1UmI7B2FVT51LTbQOyTRC0p+jLsAkbUaASKdgPotbiXiWJC1T6?=
 =?us-ascii?Q?s8VYMjAxQB2KrEDGL/sHqFXdF60a3YGB/Vsv+RMLQwJvtMMYb3WfmDL4W5iD?=
 =?us-ascii?Q?oXrULb6qtNvbpC54E+M7rwaDy8R8t5wbMJcQAyNvsltqZuW1iSKTTLnRP8tr?=
 =?us-ascii?Q?8vPBk/YCFFLdWpMEfLl3BmBTPQIBCVIuk//vP3LnEQn8P0K0eFT1vFzYrrWz?=
 =?us-ascii?Q?SnyTmQGsl2iFJia/TZ6HExgU4l/95Nq/tMO3yHXKgg5MIq+gUbNBcBcwJGh2?=
 =?us-ascii?Q?p98ZJb781CsBalJ39sHd3+wAc2ad3xcfAF/WY3w/QT9f33aem1o+mIVVNuEK?=
 =?us-ascii?Q?sCQADYTrwF0r4NmMvIeMqVp92ZIfODc2HMEY2yuKhwvlzhviWUXt/5hUU3tG?=
 =?us-ascii?Q?9Why8OtE9Y09htyKMPtb50LYq5jkFaK3OcXd5IgMlPaSxHQOFNtAfhLD3jKw?=
 =?us-ascii?Q?34bW6t1VoLKcE/AVpYcrqVadzfFELWMSWwQcEw7MtdMKWYHfAmxUALc4UbEU?=
 =?us-ascii?Q?BT5/w7cIvfj04Cympq6Jw8l7H63Sr5eoIcd/GQD34nvsZ7X4P3sq81rBh8ti?=
 =?us-ascii?Q?N+fYS0bm3lSFmrjoeszjtuh2ygiwrNwgfR1pqxFhSBLvxpDB1bBSpUxlrcnq?=
 =?us-ascii?Q?Sa84e/Y7Zbhaa7NPSTklMP1sTI70Ty6Jgv4J/f7qZBJ0+o2VR72HKXQX3qLq?=
 =?us-ascii?Q?hWtZvmfTpFV+qasCYNrgZdbEdlR8MZomMBN2rvLuzrNX3uHVb0GoSNYzeq/d?=
 =?us-ascii?Q?Px0mUYm4iuiDfFYiaoDIGwKISwPIM5djI/l5LQt6yzkOX6KMX3YThJ0f4GvU?=
 =?us-ascii?Q?u0p1MHJtkhGElH7xE7RAvw/blvnvCKV7bYn0c5dMO/TXN2ruM5TxdwWEjU+4?=
 =?us-ascii?Q?VEOyhNkS1k39BIH3ADQZTongbCqikqT+yWWl6AtjzG23y3JeSUMh5XvH/8Ll?=
 =?us-ascii?Q?qg=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 58fe5ded-290e-4afd-7d72-08dc495040f4
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Mar 2024 02:40:21.6983
 (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: c0cv315ZZy1sx/anMqpiqqXiX7NPWAQJ5IDvhvASB0nn4zqEGPaK8PNFJpjL4SCp4S20gTZ0U5TZubVVnfuR/va2SnC1bZG7CBesEboSKVE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB7689
X-OriginatorOrg: intel.com

Tom Lendacky wrote:
> On 3/13/24 16:37, Joel Becker wrote:
> > On Fri, Mar 08, 2024 at 12:35:27PM -0600, Tom Lendacky wrote:
> >> In order to support dynamic decisions as to whether an attribute should be
> >> created, add a callback that returns a bool to indicate whether the
> >> attribute should be display. If no callback is registered, the attribute
> >> is displayed by default.
> > 
> > I'm curious what the strong value is in this extra callback.  As opposed
> > to not generating the attribute in the absence of a TPM (why create a
> > config_item at all?), merely having an empty response from the attribute,
> > or having `->show()` return -ENODEV or similar.
> 
> The value is to reduce the complexity of registering with the TSM support 
> across multiple vendors. There is a base set of attributes that are common 
> across vendors and some that are specific to vendors. Creating this 
> structure in the TSM support can get unwieldy. This would make it simple 
> to determine if support is provided since the attribute will either be 
> present or not.
> 
> This would also make the support similar to sysfs in the ability to 
> dynamically hide or show attributes.
> 
> > 
> >>
> >> Cc: Joel Becker <jlbec@evilplan.org>
> >> Cc: Christoph Hellwig <hch@lst.de>
> >> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> >> ---
> >>   fs/configfs/file.c       |  13 +++++
> >>   include/linux/configfs.h | 114 +++++++++++++++++++++++++++------------
> >>   2 files changed, 93 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/fs/configfs/file.c b/fs/configfs/file.c
> >> index 0ad32150611e..c758bcc11235 100644
> >> --- a/fs/configfs/file.c
> >> +++ b/fs/configfs/file.c
> >> @@ -451,6 +451,12 @@ int configfs_create_file(struct config_item * item, const struct configfs_attrib
> >>   	umode_t mode = (attr->ca_mode & S_IALLUGO) | S_IFREG;
> >>   	int error = 0;
> >>   
> >> +	if (attr->ca_is_visible) {
> >> +		mode = attr->ca_is_visible(item, attr);
> >> +		if (!mode)
> >> +			return 0;
> > 
> > What value do we get from carrying the mode through here?  The API
> > proposed is "visible or not", which is a boolean.  Overloading that with
> > "also set the mode" is confusing, and it also can lead to the divergent
> > codepath problem you mentioned in your response, where
> > `->ca_is_visible()` fails to return the mode correctly.  If this was simpl
> > a boolean hook, the code could read like so:
> 
> A boolean would work. There was a request to make this similar to the 
> sysfs attribute visibility. I certainly can make this a simple bool 
> function if that is preferable.

The inspiration was sysfs is_visible() and an idea to reproduce the
"static declaration + dynamic visibility" model that sysfs allows.
However, in the near term boolean visibility is sufficient since the
attributes are either on/off by vendor not read-write/read-only by
vendor implementation.